Avoid UB on converting NaN to int in annotations #2992

averkhaturau · 2021-04-10T07:45:10Z

Issue

Speed annotation may fail with converting NaN to integer if the same geometry point in duplicated in a route.

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Update the docs with any new request parameters or changes to behavior described
Update the changelog

Requirements / Relations

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

src/thor/triplegbuilder.cc

kevinkreiser · 2021-04-10T11:10:24Z

Why does the duplicate occur? I thought the cutting logic was robust to this which means it has to be duplicated shape in OSM itself? If so that is an easy gruka test to write to actually get test coverage here. Shouldn't we add that quickly before merging?

danpat

Can someone add a test for this case? @kevinkreiser yeah, this is occuring when the underlying OSM data contains a zero-length, duplicate-coordinate node.

purew · 2021-04-12T20:19:36Z

test/shape_attributes.cc

+TEST(ShapeAttributes, test_shape_attributes_duplicated_point) {
+  tyr::actor_t actor(conf);
+
+  auto result_json = actor.trace_attributes(


This test passes on master as well...

@purew the test will pass, but is ubsan triggered?

I'm not sure how to test UB without ubsan, I'll enable it on CI

it would be fine to locally on your computer:

undo the fix in triplegbuilder but keep the unit test

turn on ubsan locally and run the unit test

show that the sanitizer finds the issue via the unit test and post it to this PR

we shouldnt change the behavior of CI in this PR, iirc when we had that enabled we either got randomly OOM killed or the build took forever

In my opinion, CI should test valhalla for UB, it would help to avoid UB issues in future, It's not the only issue found with ubsan.

kevinkreiser · 2021-04-12T21:54:16Z

i wonder if this line is a contributing factor:

double speed = (edge->length() * (tgt_pct - src_pct)) / edge_seconds;

maybe speed is 0 if at the beginning or end of a map match we use 0% of an edge or maybe edge_seconds is 0. @averkhaturau i assume you ran into this sometime. did you happen to debug it to see the root cause of the nan?

averkhaturau · 2021-04-13T08:57:43Z

did you happen to debug it to see the root cause of the nan?

In short, if we have a duplicated coordinate in route geometry (we usually do in multileg routes), the speed is calculated as
speed = length / time
Both length and time are 0., so speed is set to NaN. Then it converts to integer in call of set_speed(int), which is undefined behavior. Happily (or actually unhappily) clang converts NaN to integer 0 and nothing awful happens. ubsan catches it.

So, the reason is actually a duplicated coordinate.

kevinkreiser · 2021-04-13T14:08:21Z

I do not understand your comment about multileg routes! TripLegBuilder only works on one leg at a time, so surely multileg routes cannot cause this problem. I'm just wondering where the point duplication comes in. Is it in the base OSM data like that or do we have a bug elsewhere that is erroneously repeating a coordinate (ie when cutting the shape for the first or last edge of the route, also done in triplegbuilder)? its good to handle it because OSM could have duplicate adjacent points (and im certain we dont weed that out at tile building time), but my main question is what is causing duplicated points and do we need to fix something more in a separate PR

merkispavel · 2021-04-13T14:19:26Z

I do not understand your comment about multileg routes! TripLegBuilder only works on one leg at a time, so surely multileg routes cannot cause this problem. I'm just wondering where the point duplication comes in. Is it in the base OSM data like that or do we have a bug elsewhere that is erroneously repeating a coordinate (ie when cutting the shape for the first or last edge of the route, also done in triplegbuilder)? its good to handle it because OSM could have duplicate adjacent points (and im certain we dont weed that out at tile building time), but my main question is what is causing duplicated points and do we need to fix something more in a separate PR

@kevinkreiser as far as i see we do have a request that reproduces the issue. I'll come back to you soon and let you know if we need one more fix somewhere else. But I believe this activity is not a blocker for this PR

averkhaturau · 2021-04-13T14:22:34Z

This PR is a hot workaround of UB. Duplicated point is a separate issue.

kevinkreiser · 2021-04-13T14:24:54Z

yeah im not trying to block the PR. let me reiterate my points more concisely:

its good to handle it because OSM could have duplicate adjacent points

^^ we should ship this to protect us from NaN regardless of the cause

what is causing duplicated points and do we need to fix something more in a separate PR

^^ lets investigate the cause of the duplicate adjacent shape points in a separate issue/PR

we shouldnt change the behavior of CI in this PR

^^ revert the changes to CI

averkhaturau · 2021-04-13T14:33:36Z

@kevinkreiser , I reverted CI changes. Should I revert a test as well?

merkispavel

Looks good

danpat · 2021-04-13T15:28:58Z

Duplicated coordinates can easily exist in the original OSM data - I don't think there's any need to look for another cause here. A simple scan of OSM I'm sure will expose thousands, if not millions, of cases of this. Valhalla needs to be robust against that, and as @averkhaturau pointed out, we've been lucky that it just so happens that NaN->0 for our serialization in most cases.

averkhaturau requested a review from danpat April 10, 2021 07:45

averkhaturau force-pushed the av-avoid-NaN-in-annotations branch from 01da0b6 to 8491c1f Compare April 10, 2021 07:48

averkhaturau requested a review from purew April 10, 2021 08:08

SiarheiFedartsou reviewed Apr 10, 2021

View reviewed changes

src/thor/triplegbuilder.cc Outdated Show resolved Hide resolved

SiarheiFedartsou previously approved these changes Apr 10, 2021

View reviewed changes

averkhaturau dismissed SiarheiFedartsou’s stale review via b0e0f58 April 10, 2021 10:01

averkhaturau force-pushed the av-avoid-NaN-in-annotations branch 2 times, most recently from b0e0f58 to 54c8af6 Compare April 10, 2021 10:02

SiarheiFedartsou previously approved these changes Apr 10, 2021

View reviewed changes

danpat requested changes Apr 10, 2021

View reviewed changes

averkhaturau dismissed SiarheiFedartsou’s stale review via 2267060 April 12, 2021 19:55

purew reviewed Apr 12, 2021

View reviewed changes

averkhaturau force-pushed the av-avoid-NaN-in-annotations branch from 18719f1 to 1f5fc7d Compare April 13, 2021 08:58

averkhaturau added 2 commits April 13, 2021 17:32

Avoid NaN in annotations

c28121e

Add test of shape_attributes on duplicated points

98beccc

averkhaturau force-pushed the av-avoid-NaN-in-annotations branch from f66383b to 98beccc Compare April 13, 2021 14:32

merkispavel approved these changes Apr 13, 2021

View reviewed changes

danpat self-requested a review April 13, 2021 15:27

danpat approved these changes Apr 13, 2021

View reviewed changes

merkispavel merged commit 8d66f40 into master Apr 13, 2021

purew deleted the av-avoid-NaN-in-annotations branch April 14, 2021 17:25

dnesbitt61 mentioned this pull request Apr 16, 2021

Performance Drop in Long Bicycle Route #3007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid UB on converting NaN to int in annotations #2992

Avoid UB on converting NaN to int in annotations #2992

averkhaturau commented Apr 10, 2021

kevinkreiser commented Apr 10, 2021

danpat left a comment •

edited

Loading

purew Apr 12, 2021

danpat Apr 13, 2021

averkhaturau Apr 13, 2021

kevinkreiser Apr 13, 2021 •

edited

Loading

averkhaturau Apr 13, 2021

kevinkreiser commented Apr 12, 2021

averkhaturau commented Apr 13, 2021

kevinkreiser commented Apr 13, 2021 •

edited

Loading

merkispavel commented Apr 13, 2021 •

edited

Loading

averkhaturau commented Apr 13, 2021

kevinkreiser commented Apr 13, 2021 •

edited

Loading

averkhaturau commented Apr 13, 2021

merkispavel left a comment

danpat commented Apr 13, 2021

Avoid UB on converting NaN to int in annotations #2992

Avoid UB on converting NaN to int in annotations #2992

Conversation

averkhaturau commented Apr 10, 2021

Issue

Tasklist

Requirements / Relations

kevinkreiser commented Apr 10, 2021

danpat left a comment • edited Loading

Choose a reason for hiding this comment

purew Apr 12, 2021

Choose a reason for hiding this comment

danpat Apr 13, 2021

Choose a reason for hiding this comment

averkhaturau Apr 13, 2021

Choose a reason for hiding this comment

kevinkreiser Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

averkhaturau Apr 13, 2021

Choose a reason for hiding this comment

kevinkreiser commented Apr 12, 2021

averkhaturau commented Apr 13, 2021

kevinkreiser commented Apr 13, 2021 • edited Loading

merkispavel commented Apr 13, 2021 • edited Loading

averkhaturau commented Apr 13, 2021

kevinkreiser commented Apr 13, 2021 • edited Loading

averkhaturau commented Apr 13, 2021

merkispavel left a comment

Choose a reason for hiding this comment

danpat commented Apr 13, 2021

danpat left a comment •

edited

Loading

kevinkreiser Apr 13, 2021 •

edited

Loading

kevinkreiser commented Apr 13, 2021 •

edited

Loading

merkispavel commented Apr 13, 2021 •

edited

Loading

kevinkreiser commented Apr 13, 2021 •

edited

Loading