From 2d1406bfe70b92e20c66e3d7ee7cea3d0d1dff05 Mon Sep 17 00:00:00 2001 From: "detail-app[bot]" <180357370+detail-app[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 02:02:42 +0000 Subject: [PATCH] fix: reject malformed JSON containers in datadog log fast path --- src/signals/datadog/log.zig | 82 ++++++++++++++-- src/signals/datadog/logs.zig | 131 +++++++++++++++++++++++++ src/signals/json_scan.zig | 181 +++++++++++++++++++++++++++++++++-- 3 files changed, 380 insertions(+), 14 deletions(-) diff --git a/src/signals/datadog/log.zig b/src/signals/datadog/log.zig index 8fd8c847..0ae3ad68 100644 --- a/src/signals/datadog/log.zig +++ b/src/signals/datadog/log.zig @@ -299,7 +299,14 @@ pub const DatadogLog = struct { try jws.writer.writeAll(raw_json); jws.endWriteRaw(); } else { - try writeAnyValue(jws, entry.value_ptr.*); + // `extra` values were materialized by the validating parse, so + // `writeAnyValue` only fails with the writer's `WriteFailed` + // here. Non-write errors are impossible in practice; coerce + // them to `WriteFailed` to keep this method within the + // `std.json.Stringify` contract (`error{WriteFailed}!void`) + // now that `writeAnyValue` propagates malformed-container + // errors for the parse-time `stringifyAnyValue` path. + writeAnyValue(jws, entry.value_ptr.*) catch return error.WriteFailed; } } // parseRaw extras: verbatim spans of the input, all value types. @@ -651,7 +658,17 @@ pub const DatadogLog = struct { return out.toOwnedSlice(); } - /// Write a zimdjson AnyValue to a JSON writer + /// Write a zimdjson AnyValue to a JSON writer. + /// + /// Malformed containers (e.g. a trailing comma that zimdjson's ondemand + /// parser tokenized structurally but didn't reject until an element is + /// materialized) surface as errors from `iterator().next()`, `key.get()`, + /// or `value.asAny()`. Those errors are propagated — NOT swallowed — so a + /// malformed container aborts serialization cleanly. Swallowing them + /// would advance the writer past a `beginObject`/`objectField` without a + /// matching value, leaving it in the `.the_beginning`/`.colon` state where + /// `endObject` is `unreachable` and would crash the process. Callers + /// route that error to the validating-path fail-open. fn writeAnyValue(jws: anytype, value: AnyValue) !void { switch (value) { .null => try jws.write(null), @@ -665,17 +682,17 @@ pub const DatadogLog = struct { .array => |arr| { try jws.beginArray(); var arr_it = arr.iterator(); - while (arr_it.next() catch null) |item| { - try writeAnyValue(jws, item.asAny() catch continue); + while (arr_it.next() catch return error.Malformed) |item| { + try writeAnyValue(jws, try item.asAny()); } try jws.endArray(); }, .object => |obj| { try jws.beginObject(); var obj_it = obj.iterator(); - while (obj_it.next() catch null) |field| { - try jws.objectField(field.key.get() catch continue); - try writeAnyValue(jws, field.value.asAny() catch continue); + while (obj_it.next() catch return error.Malformed) |field| { + try jws.objectField(try field.key.get()); + try writeAnyValue(jws, try field.value.asAny()); } try jws.endObject(); }, @@ -1104,6 +1121,57 @@ test "DatadogLog - parseRaw rejects comma separator violations (parse parity)" { try std.testing.expectEqualStrings("info", log.status.?); } +test "DatadogLog - parseRaw rejects malformed container interiors (parse parity)" { + // Regression: FieldWalker.valueEnd used to skip every non-bracket, + // non-string interior byte of a container, so bracket-balanced but + // structurally malformed unknown-field values (trailing/missing commas, + // missing values, non-string keys, malformed scalar tokens) were stored + // verbatim in `extra_spans` and never routed to the validating fallback. + // A full parser rejects each of these; parseRaw must too, so semantics + // never depend on the fast path (logs.zig evalLogRecord contract). + const allocator = std.testing.allocator; + + const bad_record_values = [_][]const u8{ + "[1,]", // trailing comma in array + "[,]", // leading comma in array + "[1 2]", // missing comma in array + "[1,,2]", // double comma / missing value in array + "{\"k\":}", // missing object value + "{1:2}", // non-string object key + "{\"a\":1,}", // trailing comma in object + "[tru]", // malformed scalar token in array + "[1e+]", // incomplete number in array + "{\"a\":1 \"b\":2}", // missing comma between object pairs + }; + for (bad_record_values) |val| { + var buf: [128]u8 = undefined; + const json = std.fmt.bufPrint( + &buf, + "{{\"message\":\"matched\",\"service\":\"s\",\"x\":{s}}}", + .{val}, + ) catch unreachable; + try std.testing.expectError(error.Malformed, DatadogLog.parseRaw(allocator, json)); + } + + // The known-field boundary: a malformed container in a KNOWN string field + // already routed to the validating path via `stringSpan` before this fix. + // Pin that the known-field path still rejects a container-shaped value. + try std.testing.expectError(error.Malformed, DatadogLog.parseRaw(allocator, + \\{"message":[1,],"service":"s"} + )); + + // Valid containers in unknown fields still parse byte-for-byte. + var ok = try DatadogLog.parseRaw(allocator, + \\{"message":"m","http":{"method":"GET","code":200},"tags":["a","b"],"empty":{},"n":42,"ok":true} + ); + defer ok.deinit(allocator); + try std.testing.expectEqualStrings("{\"method\":\"GET\",\"code\":200}", ok.extra_spans.get("http").?); + try std.testing.expectEqualStrings("[\"a\",\"b\"]", ok.extra_spans.get("tags").?); + try std.testing.expectEqualStrings("{}", ok.extra_spans.get("empty").?); + try std.testing.expectEqualStrings("42", ok.extra_spans.get("n").?); + try std.testing.expectEqualStrings("true", ok.extra_spans.get("ok").?); +} + test "DatadogLog - parse basic fields" { const allocator = std.testing.allocator; diff --git a/src/signals/datadog/logs.zig b/src/signals/datadog/logs.zig index a9351142..ba81fc8a 100644 --- a/src/signals/datadog/logs.zig +++ b/src/signals/datadog/logs.zig @@ -1007,6 +1007,137 @@ test "evalLogRecord - malformed and non-object records fail open to keep" { try std.testing.expectEqual(RecordVerdict.keep, scalar); } +test "evalLogRecord - malformed unknown-field container fails open to keep under matching policy" { + // Regression (Datadog logs / json_scan): FieldWalker.valueEnd used to + // accept bracket-balanced-but-structurally-malformed container values in + // unknown fields and store them verbatim, so a matching drop policy + // returned `.drop` and a matching mutating policy returned `.replace` — + // making verdicts depend on the fast path, contrary to the contract on + // evalLogRecord. After the fix, parseRaw rejects these records, the + // materializing `DatadogLog.parse` rejects them too, and both policy + // shapes fail open to `.keep` (the record is forwarded verbatim). + const allocator = std.testing.allocator; + + var parser: Parser = .init; + defer parser.deinit(allocator); + + var noop_bus: NoopEventBus = undefined; + noop_bus.init(std.Options.debug_io); + const bus = noop_bus.eventBus(); + + // Every input has a malformed container in an UNKNOWN field `x` whose + // brackets balance but whose interior violates JSON grammar. The buggy + // fast path accepted these and let the policy run; the corrected path + // rejects them (parseRaw -> DatadogLog.parse) and fail-opens to `.keep`. + // Each of these is verified to be rejected by the materializing fallback + // (zimdjson ondemand is lazy; DatadogLog.parse's field iteration rejects + // them), mirroring the bug report's corrected-path verdicts table. + const malformed_values = [_][]const u8{ + "[1,]", // trailing comma in array + "[,]", // leading comma in array + "{,}", // leading comma in object + "{\"k\":}", // missing object value + "{1:2}", // non-string object key + "[\"a\",,]", // missing value in array + }; + + // --- Drop policy (keep = "none"), matched on LOG_FIELD_BODY regex "matched". + { + var registry = PolicyRegistry.init(allocator, bus); + defer registry.deinit(); + var drop_policy: proto.policy.Policy = .{ + .id = try allocator.dupe(u8, "drop-matched"), + .name = try allocator.dupe(u8, "drop-matched"), + .enabled = true, + .target = .{ .log = .{ .keep = try allocator.dupe(u8, "none") } }, + }; + try drop_policy.target.?.log.match.append(allocator, .{ + .field = .{ .log_field = .LOG_FIELD_BODY }, + .match = .{ .regex = try allocator.dupe(u8, "matched") }, + }); + defer drop_policy.deinit(allocator); + try registry.updatePolicies(&.{drop_policy}, "drop", .file); + + for (malformed_values) |val| { + var buf: [128]u8 = undefined; + const record = std.fmt.bufPrint( + &buf, + "{{\"message\":\"matched\",\"service\":\"s\",\"x\":{s}}}", + .{val}, + ) catch unreachable; + var arena = std.heap.ArenaAllocator.init(allocator); + defer arena.deinit(); + try std.testing.expectEqual( + RecordVerdict.keep, + try evalLogRecord(arena.allocator(), &parser, allocator, ®istry, bus, record, null), + ); + } + + // Contrast: a well-formed matching record IS dropped (the fallback + // accepts a valid object, so the policy runs). Guards against the + // fix over-broadening into a keep-everything regression. + const good = "{\"message\":\"matched\",\"service\":\"s\",\"x\":[1,2]}"; + var good_arena = std.heap.ArenaAllocator.init(allocator); + defer good_arena.deinit(); + try std.testing.expectEqual( + RecordVerdict.drop, + try evalLogRecord(good_arena.allocator(), &parser, allocator, ®istry, bus, good, null), + ); + } + + // --- Mutating policy (keep = "all", removes `service`), matched on the body. + { + var registry = PolicyRegistry.init(allocator, bus); + defer registry.deinit(); + var transform: proto.policy.LogTransform = .{}; + var remove_attr_path: proto.policy.AttributePath = .{}; + try remove_attr_path.path.append(allocator, try allocator.dupe(u8, "service")); + try transform.remove.append(allocator, .{ + .field = .{ .log_attribute = remove_attr_path }, + }); + var mutate_policy: proto.policy.Policy = .{ + .id = try allocator.dupe(u8, "remove-service"), + .name = try allocator.dupe(u8, "remove-service"), + .enabled = true, + .target = .{ .log = .{ + .keep = try allocator.dupe(u8, "all"), + .transform = transform, + } }, + }; + try mutate_policy.target.?.log.match.append(allocator, .{ + .field = .{ .log_field = .LOG_FIELD_BODY }, + .match = .{ .regex = try allocator.dupe(u8, "matched") }, + }); + defer mutate_policy.deinit(allocator); + try registry.updatePolicies(&.{mutate_policy}, "mutate", .file); + + for (malformed_values) |val| { + var buf: [128]u8 = undefined; + const record = std.fmt.bufPrint( + &buf, + "{{\"message\":\"matched\",\"service\":\"s\",\"x\":{s}}}", + .{val}, + ) catch unreachable; + var arena = std.heap.ArenaAllocator.init(allocator); + defer arena.deinit(); + try std.testing.expectEqual( + RecordVerdict.keep, + try evalLogRecord(arena.allocator(), &parser, allocator, ®istry, bus, record, null), + ); + } + + // Contrast: a well-formed matching record IS replaced (`service` + // removed). The malformed path used to also `.replace` with the bad + // span baked in; the fix sends malformed records to `.keep` instead. + const good = "{\"message\":\"matched\",\"service\":\"s\",\"x\":[1,2]}"; + var good_arena = std.heap.ArenaAllocator.init(allocator); + defer good_arena.deinit(); + const verdict = try evalLogRecord(good_arena.allocator(), &parser, allocator, ®istry, bus, good, null); + try std.testing.expect(verdict == .replace); + try std.testing.expect(std.mem.indexOf(u8, verdict.replace, "\"service\"") == null); + } +} + test "processLogs - no policies keeps all logs in array" { const allocator = std.testing.allocator; diff --git a/src/signals/json_scan.zig b/src/signals/json_scan.zig index 1ddd2731..b6ee7241 100644 --- a/src/signals/json_scan.zig +++ b/src/signals/json_scan.zig @@ -140,30 +140,115 @@ pub const FieldWalker = struct { switch (self.raw[start]) { '"' => return self.stringEnd(start) orelse error.Malformed, '{', '[' => { - // Opener kinds as a bit-stack (0 = object, 1 = array): - // depth counts alone accept mismatched closers like `[{]}`. - // Nesting beyond 64 levels falls to the validating parser. + // Opener kinds as a bit-stack (0 = object, 1 = array): depth + // counts alone accept mismatched closers like `[{]}`. Nesting + // beyond 64 levels falls to the validating parser. + // + // A per-depth position state machine validates the container + // grammar (commas, colons, string keys, values, closers) as the + // span is scanned, instead of skipping every non-bracket, + // non-string interior byte. Structural-grammar malformations a + // full parser rejects (trailing/leading/missing commas, missing + // values, non-string object keys, malformed scalar tokens) + // error here so the validating fallback runs and semantics + // never depend on this fast path. + const Pos = enum(u3) { + start, // after opener: array -> value|`]`; object -> key|`}` + after_elem, // after a full element: comma|closer + after_comma, // after `,`: value (array) or key (object) + after_key, // after an object key string: colon (object only) + after_colon, // after an object `:`: value (object only) + }; var stack: u64 = 0; var depth: u8 = 0; + var pos = [_]Pos{.start} ** 65; var i = start; while (i < self.raw.len) : (i += 1) { const byte = self.raw[i]; switch (byte) { - // Bulk of container bytes are string keys/values: - // vault over them with the vectorized scan. - '"' => i = (self.stringEnd(i) orelse return error.Malformed) - 1, + // Inter-token whitespace, and the documented control-byte + // deviation (raw < 0x20 inside container interiors) are + // skipped without consuming a grammar slot. + ' ', 0x00...0x1f => {}, + // Strings vault over their content; whether the string is + // an object key or a value is decided by container shape + // and position below. + '"' => { + i = (self.stringEnd(i) orelse return error.Malformed) - 1; + if ((stack & 1) == 0) { // object: key or value + switch (pos[depth]) { + .start, .after_comma => pos[depth] = .after_key, + .after_colon => pos[depth] = .after_elem, + .after_elem, .after_key => return error.Malformed, + } + } else { // array: value only + switch (pos[depth]) { + .start, .after_comma => pos[depth] = .after_elem, + else => return error.Malformed, + } + } + }, '{', '[' => { if (depth == 64) return error.Malformed; + if (depth != 0) { + if ((stack & 1) == 0) { // object: value must follow `:` + if (pos[depth] != .after_colon) return error.Malformed; + } else { // array: value at start or after a comma + switch (pos[depth]) { + .start, .after_comma => {}, + else => return error.Malformed, + } + } + pos[depth] = .after_elem; + } stack = (stack << 1) | @intFromBool(byte == '['); depth += 1; + pos[depth] = .start; }, '}', ']' => { if ((stack & 1) != @intFromBool(byte == ']')) return error.Malformed; + switch (pos[depth]) { + .start, .after_elem => {}, // empty container or after a value + .after_comma, .after_key, .after_colon => return error.Malformed, + } stack >>= 1; depth -= 1; if (depth == 0) return i + 1; + pos[depth] = .after_elem; + }, + ',' => { + if (pos[depth] != .after_elem) return error.Malformed; + pos[depth] = .after_comma; + }, + ':' => { + if ((stack & 1) != 0 or pos[depth] != .after_key) return error.Malformed; + pos[depth] = .after_colon; + }, + else => { + // A scalar value: run to the next structural byte + // or whitespace and validate the literal, so `tru`, + // `1e+`, or a non-string object key inside a + // container fails open to the validating parser. + if ((stack & 1) == 0) { // object: scalars are values only + if (pos[depth] != .after_colon) return error.Malformed; + } else { // array: value at start or after a comma + switch (pos[depth]) { + .start, .after_comma => {}, + else => return error.Malformed, + } + } + const tok_start = i; + i += 1; + while (i < self.raw.len) : (i += 1) { + switch (self.raw[i]) { + ',', '}', ']', ' ', 0x00...0x1f => break, + else => {}, + } + } + if (!validValueSpan(self.raw[tok_start..i])) return error.Malformed; + pos[depth] = .after_elem; + i -= 1; }, - else => {}, } } return error.Malformed; @@ -411,6 +496,88 @@ test "FieldWalker - mismatched container closers error" { try testing.expectEqualStrings("[{\"a\":[1]},[]]", field.value); } +test "FieldWalker - container interior grammar is validated" { + // Bracket-balanced but structurally malformed container VALUES must error + // rather than be waved through to policy eval. A full validator rejects + // every one of these; the walker must too, so the validating/fail-open + // fallback runs (semantics never depend on the fast path). + const bad_values = [_][]const u8{ + "[1,]", // trailing comma in array + "[,]", // leading comma in array + "[1 2]", // missing comma in array + "[1,,2]", // double comma in array + "{\"k\":}", // missing object value + "{1:2}", // non-string object key + "{\"a\":1,}", // trailing comma in object + "{,\"a\":1}", // leading comma in object + "{\"a\":1 \"b\":2}", // missing comma between object pairs + "{\"a\":1::2}", // extra colon in object + "{\"a\" 1}", // missing colon (scalar where colon expected) + "[tru]", // malformed scalar token in array + "[1e+]", // incomplete number in array + "[1 1]", // two scalars in array (missing comma) + "[\"a\" \"b\"]", // missing comma between string values + "{\"k\":,}", // comma where value expected + "{\"k\":}}", // closer where value expected (object) + }; + for (bad_values) |val| { + var buf: [256]u8 = undefined; + const json = std.fmt.bufPrint(&buf, "{{\"x\":{s}}}", .{val}) catch unreachable; + var walker = try FieldWalker.init(json); + try testing.expectError(error.Malformed, walker.nextField()); + } +} + +test "FieldWalker - valid containers still parse with all value types" { + // Positive coverage: the stricter grammar must still accept every shape + // a full parser accepts, including empty containers, every scalar, mixed + // nesting, and inter-token whitespace (incl. documented control bytes). + const ok_values = [_]struct { val: []const u8, want: []const u8 }{ + .{ .val = "[1]", .want = "[1]" }, + .{ .val = "[1,2,3]", .want = "[1,2,3]" }, + .{ .val = "[]", .want = "[]" }, + .{ .val = "{}", .want = "{}" }, + .{ .val = "{\"k\":\"v\"}", .want = "{\"k\":\"v\"}" }, + .{ .val = "{\"a\":1,\"b\":2}", .want = "{\"a\":1,\"b\":2}" }, + .{ .val = "[1,2,null,true,false,\"s\",{},[]]", .want = "[1,2,null,true,false,\"s\",{},[]]" }, + .{ .val = "-0.5", .want = "-0.5" }, + .{ .val = "123.456e-7", .want = "123.456e-7" }, + .{ .val = "[{\"a\":[1]},[],{\"k\":\"v\"}]", .want = "[{\"a\":[1]},[],{\"k\":\"v\"}]" }, + }; + for (ok_values) |t| { + var buf: [256]u8 = undefined; + const json = std.fmt.bufPrint(&buf, "{{\"x\":{s}}}", .{t.val}) catch unreachable; + var walker = try FieldWalker.init(json); + const field = (try walker.nextField()).?; + try testing.expectEqualStrings("x", field.key); + try testing.expectEqualStrings(t.want, field.value); + try testing.expectEqual(@as(?FieldWalker.RawField, null), try walker.nextField()); + } + + // Inter-token whitespace and the documented raw control-byte deviation + // (< 0x20 inside container interiors) must still parse. + var ws = try FieldWalker.init("{\"a\":[1,\n\t 2],\"d\":\"\x7f\"}"); + _ = (try ws.nextField()).?; + const d = (try ws.nextField()).?; + try testing.expectEqualStrings("\"\x7f\"", d.value); + + // A raw control byte sitting where whitespace is valid (between tokens) + // is accepted per the documented deviation; the surrounding grammar is + // still validated, so malformations around it are rejected. + var ctrl_ok = try FieldWalker.init("{\"x\":[1,\x0b2]}"); + const cf = (try ctrl_ok.nextField()).?; + try testing.expectEqualStrings("[1,\x0b2]", cf.value); + + // The control byte does NOT mask a trailing comma. + var ctrl_bad = try FieldWalker.init("{\"x\":[1,\x0b,]}"); + try testing.expectError(error.Malformed, ctrl_bad.nextField()); + + // 64-deep nesting still parses once the interior is grammar-valid. + const deep_ok = "{\"x\":" ++ "[" ** 64 ++ "]" ** 64 ++ "}"; + var deep = try FieldWalker.init(deep_ok); + _ = (try deep.nextField()).?; +} + test "FieldWalker - nesting beyond 64 levels falls to the validating parser" { const deep_bad = "{\"x\":" ++ "[" ** 65 ++ "]" ** 65 ++ "}"; var walker = try FieldWalker.init(deep_bad);