Repository navigation
Add fuzz tests for the FFI buffer boundary - #19
Merged
Merged
Conversation
wberry-godaddy
marked this pull request as ready for review
July 27, 2026 07:21
This was referenced Jul 27, 2026
Targets the Cobhan FFI boundary directly: buffers and length headers that in production arrive from an untrusted foreign caller (C, Java, Python, Node, .NET...). Includes a raw-header fuzzer that constructs buffers by hand to simulate a caller whose header doesn't match its payload. Run with -race: some of the crashes these are designed to catch only manifest under checkptr instrumentation. go test -race -fuzz=FuzzStringRoundTrip -fuzztime=60s FuzzBufferToJson already fails on its own seed corpus (a JSON array seed), no fuzzing required -- see PR description for details.
The original seeds all used an empty payload, which meant every one of them hit the same zero-length checkptr crash before any other header value could be exercised. Use non-empty payloads for the header values meant to probe other code paths (temp-file negation overflow, header overstating payload length), and add an explicit invariant check so a header/payload length mismatch is caught as a normal test failure even in a non-race build, not just incidentally via checkptr.
Found via mutation on FuzzBufferToJsonStruct before explicit seeds covering this case were added; same root cause as the checkptr crash covered by the empty-string/empty-byte-slice seeds elsewhere.
Required for the checkptr-class crashes the new fuzz tests catch (zero-length buffer pointer arithmetic, header/payload length mismatches) to actually be enforced in CI -- without -race these are silent in a normal build.
A length header of exactly math.MinInt32 negates to itself under two's-complement arithmetic, so the code still treated it as a (deeply negative) temp-file-name length and handed it to C.GoStringN, which attempted an ~18 exabyte allocation and killed the process with a fatal (unrecoverable) OOM error. Reject a still-negative result after negation instead -- there is no valid filename-length encoding for it. Fixes FuzzRawBufferHeader/seed#3 (header=math.MinInt32).
loadedJson.(map[string]interface{}) panicked whenever the top-level
JSON value wasn't an object (arrays, strings, numbers, bools, null
are all valid JSON but not maps). Use the ok-form assertion and
return ERR_JSON_DECODE_FAILED instead of crashing the caller's
goroutine.
Also adds a corpus regression for FuzzBufferToJson found via mutation:
an empty []byte input, which is the same zero-length checkptr crash
covered elsewhere (not yet fixed).
) bufferPtrToDataPtr computed a data pointer via raw uintptr(bufferPtr) + BUFFER_HEADER_SIZE arithmetic unconditionally, even for a zero-length (header-only) buffer. That pointer sits exactly one byte past the end of the buffer's real allocation and is never dereferenced for a zero-length value, but forming it via manual pointer arithmetic (rather than Go's slice-indexing machinery, which is allowed to compute one-past-the-end addresses) is exactly what checkptr flags as invalid -- crashing the whole process with 'fatal error: checkptr: pointer arithmetic result points to invalid allocation' under go test -race. Short-circuit on length == 0 in bufferPtrToString, bufferPtrToBytes, and BytesToBuffer instead of computing-then-discarding the invalid pointer. No behavior change for non-empty buffers. Fixes FuzzStringRoundTrip, FuzzBytesRoundTrip, FuzzBufferToJson, FuzzBufferToJsonStruct, and FuzzRawBufferHeader/seed#0-3, all of which hit this same root cause. This looks like the same bug already fixed independently in godaddy#18; will reconcile before merging this PR.
bufferPtrToBytes/bufferPtrToString trust the header's claimed length completely -- there is no independent capacity available from a bare pointer to validate it against, so a header that overstates the real buffer size (directly, or via the negative temp-file-name-length encoding) will read past the end of the real allocation with no error in a normal build, and hits checkptr's straddling-allocation check under -race when the backing memory happens to be Go-managed. This is inherent to a length-prefixed wire format that trusts its own length field; fixing it would require changing every Cobhan binding's ABI to also carry a capacity, which is out of scope here. Documented with a comment on bufferPtrToBytes. FuzzRawBufferHeader now explicitly skips generating this specific (unfixable) scenario via t.Skip, covering both the direct and temp-file-encoded overstatement cases, while still fully exercising math.MinInt32 (the negation-overflow case, which *is* fixed and must not be skipped). go test -race ./... is now fully green.
wberry-godaddy
force-pushed
the
add-fuzz-tests
branch
from
July 27, 2026 07:42
b4030ce to
272d740
Compare
jpage-godaddy
approved these changes
Jul 27, 2026
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.
Summary
Adds Go native fuzz tests (
go test -fuzz, https://go.dev/doc/security/fuzz/) targeting the Cobhan FFI boundary: the length-prefixed buffers and headers this library hands back and forth across a foreign-language call boundary (C, Java, Python, Node, .NET, ...) and therefore cannot assume are well-formed.This was built test-driven: fuzz tests added first (red), each distinct bug documented below, then fixed one at a time.
go test -race ./...is green as of the latest commit.Also adds
-racetoscripts/test.sh(required for the checkptr-class crashes below to actually be caught). (The.github/workflows/test.ymlGo-version-matrix change that was previously bundled into this PR has been split out into #21, to keep this PR scoped purely to testing and the fixes it drove.)Targets added
FuzzStringRoundTripAllocateStringBuffer->BufferToStringSafeFuzzBytesRoundTripAllocateBytesBuffer->BufferToBytesSafeFuzzBufferToJsonBufferToJsonSafeFuzzBufferToJsonStructBufferToJsonStructSafeFuzzRawBufferHeaderint32length header + arbitrary payload) ->BufferToBytes/BufferToStringdirectly on the raw pointer, bypassingAllocateBufferentirely -- simulates a caller whose header doesn't match its actual payloadRun any target with, e.g.:
-racematters -- several of the bugs below only manifest undercheckptrinstrumentation (enabled by-race, orGODEBUG=checkptr=1); a plaingo test -fuzz=...build will not find them.Each target has also been run for 45-60s of mutation-based fuzzing under
-racewith no further crashes (millions of executions each).Bugs found and fixed
1.
int32negation overflow intempToBytes-> unrecoverable OOM crash (fixed in b72f35c)cobhan.go(tempToBytes): for a length header of exactlymath.MinInt32, two's-complement negation overflows back tomath.MinInt32(still negative). The code proceeded to treat this as a temp-file-name length and calledC.GoStringNwith it, attempting an ~18 exabyte allocation:This is a
fatal error, not apanic--recover()cannot catch it, and it reproduced in any build mode, not just-race. Highest severity of the bunch: a single corrupted or malicious 4-byte length header killed the entire host process outright.Fix: reject a still-negative result after negation instead of proceeding.
2.
BufferToJsonpanics on valid JSON that isn't an object (fixed in cd4c1dc)cobhan.go(BufferToJson) didloadedJson.(map[string]interface{})unconditionally. Any valid JSON whose top-level value isn't an object ([1,2,3],"a string",42,null,true) panicked instead of returningERR_JSON_DECODE_FAILED.Fix: use the
ok-form type assertion and returnERR_JSON_DECODE_FAILEDwhen the top-level value isn't a map.3. Zero-length buffer ->
checkptrfatal crash (fixed in 414972e; same root cause as #18)bufferPtrToDataPtrcomputed a data pointer via rawuintptr(bufferPtr) + BUFFER_HEADER_SIZEunconditionally, even for a zero-length (header-only) buffer, landing exactly one byte past the end of the allocation. Never dereferenced for length 0, but forming it via manual pointer arithmetic is exactly whatcheckptrflags:Reached independently from
FuzzStringRoundTrip,FuzzBytesRoundTrip,FuzzBufferToJson,FuzzBufferToJsonStruct, andFuzzRawBufferHeader-- five different entry points into the same root cause.Fix: short-circuit on
length == 0inbufferPtrToString,bufferPtrToBytes, andBytesToBufferinstead of forming the invalid pointer. This is the same bug already fixed independently in #18 -- closed that PR in favor of this one, since the fuzz tests here catch it from five entry points instead of two, and this PR also adds-raceto CI so it stays caught going forward.4. Header claiming more data than the buffer holds -> silent heap over-read (documented as an accepted limitation, not fixed -- see 322aa5b)
bufferPtrToBytes/bufferPtrToStringtrust the header's claimed length completely, with no way to validate it against the true size of the backing allocation -- these functions only ever see a bare pointer, and in real usage that pointer often refers to foreign (non-Go) memory that Go's runtime has no visibility into at all.In a normal (non-
-race) build, a header claiming more data than was actually written succeeds silently and returns bytes read past the end of the real buffer (confirmed: 4 real payload bytes written, 64 requested and returned, with the extra 60 bytes being whatever adjacent memory happened to contain). Also reachable via the negative/temp-file-name-length encoding, not just a direct oversized header.Disposition: I initially treated this as bug #4 to fix, but there is no code-level fix available at this API surface --
BufferToBytes/BufferToStringtake only a pointer, with no independent capacity to check the header against. Cobhan's own write path (BytesToBuffer) always keeps the header accurate; a mismatch can only happen from a corrupted buffer or a bug in another language's Cobhan binding, i.e. it's the same class of "trust the sender" risk inherent to any length-prefixed wire format. A real fix would mean changing these functions' signatures (and therefore the C ABI every Cobhan binding depends on) to also carry a capacity -- out of scope here.Documented with a comment on
bufferPtrToBytes.FuzzRawBufferHeaderexplicitly skips generating this scenario (viat.Skip) rather than asserting on something unfixable, while still fully exercisingmath.MinInt32(bug #1's regression case, which is fixed).CI changes
scripts/test.shnow runs with-race(needed for bugs Set up boilerplate GoDaddy docs #1 and Go Parity #3 above to actually be caught in CI going forward)..github/workflows/test.ymlGo version matrix update (stable/oldstableinstead of a hardcoded1.24) has been moved to CI: test stable/oldstable Go matrix, bump actions #21, unrelated to fuzz testing.Testing