[SRU] urdfdom-headers and urdfdom should not use locale dependent parsing for floating point numbers
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Urdfdom Headers |
Fix Released
|
Unknown
|
|||
urdfdom (Ubuntu) |
Fix Released
|
Low
|
Unassigned | ||
Bionic |
Fix Released
|
Low
|
Unassigned | ||
urdfdom-headers (Ubuntu) |
Fix Released
|
Low
|
Unassigned | ||
Bionic |
Fix Released
|
Low
|
Unassigned |
Bug Description
[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:/
* The dockerfile here https:/
- 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:/
[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_
* The bug was reported in Nov 2017 (https:/
[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:/
- urdfdom-headers debdiff
https:/
https:/
https:/
https:/
https:/
https:/
https:/
See also the following upstream bug reports:
https:/
https:/
and a few downstream bug reports to the ubuntu package:
https:/
https:/
https:/
https:/
https:/
https:/
Changed in urdfdom-headers: | |
status: | Unknown → New |
description: | updated |
Changed in urdfdom-headers: | |
status: | New → Unknown |
description: | updated |
Changed in urdfdom (Ubuntu): | |
status: | Confirmed → Fix Released |
Changed in urdfdom-headers (Ubuntu): | |
status: | Confirmed → Fix Released |
Changed in urdfdom (Ubuntu): | |
importance: | Undecided → Low |
Changed in urdfdom (Ubuntu Bionic): | |
importance: | Undecided → Low |
Changed in urdfdom-headers (Ubuntu): | |
importance: | Undecided → Low |
Changed in urdfdom-headers (Ubuntu Bionic): | |
importance: | Undecided → Low |
tags: |
added: verification-done-bionic removed: verification-needed verification-needed-bionic |
Changed in urdfdom-headers: | |
status: | Unknown → Fix Released |
Status changed to 'Confirmed' because the bug affects multiple users.