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

fix(dagcbor): don't accept trailing bytes #386

Merged
merged 2 commits into from
Mar 16, 2022

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Mar 15, 2022

Just re-using an approach that's used by the dag-json codec rather than trying to hack refmt which won't properly support this kind of inspection since it only considers complete terminal sets rather than caring about where the bytes stop.

This does mean #374 will apply to dag-cbor too, but that's appropriate - we should be erroring here and allowing the user to opt-in (somehow!) to allowing it to go only up to the end of the terminal set and no more.


To try out the fuzz thing I grabbed go 1.18 and ran go test -fuzz=FuzzBindnodeViaDagCBOR in node/bindnode to make it run and the a000 case is fixed there, but it still fails:

fuzz: elapsed: 0s, gathering baseline coverage: 0/25 completed
fuzz: elapsed: 0s, gathering baseline coverage: 25/25 completed, now fuzzing with 16 workers
fuzz: minimizing 112-byte failing input file
fuzz: elapsed: 0s, minimizing
--- FAIL: FuzzBindnodeViaDagCBOR (0.37s)
    --- FAIL: FuzzBindnodeViaDagCBOR (0.00s)
        fuzz_test.go:165: schema in dag-cbor: a1657479706573a164526f6f74a1636d6170a2676b65795479706566537472696e676976616c75655479706563496e74
        fuzz_test.go:166: node in dag-cbor: a2615830613030
        fuzz_test.go:167: schema in dag-json: {"types":{"Root":{"map":{"keyType":"String","valueType":"Int"}}}}
        fuzz_test.go:176: node in dag-json: {"0":-17,"X":-17}
        fuzz_test.go:219: decode and encode roundtrip with dag-cbor repr=false
        fuzz_test.go:238: decode successful: struct { Keys []string; Values map[string]int }{Keys:[]string{"X", "0"}, Values:map[string]int{"0":-17, "X":-17}}
        fuzz_test.go:241: node reencoded as "\xa2a00aX0" rather than "\xa2aX0a00"
        fuzz_test.go:243: re-encode successful: "\xa2a00aX0"
        fuzz_test.go:219: decode and encode roundtrip with dag-cbor repr=true
        fuzz_test.go:238: decode successful: struct { Keys []string; Values map[string]int }{Keys:[]string{"X", "0"}, Values:map[string]int{"0":-17, "X":-17}}
        fuzz_test.go:241: node reencoded as "\xa2a00aX0" rather than "\xa2aX0a00"
        fuzz_test.go:243: re-encode successful: "\xa2a00aX0"
    
    Failing input written to testdata/fuzz/FuzzBindnodeViaDagCBOR/11124a93392709940afb28dd60fecc9841776627a436091b4b9d09da57a97905
    To re-run:
    go test -run=FuzzBindnodeViaDagCBOR/11124a93392709940afb28dd60fecc9841776627a436091b4b9d09da57a97905
FAIL
exit status 1
FAIL    github.com/ipld/go-ipld-prime/node/bindnode     0.557s

is that expected?

@rvagg rvagg requested a review from mvdan March 15, 2022 06:03
Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! I tried calling Step on the tokenizer, which panicked, but it didn't occur to me to use Read directly. I assume this works because refmt doesn't read more than it needs to, e.g. due to buffered reads.

I think the next crasher you've found is not about trailing bytes, but about dagcbor accepting unmarshals where some of the data is out of order (presumably map elements). I think we can deal with that in a separate PR.

codec/dagcbor/unmarshal.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!

@rvagg rvagg merged commit 5261f86 into master Mar 16, 2022
@rvagg rvagg deleted the rvagg/dag-cbor-trailing-bytes branch March 16, 2022 06:33
@rvagg
Copy link
Member Author

rvagg commented Mar 16, 2022

doh! I rebase-merged this, I really wish GitHub had proper native fixup! support for this.

@mvdan @warpfork I hope you don't mind if I force push to master while it's on top?

@mvdan
Copy link
Contributor

mvdan commented Mar 16, 2022

Go ahead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants