Ubsan findings fixes #1
Conversation
| @@ -503,7 +503,7 @@ void DirectedEdge::set_superseded(const uint32_t superseded) { | |||
| if (superseded > kMaxShortcutsFromNode) { | |||
| LOG_WARN("Exceeding max shortcut edges from a node: " + std::to_string(superseded)); | |||
| } else { | |||
| superseded_ = (1 << (superseded - 1)); | |||
| superseded_ = superseded ? (1 << (superseded - 1)) : 0; | |||
There was a problem hiding this comment.
shift exponent 4294967295 is too large error here if superseded is zero
| // Make sure everything is 64 bit! | ||
| uint64_t shift = localidx * 8; // 8 bits per index | ||
| return static_cast<uint32_t>(std::round( | ||
| ((headings_ & (static_cast<uint64_t>(255) << shift)) >> shift) * kHeadingExpandFactor)); |
There was a problem hiding this comment.
Also a problem with shift exponent. At set function too big localidx is skipped (see the line).
I added the same check but I'm not 100% sure what the return should be.
There was a problem hiding this comment.
related. valhalla#2494
Why would we be calling this with a high index?
There was a problem hiding this comment.
can only store headings for 8 local indexes and the if statement protects if localindex > 7. The node based headings probably need to be deprecated. These headings are only used in map matching (could use simplified turn costing logic) and during data creation (can create temporary sequence files to look up headings to all edges).
SvetlanaBayarovich
force-pushed
the
ubsan-findings-fixes
branch
from
17138a6
to
7ecf5b4
Compare
src/mjolnir/graphvalidator.cc
Outdated
| @@ -631,7 +631,7 @@ void GraphValidator::Validate(const boost::property_tree::ptree& pt) { | |||
| } | |||
| sum += density; | |||
| } | |||
| float average_density = sum / densities[level].size(); | |||
| float average_density = densities[level].size() ? sum / densities[level].size() : 0.f; | |||
There was a problem hiding this comment.
For astar test only for level 2 densities[level].size() is non-zero. Can it be because of test map is not "realistic" or this is an expected result for some input data (e.g. small)?
| @@ -61,7 +61,7 @@ struct TrafficSpeeds { | |||
| }; | |||
|
|
|||
| // Convert big endian bytes to little endian | |||
| int16_t to_little_endian(const int16_t val) { | |||
| int16_t to_little_endian(const uint16_t val) { | |||
There was a problem hiding this comment.
To get rid of left shift in case of negative value. It also seems more logical if we treat the bytes to be swapped as something unsigned. It also helps to get rid of right shift of negative value which is implementation-defined (not UB but still...)
SvetlanaBayarovich
force-pushed
the
sanitizer-checks
branch
10 times, most recently
from
0d8bd68
to
671cd24
Compare
SvetlanaBayarovich
force-pushed
the
sanitizer-checks
branch
from
671cd24
to
451fdc2
Compare
SvetlanaBayarovich
force-pushed
the
sanitizer-checks
branch
2 times, most recently
from
cec4d8e
to
7545a5a
Compare
SvetlanaBayarovich
force-pushed
the
ubsan-findings-fixes
branch
from
7ecf5b4
to
0f41d17
Compare
SvetlanaBayarovich
force-pushed
the
sanitizer-checks
branch
from
ceab67b
to
e88cbd0
Compare
|
I measured no difference between 0f41d17 and master when running 23886 routes. |
SvetlanaBayarovich
force-pushed
the
sanitizer-checks
branch
from
e88cbd0
to
3558076
Compare
SvetlanaBayarovich
force-pushed
the
sanitizer-checks
branch
2 times, most recently
from
91764ae
to
55c18ae
Compare
|
Work moved to valhalla#2498 |
Issue
What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.
Resolves findings of the Undefined behavior sanitizer.
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?
It is currently rebased on my
sanitizer-supportbranch in order to show only relevant diff but include ubsan flags as well. To be rebased on the main branch later