[PROF-11988] Add native go fuzzing for Parse #10
Conversation
…ing from goroutine" line parsing.
edznux-dd
changed the title
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| with: | ||
| go-version: 1.16 | ||
| - name: Set up Go | ||
| uses: actions/setup-go@v2 |
There was a problem hiding this comment.
🟠 Code Vulnerability
Workflow depends on a GitHub actions pinned by tag instead of a hash. (...read more)
Pin GitHub Actions by commit hash to ensure supply chain security.
Using a branch (@main) or tag (@v1) allows for implicit updates, which can introduce unexpected or malicious changes. Instead, always pin actions to a full length commit SHA. You can find the commit SHA for the latest tag from the action’s repository and ensure frequent updates via auto-updaters such as dependabot. Include a comment with the corresponding full-length SemVer tag for clarity:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
nsrip-dd
left a comment
nsrip-dd
left a comment
There was a problem hiding this comment.
LGTM. Left a small suggestion. Thanks for spotting this, and for the fix!
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
[email protected] cancelled this merge request build |
dd-devflow
bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
|
/remove |
|
View all feedbacks in Devflow UI.
|
dd-devflow
bot
added
mergequeue-status: removed
and removed
mergequeue-status: in_progress
labels
Description
This PR adds a fuzz test for the
Parsefunction.It discovered a bug in the current implementation, which I've tentatively added as a second commit.
Running the same fuzzer for multiple minutes didn't caused any issues. (It's not a very long time for a fuzzer, I'll let it run for longer, but it's also not a very large program)
Currently this fuzz test is not run automatically, but we are working on CI integration that will be able to run these in the internal fuzzing infrastructure.
You can run the fuzz test locally by running:
go test -fuzz=FuzzParse -run=FuzzParseBug found
A crafted go stack trace containing the following bytes :
goroutine 0 [0]:\n0()\n\t:0\n[originating from goroutine "would cause a panic.Root cause analysis of the bug
By using a line that starts with the
originatingFromPrefix([]byte("[originating from goroutine ")), but that does not contain any goroutine number afterward (or, for that matter, anything at all), the program was attempting the read the length of the line. Without anything after it, like the closing of the bracket](string size, - 1 "bracket", -1 of "slice index start at 0 offset" so-2), this causes a read out of bound in the line slice. Causing a panic like so:panic: runtime error: slice bounds out of range [28:26]28 being equal to the length of the string
[originating from goroutine, if the lines only contains that, it'll be len(line) == 28, 28-2 == 26.Proposed bugfix
The proposed bugfix makes sure that the string contains at least the length of the prefix otherwise return an error. I chose to use
abortGoroutineif it's not the case, as it looks like other "bad" condition are also doing.Returning a 0 value or "continuing" the for loop/goto do not seem to be in line with the other error handling.
Impact
This bugs come from searching around dd-trace-go, and especially this: https://github.com/DataDog/dd-trace-go/blob/f92a95e4313dda76ac10592731652e282e9fd66b/profiler/profile.go#L434-L445
Since it's guarded via a
recovercall, the impact here should be minimal.After an internal repository search, there's another application that uses this but, as far as I understand, only in tests.
And, In The Wild ™️, looks like there's around 86 files over github that uses this, and as far as I can tell, none are doing
recoveron it (but most of them seem to not run on user facing stacktrace)Note: I pushed these into a single PR, to both show the additionnal fuzz and its fix so CI can be green. But happy to split and push for a fix in another PR.