Closed rvagg closed 2 years ago
I think that assemblerRepr()
needs to be sensitive to _errorAssembler
types and pass those through if it sees them; as it is it's brute forcing _assembler
and failing.
I'm not sure why I can't manually reproduce that with a trivial example like the one at https://github.com/ipld/go-ipld-prime/issues/342
Thanks @rvagg - I've been able to repro by adding a few more tests to bindnode. I'll send a PR shortly.
Thanks @mvdan & @rvagg -- it's probably important we solve as many issues as we can before we put this out there.
One other option we may want to look at -- one of the popular go CBOR libraries has a fuzzing utility that includes a corpus -- https://github.com/fxamacker/cbor-fuzz/tree/master/corpus -- we could just feed these to the decoder and see if we get a fail.
AND we may also want to throw random bytes at the decoder, just to make sure we don't hit issues with REFMT's CBOR decoding.
There is an interesting question here around responsibility -- at what point is just insuring that cbor decodes properly an IPLD prime / refmt problem and not a graphsync problem.
FYI, I intend to tackle https://github.com/ipld/go-ipld-prime/issues/357 next. I imagine I'll have picked most of the low-hanging fruit found via fuzzing in about a week's time from now.
There is an interesting question here around responsibility
@hannahhoward yeah, I certainly don't think it's the responsibility of this library; I'm taking a personal interest because I know how new and raw everything is and significant potential for us shipping something that can easily DoS a Filecoin SP if we allow crashing. I'm anticipating @mvdan taking over this work, as per https://github.com/ipld/go-ipld-prime/issues/357.
But what I was thinking, was that we should add a panic recover into the messaging layer, since we know how complex and new it is. I just didn't get around to doing that (and figuring out how to do it) in the initial round of changes. We should probably have a ticket for it though I suppose!
Closing this because it's not something I think is particularly useful here, at least in its current state. Will leave the branch for now though.
This might just be a temporary branch but I wanted to throw some garbage, but valid, dag-cbor at the message decoder to see how easy it was to break; it turns out it's still pretty trivial to break but I don't really understand why. So this one's for @mvdan.
Any time it comes up with a top-level map it'll panic like this:
I initially started this branch on an version of the code prior to the schema changes in https://github.com/ipfs/go-graphsync/pull/354 with the same error, so it's not just due the top-level keyed union introduced there.
I was also playing around with a variation of the example posted in https://github.com/ipld/go-ipld-prime/issues/342 and was able to easily get
panic: bindnode TODO: invalid key: "J" is not a field in type Foo
if I just fed it a map with a misnamed field.