Skip to content

[Algorithm] Fix inverted trailer check condition in ForwardParser#15547

Open
niteshg97 wants to merge 3 commits into
AliceO2Group:devfrom
niteshg97:fix/forward-parser-trailer-check
Open

[Algorithm] Fix inverted trailer check condition in ForwardParser#15547
niteshg97 wants to merge 3 commits into
AliceO2Group:devfrom
niteshg97:fix/forward-parser-trailer-check

Conversation

@niteshg97

Copy link
Copy Markdown

Problem

ForwardParser::parse() in Algorithm/include/Algorithm/Parser.h contains
an inverted condition for trailer handling:

// Before (buggy)
if (tailOffset > 0) {        // tailOffset>0 means a TrailerType IS defined
    entry.trailer = nullptr;  // silently nulls it out — callback never called
} else {                      // else runs when there is NO trailer (void)
    ...CheckTrailer(entry, checkTrailer)...
}

tailOffset = typesize<TrailerType>::size, so it is > 0 precisely when
a real TrailerType is specified. The condition is completely inverted:

  • When a TrailerType exists (tailOffset > 0), entry.trailer is set
    to nullptr and the user-supplied checkTrailer callback is never invoked.
  • When TrailerType is void (tailOffset == 0), the dead else-branch runs
    a no-op CheckTrailer specialisation that always returns true.

Consequence: frames with corrupt or invalid trailers are silently accepted
without any validation. No crash occurs, making this bug invisible without
dedicated tests.

Why existing tests did not catch this

test_forwardparser_header_and_trailer provides a checkTrailer lambda but
never asserts it was actually called — it only checks payload bytes. The tests
passed even though the callback was completely bypassed.

Fix

Invert the condition (tailOffset > 0tailOffset == 0):

// After (fixed)
if (tailOffset == 0) {
    // no trailer type defined, nothing to extract
    entry.trailer = nullptr;
} else {
    auto trailerStart = buffer + position + frameSize - tailOffset;
    entry.trailer = reinterpret_cast<const TrailerType*>(trailerStart);
    if (!CheckTrailer(entry, checkTrailer)) {
        break;
    }
}

Tests

  • Updated test_forwardparser_header_and_trailer:

    • Counts checkTrailer invocations and asserts it is called once per frame.
    • Asserts frames[i].trailer != nullptr and correct flags fields are extracted.
  • Added test_forwardparser_trailer_check_rejection:

    • Provides a checkTrailer that rejects the second frame.
    • Asserts result == -1 (format error) and that insert is never called.

All existing tests (void-trailer, no-frames, format-error, reverse-parser)
are unaffected — they use TrailerType = void (tailOffset == 0) and are
handled by the unchanged if-branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant