Bugs related to codecs that return EOF on encode/decode (EOF error are ignored by CLI so we would need to either fix that or wrap the errors) #8378
Comments
|
@aschmahmann suggested codecs may be returning EOF when they shouldn't? Or maybe they should be returning UnexpectedEOF rather than EOF in some cases? I do believe there may be upstream bugs here, but info on which codecs, and what exactly the misbehavior is, would help. |
|
Possibly relatedly, I've also recently found at least one bug upstream where a codec returns not enough EOF errors: ipld/go-ipld-prime#230 |
|
A couple of examples of this are: They both don't error or return a CID |
|
At least on Go 1.17, I can't reproduce the dag-json one: And it seems like master (701828d) errors correctly. Am I missing something? The git-raw case is the same story; on 0.9.1 it errors with "no input parser", and on master it correctly fails: |
|
For the sake of completeness, I went through the trouble of installing Go 1.16.7, and it has the same results as Go 1.17 above. |
|
@mvdan are you running a daemon or just running the ipfs offline? If you run To be more precise the EOF errors are ignored by the CLI when it is using the HTTP API behind the scenes (e.g. https://github.com/ipfs/go-ipfs-cmds/blob/4ade007405e5d3befb14184290576c63cc43a6a3/http/responseemitter.go#L175). So these are both giving you EOFs, but EOF doesn't really indicate an error which is why the HTTP API skips it. |
|
Ah, oops. I guess you can file that under "repro steps unclear" :) I wonder why I'm not getting a clear error along the lines of "IPFS daemon not found". I'll take another look. |
|
Thanks, I can reproduce now. TIL that the ipfs CLI can work in "offline mode" as well as "online mode" with a daemon. I've looked through the code, and I think I agree with Eric. The codec decoder is returning io.EOF when it should be returning io.ErrUnexpectedEOF. The latter is clearly fatal; the former is usually just a "no more input" signal. It's perhaps wrong for go-ipfs-cmds to ignore io.EOF as a nil error, but changing that will likely break other stuff. Much easier to make the ipld-prime codecs return better errors, which we probably want to do anyway. |
The added test cases would fail before this fix:
--- FAIL: TestDecodeError/Empty (0.00s)
unmarshal_test.go:28: Decode("") got "EOF", want "unexpected EOF"
--- FAIL: TestDecodeError/NoSpace (0.00s)
unmarshal_test.go:28: Decode("foo") got "EOF", want "unexpected EOF"
They would still return an error, but they'd just return io.EOF.
While that error signals that we've hit the end of input,
it doesn't signal to the codec user that something went wrong.
An io.EOF is not a worrying error, in many situations.
If the input is empty, or is too short to be a valid git object,
we should return a clear error to signal the operation did not work.
io.ErrUnexpectedEOF is unambiguous,
and will make go-ipfs start erroring on this edge case.
For ipfs/kubo#8378.
The added test cases would fail before this fix:
--- FAIL: TestDecodeError/Empty (0.00s)
unmarshal_test.go:28: Decode("") got "EOF", want "unexpected EOF"
--- FAIL: TestDecodeError/NoSpace (0.00s)
unmarshal_test.go:28: Decode("foo") got "EOF", want "unexpected EOF"
They would still return an error, but they'd just return io.EOF.
While that error signals that we've hit the end of input,
it doesn't signal to the codec user that something went wrong.
An io.EOF is not a worrying error, in many situations.
If the input is empty, or is too short to be a valid git object,
we should return a clear error to signal the operation did not work.
io.ErrUnexpectedEOF is unambiguous,
and will make go-ipfs start erroring on this edge case.
For ipfs/kubo#8378.
The added test cases would fail before this fix:
--- FAIL: TestDecodeError/Empty (0.00s)
unmarshal_test.go:28: Decode("") got "EOF", want "unexpected EOF"
--- FAIL: TestDecodeError/NoSpace (0.00s)
unmarshal_test.go:28: Decode("foo") got "EOF", want "unexpected EOF"
They would still return an error, but they'd just return io.EOF.
While that error signals that we've hit the end of input,
it doesn't signal to the codec user that something went wrong.
An io.EOF is not a worrying error, in many situations.
If the input is empty, or is too short to be a valid git object,
we should return a clear error to signal the operation did not work.
io.ErrUnexpectedEOF is unambiguous,
and will make go-ipfs start erroring on this edge case.
For ipfs/kubo#8378.
|
Alright, the upstream fix for dag-json is ipld/go-ipld-prime#241, and for git-raw it's ipfs/go-ipld-git#53. When those are merged, we should be able to just pull master from both of them and be ready for a release. If we need those to also be releases, we can also do that. I imagine that regression tests for |
The added test cases would fail before this fix:
--- FAIL: TestDecodeError/Empty (0.00s)
unmarshal_test.go:28: Decode("") got "EOF", want "unexpected EOF"
--- FAIL: TestDecodeError/NoSpace (0.00s)
unmarshal_test.go:28: Decode("foo") got "EOF", want "unexpected EOF"
They would still return an error, but they'd just return io.EOF.
While that error signals that we've hit the end of input,
it doesn't signal to the codec user that something went wrong.
An io.EOF is not a worrying error, in many situations.
If the input is empty, or is too short to be a valid git object,
we should return a clear error to signal the operation did not work.
io.ErrUnexpectedEOF is unambiguous,
and will make go-ipfs start erroring on this edge case.
For ipfs/kubo#8378.
|
@mvdan: I think once merged, lets bump the version of go-ipld-version that go-ipfs depends on and I think we're good. @aschmahmann can weight in, but he's out until 2021-09-09. |
In particular, if the input doesn't have a single token, we would return io.EOF, which can be easily mistaken by an error that can be ignored. In particular, go-ipfs-cmds ignored it as such. Make them return a clearly bad "unexpected EOF" error, which mirrors the behavior found in many other unmarshal APIs such as encoding/json.Unmarshal. Finally, add a table-driven test that we can extend in the future, which tests all four codecs. For ipfs/kubo#8378.
No description provided.
The text was updated successfully, but these errors were encountered: