Potential integer underflow in file suffix generation #3783
Conversation
Integer underflow if maximum tile ID for leven is a 6-digit number. Signed-off-by: Philipp Voigt (<philipp.voigt@mercedes-benz.com>)
PhilippVoigt
changed the title
Added CHANGELOG.md entry Signed-off-by: Philipp Voigt (<philipp.voigt@mercedes-benz.com>)
Added CHANGELOG.md entry Signed-off-by: Philipp Voigt (<philipp.voigt@mercedes-benz.com>)
Moved CHANGELOG.md entry down Signed-off-by: Philipp Voigt (<philipp.voigt@mercedes-benz.com>)
Sorry, beginner's mistake. Thanks for reviewing this so quickly! |
No worries, it's not very intuitive, literally every first time contributor to Valhalla does that (which would speak for a PR template overhaul..) |
|
this is too perfect a test case to not actually add a test for it. i'm going to do so. perhaps we can also simplify the code a bit. |
I actually spent some time trying to add a test case but there is no way to reproduce it with the constants returned by |
|
Yeah its quite annoying because of the static use of tileheirarchy. I was able to do it locally though. i'll push an update to your branch with it after i confirm it fails on master |
…3 digits. master doesnt fail though
|
@PhilippVoigt i added a test here but i found that even master successfully passed the test! would you be able to tweak the test to actually test for the case that your code fixes? |
Added test case with GraphId that uses all 6 available digits Signed-off-by: Philipp Voigt (<philipp.voigt@mercedes-benz.com>)
The problematic TileIDs are those that fill all 6 available digits, so with TileID 123456, we can make it fail on master. I added an ASSERT_EXIT to make sure the test actually fails since it otherwise gets interrupted by SIGSEGV and doesn't report it's outcome. Is that fine for you or are there better ways to catch SIGSEGV? |
test/graphtile.cc
Outdated
| TileLevel level{7, valhalla::baldr::RoadClass::kSecondary, "half_degree_is_a_multiple_of_3", | ||
| Tiles<PointLL>{{{-180, -90}, {180, 90}}, .5, 1}}; | ||
| EXPECT_EQ(GraphTile::FileSuffix(GraphId(1234, 7, 0), ".qux", false, &level), "7/001/234.qux"); | ||
| ASSERT_EXIT((GraphTile::FileSuffix(GraphId(123456, 7, 0), ".qux", false, &level),exit(0)),::testing::ExitedWithCode(0),".*"); |
There was a problem hiding this comment.
You can remove the ASSERT_EXIT I guess. The point was that the below test line should fail master, we can't verify that anymore on this branch since you fixed it. It's enough for us when you say "jep, the below test fails master". Just verified it myself, you can safely remove this test line.
Confirmed that test fails on master and removed assert Signed-off-by: Philipp Voigt (<philipp.voigt@mercedes-benz.com>)
Confirmed that test fails on master and removed assert Signed-off-by: Philipp Voigt (<philipp.voigt@mercedes-benz.com>)
|
@kevinkreiser Is there a chance we could merge this in time for release 3.2.0? We currently depend on this fix and it would help us to base our work on a specific release. Thanks! |
|
Yeah we’re just having 2 weekly merge PR meetings. We go chronologically more or less. We won’t release before all the low hanging fruits like this PR aren’t merged, no worries:) I’ll add this one later to the list in the release issue |
fixes #3780
For certain level configurations the maximum tile ID can be a 6-digit number, e.g. for 512-by-265 tiles. In this case the separator at index 0 is added to the file suffix and the unsigned variable ind is reduced by 1 leading to a large positive number which causes an out-of-bounds in the subsequent loop. The default level configuration does not lead to 6-digit tile IDs, therefore the changes were tested solely for our own use cases, which might differ from yours.
Philipp Voigt (philipp.voigt@mercedes-benz.com) on behalf of Mercedes-Benz Tech Innovation GmbH.
Provider Information
Licensed under MIT