fix: reject malformed JSON containers in datadog log fast path - #299
Open
detail-app[bot] wants to merge 1 commit into
Open
detail-app[bot] wants to merge 1 commit into
detail-app[bot] wants to merge 1 commit into
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a localized Datadog JSON parsing bug fix that makes malformed containers fail open and preserves valid-record behavior. The added scanner logic and serialization safeguards are covered by targeted regression tests, with no schema, security, billing, or deployment changes. You can add or adjust custom eligibility rules. Learn more. |
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.
Detail bug report: View on Detail
Bug
The per-record Datadog-log fast path (
FieldWalker.valueEndinsrc/signals/json_scan.zig, used byDatadogLog.parseRaw) matched container brackets and tracked string spans but skipped every other interior byte (else => {}), andvalidValueSpanaccepted any{/[-leading span unconditionally. Bracket-balanced-but-structurally-malformed container values in unknown fields (e.g.[1,],{"k":},{1:2}) were stored verbatim and never routed to the validating fallback — contradicting the scanner's stated strictness guarantee and theevalLogRecordcontract that "semantics never depend on the fast path."With a matching drop or mutating policy, the verdict diverged from the corrected (fail-open) path: a drop policy returned
.drop(record discarded) and a mutating policy returned.replace(record re-serialized with the malformed span baked in), instead of.keep.Fix
src/signals/json_scan.zig: replaced the container-interior byte skip invalueEndwith a per-depth position state machine that validates the JSON container grammar (commas, colons, string keys vs. values, closers, and scalar tokens viavalidValueSpan) as the span is scanned. Malformed containers now errorMalformedand route to the validating/fail-open fallback. Whitespace and the documented raw control-byte (< 0x20) deviation are unchanged.src/signals/datadog/log.zig: companion to the above —writeAnyValuepreviously swallowed iterator/materialization errors, leaving the JSON writer in the.the_beginning/.colonstate whereendObject/endArrayareunreachableand would crash. Since the scanner fix now routes malformed records to this fallback, those errors are propagated so a malformed container aborts serialization cleanly andevalLogRecordfail-opens to.keeprather than panicking. ThejsonStringifycall site coerces the (unreachable for valid data) non-WriteFailederrors toWriteFailedto preserve thestd.json.Stringifycontract.Testing
parseRawrejection parity; and anevalLogRecordintegration test asserting that matching drop and matching mutating policies both fail open to.keepfor malformed unknown-field containers, with well-formed contrasts confirming the policy still runs on valid records.zig build,zig build -Doptimize=ReleaseSafe,zig fmt --check,ziglint,zig build test) all pass — 523/524 tests (1 skipped, unchanged), up 4 from baseline, no regressions.panic: reached unreachable codeinStringify.endObjectwhen thewriteAnyValuecompanion fix is reverted — confirming both fixes are load-bearing.evalLogRecordmatrix benchmark builds and its verdict sanity checks pass. A temporary probe confirmedwriteAnyValuenow returns a clean error on malformed containers instead of panicking; it was reverted before commit. Perf cost from the added grammar work is ~2-4% on container-heavy records and flat/noise elsewhere, well under the documented 10-15% budget.exec.zig'sevalJsonRecorddelegates 1:1 toevalLogRecord.Fixes ENG-700
Automatic Fixes PRs can be configured here.