Repository navigation
fix(logger): validate ring-buffer sample bounds before decoding - #1972
vikash2006-dot wants to merge 1 commit into
Conversation
kmesh-bot
added
kind/bug
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @vikash2006-dot! It looks like this is your first PR to kmesh-net/kmesh 🎉 |
pkg/logger.decodeRecord previously performed unchecked slice operations on raw ring-buffer event samples. If a sample was shorter than 4 bytes, advertised a zero message length, or specified a message length exceeding the sample buffer, decodeRecord panicked with a slice bounds out of range error. Additionally, handleLogEvents did not skip further processing when decodeRecord failed, causing a nil pointer dereference when accessing le.Msg and killing the daemon log reader goroutine. This commit: - Validates the sample has at least 4 bytes to read the message length prefix. - Validates that the message length is at least 1 byte (including '\0'). - Uses 64-bit comparisons to prevent integer overflow when verifying that the advertised message fits within the sample slice. - Adds a continue guard on decode failure in handleLogEvents so later events can still be read. - Adds comprehensive table-driven unit tests for decodeRecord covering empty, short, zero-length, oversized, max uint32, and valid samples. Signed-off-by: Vikash Kumar <vk2542883@gmail.com>
9ff2d47 to
9b7280f
Compare
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
What type of PR is this?
/kind bug
What this PR does / why we need it:
pkg/logger.decodeRecordpreviously performed unchecked slice operations on raw ring-buffer event samples (km_log_event). A truncated sample with fewer than 4 bytes, a sample specifying a zero message length, or a sample advertising a length exceeding the buffer bounds causeddecodeRecordto panic with a slice bounds out of range error.Additionally,
handleLogEventsdid not skip processing whendecodeRecordreturned an error, immediately falling through to dereferencele.Msgon a nillepointer, crashing the daemon's log reader goroutine.This PR fixes these issues by:
recordLenSize) before reading the length prefix.recordNullSizefor the null terminator), rejecting zero-length payloads.uint64(len(data)) < uint64(recordLenSize)+uint64(lenOfMsg)) to prevent 32-bit integer overflows when checking that the advertised payload fits within the sample.continueguard on decode failure inhandleLogEventsso malformed events are logged and skipped, allowing the log reader to continue processing subsequent events.pkg/logger/logger_test.gocovering empty slices, short samples (<4 bytes), zero message length, oversized lengths,math.MaxUint32boundary handling, empty messages (\x00), valid messages, and padded ring-buffer records.Which issue(s) this PR fixes:
Fixes #1777
Special notes for your reviewer:
Please pay particular attention to the bounds checks in
decodeRecord()and the error handling inhandleLogEvents(). The added tests cover truncated samples, zero-length messages, oversized lengths, integer boundary cases, and valid records. Feedback on any edge cases or potential regressions is welcome.Does this PR introduce a user-facing change?: