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

kraken: Fixing undefined behaviour and crash of A* #2846

Merged
merged 3 commits into from
Jul 8, 2019

Conversation

AurelienLP
Copy link
Contributor

@AurelienLP AurelienLP commented Jun 28, 2019

This PR MUST be merged AFTER #2843.

Only the commit "Fixing A* init by checking if starting edge is found" needs to be reviewed since the first one is the un-revert of the A*.

During A* init, we initialize costs vector for the starting edge. However, if this edge does not exist, its index is either undefined or -1 (max of std::size_t) creating crash or undefined behavior.

We fixed that checking if starting.edge is found. We also access cost vector with "at" and not the operator "[]" which checks the index and the bound of the vector, throwing an exception instead of crashing.

JIRA: https://jira.kisio.org/browse/NAVITIAII-2761

Copy link
Contributor

@pbougue pbougue left a comment

Choose a reason for hiding this comment

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

Just reviewed last commit.

We may as well just keep this PR and not #2843 to avoid remerging unfixed A*

As mentioned orally :

  1. It can be worth running artemis + artemis_idfm on that (let you decide)
  2. Just curious of impact on perf if you know it (probably not worth rerunning bench just for that)

@AurelienLP
Copy link
Contributor Author

  1. Currently building the PR to run artemis + artemis_idfm
  2. It should be negligible since we are not in a loop

@codecov-io
Copy link

codecov-io commented Jun 28, 2019

Codecov Report

Merging #2846 into dev will decrease coverage by 0.8%.
The diff coverage is 94.4%.

@@           Coverage Diff           @@
##             dev   #2846     +/-   ##
=======================================
- Coverage   87.6%   86.8%   -0.9%     
=======================================
  Files        438     446      +8     
  Lines      66672   60121   -6551     
=======================================
- Hits       58433   52192   -6241     
+ Misses      8239    7929    -310

compute_cost_from_starting_edge_to_dist(starting_edge[source_e], dest_projected_coord);
costs[starting_edge[target_e]] =
compute_cost_from_starting_edge_to_dist(starting_edge[target_e], dest_projected_coord);
if (starting_edge.found) {
Copy link
Contributor

@nonifier nonifier Jul 1, 2019

Choose a reason for hiding this comment

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

This is a good fix, but it doesn't prevent us from doing the same mistake ever again.....
I would be in favour of raising an exception in ProjectionData::operator[] if found isn't set.
Or you could change the way ProjectionData::vertices is implemented and make sure it's always in a state that will not bring you to Undefined Behavior Land.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated my PR to throw an exception in ProjectionData::operator[] when found is false ! :D

@AurelienLP AurelienLP requested a review from nonifier July 1, 2019 16:25
@pbougue
Copy link
Contributor

pbougue commented Jul 2, 2019

Not sure: Is it possible to add a test that would trigger the bug before merge?
I think of 3 "situations":

  1. before any fix : doesn't crash (or just using sanitizer) : return wrong result?
  2. using .at() crashes
  3. after all fix : doesn't crash, return no-result as expected?

It may be nice to have the "functional case" that triggered the bug pinned in a test.

@AurelienLP
Copy link
Contributor Author

I am trying to create an artemis test for this PR. However, I couldn't reproduce the crash without the fix. We can only create a test that returns "no solution found for this journey" when the starting edge is not found (and check that no crash happens). This is what I am trying to do now :)

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.

5 participants