Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fuzz test Prometheus #667

Open
fabxc opened this Issue May 2, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@fabxc
Copy link
Member

fabxc commented May 2, 2015

Just found this new tool for Go: https://github.com/dvyukov/go-fuzz

Given that it found quite a few bugs in the standard library, we will certainly find some stuff in Prometheus. Especially as we are now using two text/template style parsers ourselves that might suffer from very similar bugs as the ones found in the stdlib.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Jul 15, 2015

Arguably, this is more a client_golang issue (or the upcoming expfmt), but let's leave it here for now.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Jul 15, 2015

The query language could use a run as well ;)

On Wed, Jul 15, 2015 at 1:35 PM Björn Rabenstein notifications@github.com
wrote:

Arguably, this is more a client_golang issue (or the upcoming expfmt),
but let's leave it here for now.


Reply to this email directly or view it on GitHub
#667 (comment)
.

@msiebuhr

This comment has been minimized.

Copy link
Contributor

msiebuhr commented Jul 28, 2015

Played a bit with the go-fuzz, first implementing promql/fuzz.go, by testing all the Parser*()-functions:

package promql
// +build gofuzz

const (
    fuzz_interesting = 1
    fuzz_meh         = 0
    fuzz_discard     = -1
)

// Fuzz the promql parser
func Fuzz(input []byte) int {
    str := string(input)

    // Parse as metric
    _, err := ParseMetric(str)
    if err == nil {
        return fuzz_interesting
    }

    // What about a selector
    _, err = ParseMetricSelector(str)
    if err == nil {
        return fuzz_interesting
    }

    _, err = ParseExpr(str)
    if err == nil {
        return fuzz_interesting
    }

    _, err = ParseStmts(str)
    if err == nil {
        return fuzz_interesting
    }

    return fuzz_discard
}

I then gave it a small input corpus in ./corpus/with individual files containing name{} 42, a{b="c"} 1.0 456, metric_name and avg(metric_name{}[5m]) by (key1, key2). (I'm not certain what the parser(s) are for, so I conjured some simple inputs from memory...)

Then I ran godep restore to set the current $GOPATH up with all the right versions of relevant packages. Finally, go-fuzz-build github.com/prometheus/prometheus/promql instruments the code and go-fuzz -bin=./promql-fuzz.zip -workdir=fuzz-workdir to test it out. It crashes a lot in the beginning, but restarting it a few times seem to improve things dramatically.

I got a few hundred crashing examples from running it in half an hour or so, but it looks like the stack traces are all over the place, so I'm a bit hesitant to begin opening bug-reports left and right. Also, this doesn't keep track of which parser crashes, making triage more difficult.

Right now I'm trying to do quick runs of the fuzzer with one function at a time and see what I get.

@msiebuhr

This comment has been minimized.

Copy link
Contributor

msiebuhr commented Jul 28, 2015

Hit in ParseMetricSelector(); it crashes on inputs , G.٩ and .٩٩٩٩٩٩٩٩٩٩٩٩٩٩٩٩٩...

@msiebuhr

This comment has been minimized.

Copy link
Contributor

msiebuhr commented Jul 28, 2015

Reported the first cases as separate bugs. One noteworthy find was that no crashes was found in ParseMetric()...

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Jul 28, 2015

@msiebuhr This is brilliant, thanks so much! This is exactly what we were hoping for (well, uh, not the crashes, they are bad, but that uncovering them works so well like this). @fabxc is on vacation this week and back on Monday, so it might take us a bit to get to them all, but we'll definitely address all of these soon!

Fuzzing results are scary...

@msiebuhr

This comment has been minimized.

Copy link
Contributor

msiebuhr commented Jul 29, 2015

I'd be happy to make a pull-request w. some tidying up, documentation &c, if you're interested.

Also, go-fuzz mentions that "thousand[s] of inputs is fine", while I started out with the five above; I have a hunch that more variety would be nice.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Jul 29, 2015

Fantastic!
This is basically what I expected would happen as parser and lexer are
tested against valid input and common mistakes or typos - not against the
whole variety of possible Unicode strings. I hope we can isolate a few
common root causes to fix the majority.

Definitely great work. A pull request would, of course, be awesome.

On Wed, Jul 29, 2015, 8:20 AM Morten Siebuhr notifications@github.com
wrote:

I'd be happy to make a pull-request w. some tidying up, documentation &c,
if you're interested.

Also, go-fuzz mentions that "thousand[s] of inputs is fine", while I
started out with the five above; I have a hunch that more variety would be
nice.


Reply to this email directly or view it on GitHub
#667 (comment)
.

@msiebuhr

This comment has been minimized.

Copy link
Contributor

msiebuhr commented Jul 29, 2015

I've put my work in a branch, https://github.com/msiebuhr/prometheus/tree/fuzz/promql, with a fuzzer-function for each parser + small corpuses for ParseMetric and ParseExpr:

go-fuzz-build -func FuzzParseExpr -o FuzzParseExpr.zip github.com/prometheus/prometheus/promql
go-fuzz -bin FuzzParseExpr.zip -workdir fuzz-data/ParseExpr

From my (limited) experience, the fuzzer really improves with larger corpuses. So far I've mostly copy-pasted from the test-suites. I thought about just writing out the files directly from the test-suite, but I'm getting a bit too tired to hammer that out.

I also poked a bit at setting up the Makefile, but I screwed something up somewhere and thought it would be better to polish the working parts a bit more and get it out.

@msiebuhr

This comment has been minimized.

Copy link
Contributor

msiebuhr commented Jul 29, 2015

I've created an in-progress pullrequest over at #945.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jul 14, 2017

I believe we still need to make this vaguely automatic.

simonpasquier pushed a commit to simonpasquier/prometheus that referenced this issue Oct 12, 2017

Merge pull request prometheus#667 from prometheus/beorn7/doc-improve
Community: Add section to discourage 1:1 support queries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.