Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ubsan findings fixes #2498

Merged
merged 10 commits into from
Aug 12, 2020
Merged

Ubsan findings fixes #2498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9000d5f
Fix division by zero
SvetlanaBayarovich Jul 22, 2020
4542088
fix negetive shift in encoding.h
SvetlanaBayarovich Jul 23, 2020
bd5a581
Fix too large shift exponent in nodeinfo.cc
SvetlanaBayarovich Jul 23, 2020
1f30e29
fix shift of negative value in valhalla_add_predicted_traffic.cc
SvetlanaBayarovich Jul 23, 2020
2bc3b5a
Update changelog
SvetlanaBayarovich Jul 29, 2020
58cbe10
Merge branch 'master' into ub-fixes
kevinkreiser Jul 31, 2020
4c09136
Revert "Fix too large shift exponent in nodeinfo.cc"
SvetlanaBayarovich Aug 3, 2020
d6b9a80
Merge remote-tracking branch 'upstream/master' into ub-fixes
SvetlanaBayarovich Aug 10, 2020
98ed89a
Merge remote-tracking branch 'upstream/master' into ub-fixes
SvetlanaBayarovich Aug 12, 2020
40f327e
Merge branch 'master' into ub-fixes
kevinkreiser Aug 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@
* FIXED: Fix missing nullptr checks in graphreader and loki::Reach (causing segfault during routing with not all levels of tiles availble) [#2504](https://github.com/valhalla/valhalla/pull/2504)
* FIXED: Fix mismatch of triplegedge roadclass and directededge roadclass [#2507](https://github.com/valhalla/valhalla/pull/2507)
* FIXED: Improve german destination_verbal_alert phrases [#2509](https://github.com/valhalla/valhalla/pull/2509)
* FIXED: Undefined behavior cases discovered with undefined behavior sanitizer tool. [2498](https://github.com/valhalla/valhalla/pull/2498)

* FIXED: Fixed logic so verbal keep instructions use branch exit sign info for ramps [#2520](https://github.com/valhalla/valhalla/pull/2520)
* FIXED: Fix bug in trace_route for uturns causing garbage coordinates [#2517](https://github.com/valhalla/valhalla/pull/2517)
* FIXED: Simplify heading calculation for turn type. Remove undefined behavior case. [#2513](https://github.com/valhalla/valhalla/pull/2513)
Expand Down
3 changes: 3 additions & 0 deletions src/mjolnir/graphvalidator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,9 @@ void GraphValidator::Validate(const boost::property_tree::ptree& pt) {
LOG_WARN((boost::format("Possible duplicates at level: %1% = %2%") % std::to_string(level) %
duplicates[level])
.str());
if (densities[level].empty()) {
continue;
}
// Get the average density and the max density
float max_density = 0.0f;
float sum = 0.0f;
Expand Down
4 changes: 2 additions & 2 deletions src/mjolnir/valhalla_add_predicted_traffic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ struct TrafficSpeeds {
};

// Convert big endian bytes to little endian
int16_t to_little_endian(const int16_t val) {
int16_t to_little_endian(const uint16_t val) {
return (val << 8) | ((val >> 8) & 0x00ff);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah so bitshifting signed numbers is implementation dependent so im not sure what was happening here. i'm not sure if we have any negative numbers in the input but if we do this change will definitely have an impact on what we do with the data. i'll follow up with you offline with some sample data to see if we need to worry about it

}

Expand Down Expand Up @@ -146,7 +146,7 @@ ParseTrafficFile(const std::vector<std::string>& filenames, stats& stat) {
traffic->second.coefficients.reserve(kCoefficientCount);
for (uint32_t i = 0, idx = 0; i < kCoefficientCount; ++i, idx += 2) {
traffic->second.coefficients.push_back(
to_little_endian(*(reinterpret_cast<const int16_t*>(&raw[idx]))));
to_little_endian(*(reinterpret_cast<const uint16_t*>(&raw[idx]))));
}
stat.compressed_count++;
} catch (std::exception& e) {
Expand Down
4 changes: 2 additions & 2 deletions valhalla/midgard/encoded.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ std::string encode(const container_t& points, const int precision = 1e6) {
// handy lambda to turn an integer into an encoded string
auto serialize = [&output](int number) {
// move the bits left 1 position and flip all the bits if it was a negative number
number = number < 0 ? ~(number << 1) : (number << 1);
number = number < 0 ? ~(static_cast<unsigned int>(number) << 1) : (number << 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is again bit shifting a signed number and in this case we know its negative. im surprised this doesnt change lots of stuff.. one good test for this would be to run valhalla_export_edges on a larger tileset since it dumps out the whole tilesets shape. then we can look at the diffs in there to see what this does. maybe we were just lucky on the platforms that we run that the implementation dependent handling was equivalent to the above? we need to test this more rigorously

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even more lucky, because implementation dependent is for right shift of negative value, for left it's UB.
What exactly do you suggest to test? As for correctness, there is a unit-test eg this one which does encoding-decoding and definitely creates the case with negative values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well my suggestion was as i stated:

one good test for this would be to run valhalla_export_edges on a larger tileset ...

yep i am aware of the unit test (i wrote the original), but its not extensive. the reason i want to do a broader test is because this touches literally everything. every single route, mapmatch, how the data is stored. this is a fundamental change so we better be sure its ok. same with the other one above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wrote a small program just to check int << 1 vs static_cast<unisigned int>(int) << 1 and it seems that at least on my computer the two operations have identical results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinkreiser I did the testing we discussed

  1. Build tiles
  2. export edges -> compare
  3. Add live traffic -> compare tiles
    for Utrecht tiles. Comparison showed no difference. Is there any testing to be done, eg regarding this comment ?

// write 5 bit chunks of the number
while (number >= 0x20) {
int nextValue = (0x20 | (number & 0x1f)) + 63;
Expand Down Expand Up @@ -134,7 +134,7 @@ template <class container_t> std::string encode7(const container_t& points) {
auto serialize = [&output](int number) {
// get the sign bit down on the least significant end to
// make the most significant bits mostly zeros
number = number < 0 ? ~(number << 1) : number << 1;
number = number < 0 ? ~(static_cast<unsigned int>(number) << 1) : number << 1;
// we take 7 bits of this at a time
while (number > 0x7f) {
// marking the most significant bit means there are more pieces to come
Expand Down