fix(guidance): osrm-routed aborted on a plaza vertex with more than 32 ways - #7697
Merged
Conversation
…2 ways EntryClass holds 32 roads. allowsEntry() asserted that the index was inside that and then shifted by it, so an index at or past the width was undefined: with asserts on it aborted the server, and with them off it returned whatever the shift produced. The caller cannot avoid asking. assembleSteps walks the bearings of an intersection and asks about each one, and there is nothing keeping the number of bearings inside the number of bits. An ordinary junction never comes close. A meshed pedestrian area does: every line of sight from a plaza vertex becomes a way, and extracting Ile-de-France reports 92399 roads that could not be recorded, up to road 95 at a single vertex on Place de la Sorbonne. So any route reported with steps that turned at such a vertex killed osrm-routed. That is every request the standard debug frontend makes. Answered rather than asserted now, the same way activate() already refuses an index it cannot store. A road that could not be recorded reports no entry, which is what the stored data says about it, and bearings and entry stay the same length so the API contract is unchanged. This does not widen the class. A vertex with 96 ways still has 64 of them reported as not enterable, which is wrong but is the limit the extractor already warns about, and widening the flags changes the on-disk format.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash/abort in OSRM guidance where EntryClass::allowsEntry() could assert and perform an out-of-range shift when queried with an index beyond its bit-capacity (e.g., intersections with >32 bearings in meshed pedestrian areas). The change makes allowsEntry() return false for out-of-range indices, aligning read behavior with activate()’s write-side capacity limit, and adds unit tests to prevent regressions.
Changes:
- Replace the
BOOST_ASSERTinEntryClass::allowsEntry()with a bounds check that returnsfalsefor unsupported indices (avoids UB / abort). - Add unit tests covering normal activation/reads and out-of-capacity queries (including large indices seen in the wild).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/util/guidance/entry_class.cpp | Prevents abort/UB by handling out-of-range indices safely in allowsEntry() |
| unit_tests/util/entry_class.cpp | Adds regression coverage for out-of-capacity entry queries and ensures in-range behavior remains correct |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+13
to
+14
| // How many roads the class can hold, which is how wide its flags are. | ||
| constexpr std::uint32_t CAPACITY = 32; |
Comment on lines
+21
to
+34
| // Answered rather than asserted, because the caller cannot avoid asking. | ||
| // | ||
| // A road beyond the ones this class can hold was never stored: activate() refuses it | ||
| // and the extractor logs that it did. The engine, though, walks the bearings, and | ||
| // there can be more of those than there are bits here. An ordinary junction never | ||
| // gets near the limit, but a meshed pedestrian area does: every line of sight from a | ||
| // plaza vertex is a way, and a vertex on a busy plaza has been seen with 96 of them. | ||
| // | ||
| // Shifting by the width of the type is undefined, so with asserts on this aborted the | ||
| // server and with them off it read whatever the shift happened to produce. Neither | ||
| // is an answer. A road that could not be recorded reports no entry, which is what | ||
| // the stored data says about it. | ||
| if (index >= CHAR_BIT * sizeof(FlagBaseType)) | ||
| return false; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7697 +/- ##
=======================================
Coverage 94.75% 94.75%
=======================================
Files 519 520 +1
Lines 41582 41604 +22
=======================================
+ Hits 39402 39423 +21
- Misses 2180 2181 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
No issue. Hit while driving a running
osrm-routedfrom the standard debug frontend againstan Ile-de-France extract built with
profiles/foot_area.lua.The symptom
The server dies. Not an error response, not a 500: the process aborts and every subsequent
request gets connection refused. With asserts compiled out it does not abort, it shifts by
the width of the type, which is undefined, and returns whatever that produced.
The defect
EntryClassholds 32 roads in astd::uint32_t.activate()already refuses an index itcannot store and returns false.
allowsEntry()asserted instead, then shifted:The caller cannot avoid asking.
assembleStepswalks the bearings of an intersection andasks about each one, and nothing keeps the number of bearings inside the number of bits:
classifyIntersectionwarns whenactivate()refuses a road but still adds its bearing tothe
BearingClass. So the two go out of step by design, and the read side was not ready for it.Why this only shows up now
An ordinary road junction never has 32 roads, so on a normal extract the gap never opens.
A meshed pedestrian area is a different shape entirely: every line of sight from a plaza
vertex becomes a way. Extracting Ile-de-France with
foot_area.lualogs 92,399 roadsthat could not be recorded, reaching road 95 at one vertex on Place de la Sorbonne, three
times what the class can hold.
That makes this reachable from any route reported with steps that turns at such a vertex,
which is every request the debug frontend makes (
steps=true). My own probing missed itentirely because I had been asking for
steps=false.The fix
Answer instead of asserting, mirroring
activate(). A road that could not be recordedreports no entry, which is exactly what the stored data says about it.
bearingsandentrystay the same length, so the API contract between those two arrays is unchanged.
What this deliberately does not fix
A vertex with 96 ways still has 64 of them reported as not enterable. That is wrong, but it
is the same limit the extractor already warns about 92,399 times, and it is not a crash.
Fixing it properly means one of: widening
EntryClass, which changes the on-disk format;or not presenting a meshed plaza vertex as a 96-branch intersection in the first place, which
is a guidance design question for areas. Both want their own decision rather than being
smuggled into a crash fix.
Testing
unit_tests/util/entry_class.cpp, two cases. The second reproduces the abort above exactly,same assertion and same line, and checks every index the engine might reach, including the
96-road case seen in the wild. It also checks the 32 roads that were recorded still read
back correctly, so the fix cannot be "make it always say false".
All fourteen unit suites pass. Verified live: with this, all 21 plaza crossings over seven
Paris plazas answer with
steps=trueand the server stays up, including the two URLs fromthe frontend that killed it.
Was this change primarily generated using an AI tool? Yes.
🤖 Claude Code, Claude Opus 5
Tasklist
Requirements / Relations
Independent of #7695, but you need both to use a meshing profile on a real extract: #7695
so extraction finishes at all, this one so the server survives being asked for steps.