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.

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

Fix segfault: Do not combine last turn channel maneuver #2463

Merged
merged 6 commits into from
Jul 17, 2020

Conversation

danpaz
Copy link
Collaborator

@danpaz danpaz commented Jul 10, 2020

Issue

Fixes an invalid access bug triggered by combining maneuvers at a turn channel that is also a roundabout exit. The fix is to simply skip the turn channel collapsing logic if the next maneuver is the last maneuver in the route.

Although the turn channel collapsing logic is not new and the bug has been in valhalla for a while, the segfault actually occurs further along in new code that was added to collapse roundabout exit maneuvers. The combination of collapsing the final maneuver, then collapsing again for roundabout_exits=false, triggers the bug.

Address sanitizer output: asan.txt

We haven't been able to craft a reproducible test case in gurka yet as it's been tricky getting the tags just right for a link to be marked as a turn_channel. I'll follow up with a test case in a future PR.

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Generally use squash merge to rebase and clean comments before merging
  • Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

gknisely
gknisely previously approved these changes Jul 10, 2020
@@ -2336,7 +2336,7 @@ bool ManeuversBuilder::IsTurnChannelManeuverCombinable(std::list<Maneuver>::iter
bool start_man) const {

// Current maneuver must be a turn channel and not equal to the next maneuver
if (curr_man->turn_channel() && (curr_man != next_man)) {
if (curr_man->turn_channel() && (curr_man != next_man) && !next_man->IsDestinationType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we leave a TODOD note that we need to extend this logic (of not combining with the destination maneuver) to other places as well?

Are there tests which exercise this code-path when roundabout_exits is true & false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm actually not sure where it makes sense to extend this logic, as it seems to have been deliberately left out of the other collapse cases. I'll follow up with @dgearhart about that as soon as I can.

Re: tests, I mentioned in the OP that I'll follow up with a gurka test. Meanwhile @gknisely helped run RAD tests with this change and saw no diffs, so I'm fairly confident the fix is localized to the situation that triggers the bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. RAD showing 0 diffs is a good sign! :) 👍

mandeepsandhu
mandeepsandhu previously approved these changes Jul 10, 2020
@danpaz
Copy link
Collaborator Author

danpaz commented Jul 13, 2020

@gknisely thank you for adding the gurka test! I confirmed it fails on master due to missing the destination maneuver:

Expected equality of these values:
  actual_maneuvers
    Which is: { 1, 20, 26, 27 }
  expected_maneuvers
    Which is: { 1, 20, 26, 27, 5 }
Actual maneuvers didn't match expected maneuvers

@danpaz danpaz merged commit 71cf5b8 into master Jul 17, 2020
@danpaz danpaz deleted the fix_roundabout_exit_segfault branch July 17, 2020 14:38
yuzheyan added a commit that referenced this pull request Jul 28, 2020
* 'master' of github.com:valhalla/valhalla:
  Fix segfault: Do not combine last turn channel maneuver (#2463)
  Update ja-JP.json roundabout phrases (#2474)
  Update ja-JP.json (#2471)
  Add Dutch locale (#2464)
  Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants