Fix ubsan error due to possible invalid shift exponent

[mandeepsandhu]

ubsan complained about the shift exponent in NodeInfo::heading() being
possibly too large under some conditions.

Fixed by checking for valid localidx. If invalid index is passed in, we
log a warning and return a heading of 0.

[kevinkreiser]

i saw this too but what i wonder is why is the local index out of range in the first place? specifically what changed? i know you can have tons of edges connecting at a node and go past the maximum that we support so the check you added is fine but what changed that exposed this particular situation?

[mandeepsandhu]

what changed that exposed this particular situation?

I haven't run bisect, so can't say which change introduced this bug. Was ubsan always on? or was it switched on recently?

[mandeepsandhu]

i saw this too but what i wonder is why is the local index out of range in the first place?

I see some places where we're looping the edge count and passing that as index into NodeInfo::heading (without capping it at 7)), so seems quite likely that this index goes out of valid range. eg: IsInternalIntersection passes the idx arg as-is into NodeInfo::heading (idx if fetch from a loop through the node's edges). There might be more instances of such usage.

[dnesbitt61]

This might only be used in map-matching. I am thinking we should use turn types there and then we wouldn't even need this

[kevinkreiser]

yep @mandeepsandhu i totally get why it happens i just dont get why it started happening in the check target semi-recently. something changing elsewhere must have caused it and thats what im interested in.

and yeah @dnesbitt61 has made this point before i think, perhaps we can just jetison it completely?

[dgearhart]

This might only be used in map-matching. I am thinking we should use turn types there and then we wouldn't even need this

@dnesbitt61 @kevinkreiser we use the intersecting edge begin heading - can not remove it

[dgearhart]

🚢 we will keep our eyes out for reason of recent failure

[mandeepsandhu]

yep @mandeepsandhu i totally get why it happens i just dont get why it started happening in the check target semi-recently. something changing elsewhere must have caused it and thats what im interested in.

As I said, a time-consuming git-bisect can telll us exactly when it started occurring. Initially I though usan must've been enabled recently, but I think its been on for some time now (unless some ubsan configuration changed recently).

and yeah @dnesbitt61 has made this point before i think, perhaps we can just jetison it completely?

[dnesbitt61]

Unfortunately we need it (even if we change map matching) to get the heading of intersecting edges along the path. If we remove it we would have to access other tiles (level 2 in particular) along the route which would slow things down considerably.This would undo much of the benefits of hierarchies.