fix: guard optional output/input dereferences in FarDetectorLinearTracking and ActsToTracks - #2779
Open
wdconinc wants to merge 1 commit into
Open
fix: guard optional output/input dereferences in FarDetectorLinearTracking and ActsToTracks#2779wdconinc wants to merge 1 commit into
wdconinc wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash class in EICrecon tracking by guarding dereferences of optional inputs/outputs in the FarDetectorLinearTracking and ActsToTracks algorithms, addressing issue #2761 (unconditional dereference of truth-association data).
Changes:
- Adds null/emptiness guards for optional truth-association inputs/outputs in
ActsToTracksbefore iterating/creating association collections. - Makes truth association handling in
FarDetectorLinearTrackingconditional, including switching association particle storage tostd::optional<edm4hep::MCParticle>. - Updates
FarDetectorLinearTrackinginterfaces to pass link navigation / association collections as pointers and threads ado_assocflag intocheckHitCombination.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/algorithms/tracking/ActsToTracks.cc | Adds flags to gate association processing and output collection creation based on optional inputs/outputs. |
| src/algorithms/fardetectors/FarDetectorLinearTracking.h | Updates method signatures to use optional particle association storage and pointer-based association inputs, adding a do_assoc control flag. |
| src/algorithms/fardetectors/FarDetectorLinearTracking.cc | Implements guarded truth association logic, optional particle handling, and avoids unsafe dereferences of optional collections/navigation. |
Comments suppressed due to low confidence (1)
src/algorithms/fardetectors/FarDetectorLinearTracking.cc:109
ConvertClusterswill still try to resolve associated particles vialink_nav/assocHitseven whendo_associs false (e.g., when association outputs are not requested), which can be significant extra work. Passing nullptrs whendo_associs false avoids the extra lookup/scanning cost.
ConvertClusters(*layerHits, link_nav ? &(*link_nav) : nullptr, assocHits, convertedHits,
assocParts);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+69
| const bool do_track_links = tracks_links != nullptr; | ||
| const bool do_track_assoc = tracks_assoc != nullptr; | ||
| const bool do_raw_assoc = raw_hit_assocs != nullptr && !raw_hit_assocs->empty(); |
Comment on lines
236
to
+239
| // Determine track associations if hit associations provided | ||
| // FIXME: not able to check whether optional inputs were provided | ||
| //if (raw_hit_assocs->has_value()) { | ||
| for (const auto& hit : meas2D.getHits()) { | ||
| auto raw_hit = hit.getRawHit(); | ||
| for (const auto raw_hit_assoc : *raw_hit_assocs) { | ||
| if (raw_hit_assoc.getRawHit() == raw_hit) { | ||
| auto sim_hit = raw_hit_assoc.getSimHit(); | ||
| auto mc_particle = sim_hit.getParticle(); | ||
| mcparticle_weight_by_hit_count[mc_particle]++; | ||
| if (do_raw_assoc) { |
Comment on lines
85
to
89
| // Build fast lookup once per event using podio::LinkNavigator | ||
| std::optional<podio::LinkNavigator<edm4eic::MCRecoTrackerHitLinkCollection>> link_nav; | ||
| if (do_assoc) { | ||
| if (hitLinks != nullptr && !hitLinks->empty()) { | ||
| link_nav.emplace(*hitLinks); | ||
| } |
github-actions
Bot
added
topic: tracking
wdconinc
changed the title
Contributor
Capybara summary for PR 2779
Last updated 2026-07-27T19:39-04:00 266c9e8 |
…cking and ActsToTracks FarDetectorLinearTracking (closes #2761): - link_nav is now an optional<LinkNavigator> that is only populated when hitLinks is non-null and non-empty; ConvertClusters receives it as a nullable pointer rather than a reference, eliminating the unconditional dereference. - assoc_hits / assoc_parts changed to nullable pointer / optional-element vector so ConvertClusters can be called even when no association collections are provided. - checkHitCombination gated by do_assoc flag; trackLinks and assocTracks outputs checked for nullptr before writing. ActsToTracks (closes #2761, comment): - tracks_links, tracks_assoc, and raw_hit_assocs dereferenced only after checking for nullptr / empty; association block skipped entirely when no output collection is provided. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
wdconinc
force-pushed
the
fix/2761-null-deref
branch
from
July 27, 2026 21:43
6c510ac to
266c9e8
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
src/algorithms/tracking/ActsToTracks.cc:239
- The comment block still says it is "not able to check whether optional inputs were provided", but the new
do_raw_assocflag does check whether the optional input collection is present/non-empty. Leaving the stale FIXME (and the commented-outhas_value()code) is misleading for future maintenance.
// Determine track associations if hit associations provided
// FIXME: not able to check whether optional inputs were provided
//if (raw_hit_assocs->has_value()) {
if (do_raw_assoc) {
src/algorithms/tracking/ActsToTracks.cc:260
- Same as above: this FIXME/commented-out
has_value()check is now outdated. The code is already checking output pointers and whether any associations were accumulated; keep the comment consistent with the actual gating logic.
// Store track associations if hit associations provided
// FIXME: not able to check whether optional inputs were provided
//if (raw_hit_assocs->has_value()) {
if ((do_track_assoc || do_track_links) && !mcparticle_weight_by_hit_count.empty()) {
src/algorithms/fardetectors/FarDetectorLinearTracking.cc:110
ConvertClusterswill still scan/link truth associations even whendo_associs false (e.g. when association outputs are disabled). That extra work is unnecessary and could be expensive whenassocHitsis large. Passnullptrforlink_nav/assocHitswhendo_associs false so the conversion stays purely geometric in that mode.
}
ConvertClusters(*layerHits, link_nav ? &(*link_nav) : nullptr, assocHits, convertedHits,
assocParts);
}
Comment on lines
+82
to
83
| debug("Truth association inputs or outputs are missing. No truth associations " | ||
| "will be performed."); |
Comment on lines
212
to
214
| // Add Measurement2D relations and count occurrence of particles contributing to the track | ||
| std::unordered_map<edm4hep::MCParticle, int> particleCount; | ||
| for (std::size_t layer = 0; layer < layerHitIndex.size(); layer++) { |
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.
Guards optional output/input dereferences in
FarDetectorLinearTrackingandActsToTracks(closes #2761).FarDetectorLinearTrackinglink_nav(std::optional<podio::LinkNavigator>) was dereferenced unconditionally — now only populated and passed when the link collection is non-null/non-empty.assocHits(optional input) andassocTracks/trackLinks(optional outputs) now guarded with null checks.vector<MCParticle>tovector<optional<MCParticle>>.checkHitCombinationreceives ado_assocflag.ActsToTrackstracks_links->create(),tracks_assoc->create(), and*raw_hit_assocswere called unconditionally — now gated ondo_track_links,do_track_assoc,do_raw_assocflags.What is the urgency of this PR?
What kind of change does this PR introduce?
Please check if any of the following apply