Fix undefined behavior related to date parsing #3278
Conversation
|
https://github.com/valhalla/valhalla/blob/master/src/thor/bidirectional_astar.cc#L512-L513 can you check why that isnt happening in your case? |
src/thor/route_action.cc
Outdated
| @@ -455,7 +456,8 @@ void thor_worker_t::path_arrive_by(Api& api, const std::string& costing) { | |||
|
|
|||
| for (auto& temp_path : temp_paths) { | |||
| // back propagate time information | |||
| if (destination->has_date_time() && options.date_time_type() != valhalla::Options::invariant) { | |||
| if (destination->has_date_time() && options.date_time_type() != valhalla::Options::invariant && | |||
| options.date_time_type() != valhalla::Options::current) { | |||
There was a problem hiding this comment.
i mentioned this in another comment but the proper fix is to figure out why your case didnt change the date_time string from current into a proper date_time string
There was a problem hiding this comment.
Hi @kevinkreiser! I have a proper fix for that, but do not have rights to push to this repository
There was a problem hiding this comment.
@oleg-liatte you've been invited
|
As @SiarheiFedartsou said, UB was caused by date_time string conversion failure: default zero time was adjusted by time zone to negative side, causing this way unsigned overflow. The problem was that Turned creating a local copies into a references. |
|
Yep this is a result of a recent refractor. Sadly it was very hard to catch this mistake during review: https://github.com/valhalla/valhalla/pull/2987/files#diff-93d0749f9c1f6cd0bc29f8b7db15520349e887fc8aa7bbcbe26475b14fcc9b88R434 |
|
:tableflip: that's it, I'm never trying to fix anything ever again. Goddamn output parameters.... |
|
@danpat sadly its just a fact of complicated life/code, dont beat yourself up about it |
Issue
We found this issue using undefined behavior sanitizer:
Why it happened in our case:
offset_dateis called within_dt == "current".DateTime::seconds_since_epochis called withdate_time == "current".UINT_MAX. That's why we run into UB duringstd::chrono::...related operations.I'm absolutely not confident if it should be fixed this way, so want to hear opinion of @kevinkreiser first before covering it with tests etc.
Also related question: I see that Valhalla uses time zone information. Which features actually require it? Why I am asking, I see potential problem here in our use case: we use Valhalla on mobile phones and can download tiles generated with newer Valhalla version than we currently have on-board. Newer Valhalla can potentially have updated time zone database, i.e.
NodeInfo::timezonecan be wrong/out-of-bounds for our local time zone database. Is there easy way to simply turn off features which use this timezone information for our use case? Or probably easy way to bake this time zone info into tiles?Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?