[SRU] urdfdom-headers and urdfdom should not use locale dependent parsing for floating point numbers

[simonschmeisser]

[Impact]

* The parsing done by the code of urdfdom truncate floating values on systems that are not using the locales defined by default on English systems. The users of this system are getting wrong values which are specially sensible on variables with a range between 0 and 1.

* The URDF schema defines doubles in URDF as xs:double. Looking at the XSD document it says that the mantissa for a double is represented as a "decimal" number. And looking at the XSD document a decimal number is a "finite-length sequence of decimal digits (#x30-#x39) separated by a period as a decimal indicator". Thus, the locale should have no effect on how the document is parsed, and using std::stod is incorrect. This patch switches the Vector3 class to using the stream operator, which seems to ignore locale.

* urdfdom-headers is a key part for the largest open source robotics framework, ROS (Robot Operative System). So it potentially affects a big part of the robotics open source community. We (the devs and maintainers of ROS have been receiving many different issues or bugs coming from this problem, as Simon wrote in the initial post of this bug.

[Test Case]

* We have prepared a test case that demonstrate the problem and the fix. Patched packages for both urdfdom and urdfdom_headers are in this ppa:
https://launchpad.net/~j-rivero/+archive/ubuntu/urdfdom-headers-sru2/+packages

* The dockerfile here https://github.com/j-rivero/sru_urdfdom/blob/master/Dockerfile tries to demonstrate the following:
 - Setup the test case, install packages
 - Import locale test from the latest development of urdfdom repo
 - Run tests, failed (demonstration of the bug)
 - Setup the PPA, install new version of packages
 - Run test, success (demonstration of the fix)

* This is the testing build of the dockerfile
https://build.osrfoundation.org/job/_urdfdom_sru/15/console

[Regression Potential]

* The change affected the result of parsed values from using stof/stod functions to use stringstream. Although the patch is not small, the same pattern is replied on every chunk of code changed. Standard use case of users setting up digits in the form of integer or double is unlikely to generate big regressions. Alternative use cases of users with weird values in the input would trigger an exception from the code. This is no different than the exceptions thrown by previous stof/stod funtions (std::invalid_argument or std::out_of_range).

* The bug was reported in Nov 2017 (https://github.com/ros/urdfdom_headers/issues/41) the ROS community and fixes landed in upstream repositories few weeks after. Many users from different countries have been using new versions or these patches since them, some of them are commenting/voting in this bug. Note that we were also playing with upstream test suite to build the test case and no regression was found in there.

[Other Info]

* Upstream releases new patched versions with the fix that I’ve included into Debian Sid and have been synced by Ubuntu starting from Disco: 1.0.3-1

* debdiffs for versions from the PPA:
 - urdfdom debdiff:
   https://gist.github.com/j-rivero/9983dddc6dbbea99cd3c39774f1d0498

 - urdfdom-headers debdiff
   https://gist.github.com/j-rivero/bc563620802d78129d775b2d5aee69ac

https://github.com/ros/urdfdom/commit/3dc7ee812827cc69ffa457ef01fe7b9623096aed
https://github.com/ros/urdfdom_headers/commit/9d2b421f3fcbc2a32af40b99ebd9c2cb2d088fb9
https://github.com/ros/urdfdom_headers/pull/42
https://github.com/ros/urdfdom/pull/105
https://github.com/ros/urdfdom_headers/pull/47
https://github.com/ros/urdfdom/pull/115

See also the following upstream bug reports:
https://github.com/ros/urdfdom_headers/issues/45
https://github.com/ros/urdfdom/issues/119

and a few downstream bug reports to the ubuntu package:
https://github.com/ros-planning/moveit/issues/1333
https://github.com/ros/urdf/issues/21
https://github.com/ros-visualization/rviz/issues/1249
https://github.com/ros-visualization/rviz/issues/1151
https://github.com/ros-planning/moveit/issues/1050
https://github.com/ros-visualization/rviz/issues/1298

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Martin Pecka (peci1)]

Workaround until the release actually happens: set LC_NUMERIC=C in your .bashrc.

[Victor Lamoine (victor-lamoine)]

This is affecting me. This bug is very nasty because it is hard to trace the origin and many users are affected.

[Matthias (redkite)]

Is there a chance to still update this for Bionic? It's likely the largest user base since the latest stable ROS release is based on Bionic.

[Jose Luis Rivero (j-rivero)]

Thanks Simon for submitting the bug and the help to prepare this comment. SRU bugs in Ubuntu require to follow closely the instructions provided https://wiki.ubuntu.com/StableReleaseUpdates. Here we go:

[Impact]

* The parsing done by the code of urdfdom truncate floating values on systems that are not using the locales defined by default on English systems. The users of this system are getting wrong values which are specially sensible on variables with a range between 0 and 1.

* The URDF schema defines doubles in URDF as xs:double. Looking at the XSD document it says that the mantissa for a double is represented as a "decimal" number. And looking at the XSD document a decimal number is a "finite-length sequence of decimal digits (#x30-#x39) separated by a period as a decimal indicator". Thus, the locale should have no effect on how the document is parsed, and using std::stod is incorrect. This patch switches the Vector3 class to using the stream operator, which seems to ignore locale.

* urdfdom-headers is a key part for the largest open source robotics framework, ROS (Robot Operative System). So it potentially affects a big part of the robotics open source community. We (the devs and maintainers of ROS have been receiving many different issues or bugs coming from this problem, as Simon wrote in the initial post of this bug.

[Test Case]

* We have prepared a test case that demonstrate the problem and the fix. Patched packages for both urdfdom and urdfdom_headers are in this ppa:
https://launchpad.net/~j-rivero/+archive/ubuntu/urdfdom-headers-sru2/+packages

* The dockerfile here https://github.com/j-rivero/sru_urdfdom/blob/master/Dockerfile tries to demonstrate the following:
 - Setup the test case, install packages
 - Import locale test from the latest development of urdfdom repo
 - Run tests, failed (demonstration of the bug)
 - Setup the PPA, install new version of packages
 - Run test, success (demonstration of the fix)

* This is the testing build of the dockerfile
https://build.osrfoundation.org/job/_urdfdom_sru/15/console

[Regression Potential]

* The change affected the result of parsed values from using stof/stod functions to use stringstream. Although the patch is not small, the same pattern is replied on every chunk of code changed. Standard use case of users setting up digits in the form of integer or double is unlikely to generate big regressions. Alternative use cases of users with weird values in the input would trigger an exception from the code. This is no different than the exceptions thrown by previous stof/stod funtions (std::invalid_argument or std::out_of_range).

* The bug was reported in Nov 2017 (https://github.com/ros/urdfdom_headers/issues/41) the ROS community and fixes landed in upstream repositories few weeks after. Many users from different countries have been using new versions or these patches since them, some of them are commenting/voting in this bug. Note that we were also playing with upstream test suite to build the test case and no regression was found in there.

[Other Info]

* Upstream releases new patched versions with the fix that I’ve included into Debian Sid and have been synced by Ubuntu sta...

[Julian Andres Klode (juliank)]

Some review:

- debdiffs should be attached to the bug (not a gist)
- version number should follow https://wiki.ubuntu.com/SecurityTeam/UpdatePreparation#Update_the_packaging - e.g. 2ubuntu0.1, not 2ubuntu1

Unsubscribing ubuntu-sponsors. Once the issues have been addressed, please resubscribe.

Personally, I'd also keep the two commits in the one patch separate, makes it easier to check

I find the impact too long to read (but it's late). I don't fully comprehend the test cases - is the library header-only so that it needs a rebuild? Otherwise, what happens to existing binaries.

[Kyle Fazzari (kyrofa)]

This patch is complex enough to make folks a little uncomfortable with the SRU. Beyond Julian's suggestions (thanks Julian), Jose, are you willing to commit to thoroughly testing this and resolving any autopkgtest regressions?

[Jose Luis Rivero (j-rivero)]

@Julian: thanks very much for reviewing.

1. I attach debdiffs to the bug
2. Fixed release version number in the debdiffs to be 0.1
3. I've changed the patching to have one commit in each patch
4. Upstream split the library in two repositories one for the headers and one for the libraries, this SRU covers both. Not usual but the packaging was done using this same division done upstream. With this in mind I think that the test case is a bit more clear but feel free to ask if you need more information.

@Kyle: I'm happy to help with any regression that could appear of any other problem we found.

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Julian Andres Klode (juliank)]

Fixed some minor issues (missing LP: in changelog and update-maintainer for -headers), and uploaded it.

[Julian Andres Klode (juliank)]

For SRU team review: The liburdfdom-headers-dev (>= 1.0.0-1ubuntu0.1) build-depends bump in urdfdom is needed because urfdom now uses the new function added in the header upload.

[Jose Luis Rivero (j-rivero)]

Thanks Julian for the fixes. Let us know if we can help with the SRU verification once the testing packages have been generated.

[Seth Arnold (seth-arnold)]

Julian, can you take another look at this bug? Your comments here give me the impression you've uploaded the package, but I only see the version in the -release pocket in Bionic:

https://launchpad.net/ubuntu/+source/urdfdom-headers
https://launchpad.net/ubuntu/+source/urdfdom

Are there any blockers left? The debdiffs looked good to me, the changes are in focal. Even though the urdfdom-headers changes are to a header file and inlined methods, I don't see any Built-Using: lines that would indicate any other packages need to be rebuilt afterwards. (Is this correct? Or is this a failing of the tooling?)

Thanks

[Chris Halse Rogers (raof)]

Hello simonschmeisser, or anyone else affected,

Accepted urdfdom into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/urdfdom/1.0.0-2ubuntu0.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Chris Halse Rogers (raof)]

Hello simonschmeisser, or anyone else affected,

Accepted urdfdom-headers into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/urdfdom-headers/1.0.0-1ubuntu0.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[simonschmeisser (s-schmeisser)]

Hello Chris (raof), Matthew (mathew-hodson) and others involved,

I enabled bionic-proposed yesterday evening, installed both new packages (urdfdom and urdfdom-headers) and can confirm the bugs fixed. Thanks a lot to everybody involved!

I was not able to find any fields where I could set "verification-done-bionic"

Do you have any hints on how to speed up similar bug fixes in future? (Besides making sure Debian has current releases before the debian import freeze)

Cheers
Simon

[Kyle Fazzari (kyrofa)]

Thanks for testing, Simon!

> I was not able to find any fields where I could set "verification-done-bionic"

Right under the bug description you'll see a set of tags that you can edit.

[Martin Pecka (peci1)]

I've just noticed that the version number stayed the same - 1.0.0. Now how can I recognize e.g. in CMake whether the user has the new fixed version of this package or the old one without the fix?

[simonschmeisser (s-schmeisser)]

That's why I would have preferred a simple update to 1.0.3 patch release ... it's not only cmake but also a support issue as now you have differentiate between upstream 1.0.0 and ubuntu 1.0.0

I just hope that as soon as this hits the update channels most/all people will install it automatically and we never hear about this bug again :)

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for urdfdom has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package urdfdom - 1.0.0-2ubuntu0.1

---------------
urdfdom (1.0.0-2ubuntu0.1) bionic; urgency=medium

  * Patch to avoid truncating problems on non english locale systems (LP: #1817595)

 -- Jose Luis Rivero <email address hidden> Tue, 06 Aug 2019 17:51:52 +0000

[Launchpad Janitor (janitor)]

This bug was fixed in the package urdfdom-headers - 1.0.0-1ubuntu0.1

---------------
urdfdom-headers (1.0.0-1ubuntu0.1) bionic; urgency=medium

  * Patches to avoid truncating problems on non english locale systems (LP: #1817595)

 -- Jose Luis Rivero <email address hidden> Thu, 08 Aug 2019 17:49:30 +0000