fix(xodr): never dereference a stale road link when writing (#311) - #330
Merged
Conversation
`write_link_element` dereferenced `network.road(*road_target)->odr_id` with no null check, so serializing a road whose predecessor/successor target had been erased segfaulted. `RoadNetwork::erase_road` deliberately drops only the junction connections that name the erased road (road/network.hpp) — surviving connecting roads keep a `predecessor`/`successor` holding the now-stale `RoadId`, which is exactly the state issue #311 reproduces by erasing a junction arm. `erase_junction` leaves the symmetric hole on the `JunctionId` arm of the variant. The editor's `edit::delete_road` closure repairs both in practice, so this is the defence-in-depth guard the export path was missing; a hand-edited or foreign .xodr reaches it without any closure at all. The junction surface and boundary paths were already guarded (`arm_face`, `link_arm`, `arm_face_vertices` all null-check), so the crash was plain road-link serialization reached via the dangling arm. - `link_target_odr_id` resolves a link to its `@elementId` or nullptr, covering both arms of the target variant, never dereferencing. - `write_road` resolves both ends before opening `<link>`, so a stale link is dropped and an all-stale road emits no empty `<link/>`. - `validate_network` warns per omitted link citing `asam.net:xodr:1.4.0:ids.only_ref_defined_ids`, so the drop is never silent — mirroring the rm:arms rule that stale references are not written. Tests cover the crash repro, the omitted-link output shape, the validator warning, write determinism with a dangling arm, and the `erase_junction` variant. Closes #311
17 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.
Fixes the pre-existing
write_xodrsegfault filed as #311 — a P4 blocker: it gates p4-s4 (#319), whose junction membership editing multiplies the code paths that can leave a stale reference behind.Root cause
core/src/xodr/writer.cppdereferencednetwork.road(*road_target)->odr_idinwrite_link_elementwith no null check.RoadNetwork::erase_roaddeliberately drops only the junction connections that name the erased road (core/src/road/network.cpp:110-115) — the surviving connecting roads keep apredecessor/successorholding the now-staleRoadId. That is exactly the state #311 reproduces by erasing a junction arm. Confirmed under lldb:EXC_BAD_ACCESSat+0x18, insidestd::string.erase_junctionleaves the symmetric hole on theJunctionIdarm of the same variant.Two things worth recording, both of which the issue anticipated:
arm_face,link_armandarm_face_verticesall null-check. The crash was plain road-link serialization, merely reached via the dangling arm.edit::delete_road's closure strips connections and arms and deletes orphaned connecting roads. So this is the defence-in-depth guard the issue asked for — and it also covers a hand-edited or foreign.xodr, which reaches the writer with no closure at all.Fix
link_target_odr_idresolves aRoadLinkto its@elementIdornullptr, covering both arms of the target variant and never dereferencing.write_roadresolves both ends before opening<link>, so a stale link is dropped and a road whose links are all stale emits no empty<link/>.validate_networkwarns per omitted link, citingasam.net:xodr:1.4.0:ids.only_ref_defined_ids, so the drop is never silent — mirroring the existing rm:arms rule that stale references are not serialized.Tests
Five new tests in
core/tests/test_junction_export.cpp:WriteXodrSurvivesAnErasedArmRoadStaleRoadLinksAreOmittedNotWritten<link>ValidateNetworkWarnsAboutTheOmittedLinkWriteWithADanglingArmIsDeterministicStaleJunctionLinksAreOmittedTooerase_junctionvariantVerification
roadmaker_core_tests— 670/670 pass.-Werror) clean; no new warnings.clang-format --dry-run --Werror(CI's exact command) clean.No public kernel API change, so no bindings or example update is required.
Closes #311