search

google / wuffs

Wrangling Untrusted File Formats Safely
Other
4.06k stars 129 forks source link

"truncated input" API change seems like a mess #101

Open pjanx opened 1 year ago

pjanx commented 1 year ago
nigeltao commented 1 year ago

The PNG decoder still loops around "short read" in one place.

Can you elaborate on (1) where is this one place and (2) why this is a problem?

If the answer to (1) is in decoder.do_decode_frame line 1248 then decoder.decode_frame should 'catch' it and turn it into "truncated input".

DecodeImage0 doesn't reflect this change (no DecodeImage_UnexpectedEndOfFile as far as I can tell).

Yeah, DecodeImage_UnexpectedEndOfFile ought to be unreachable now. We could probably remove it in v0.4.

All decoders have their own version of this error, is that necessary?

Go isn't Wuffs, but in the Go standard library, we returned a common error (io.ErrUnexpectedEOF) instead of each image decoder having their own version of this error. This was apparently confusing / surprising, so it's the other way round in Wuffs.

pjanx commented 1 year ago

Can you elaborate on (1) where is this one place and (2) why this is a problem?

https://github.com/google/wuffs/blob/main/std/png/decode_png.wuffs#L1237

Inconsistency. I haven't retested what actually happens, I've just reread the code.

This was apparently confusing / surprising, so it's the other way round in Wuffs.

This particular argument doesn't convince me. The thread is about people trying to receive as much of an image as they can, and dealing with the possibility of receiving an error (warning) at the same time.

nigeltao commented 1 year ago

https://github.com/google/wuffs/blob/main/std/png/decode_png.wuffs#L1237

OK that (line 1237) rolls into line 1248 that I referenced above. I suppose we could make line 1248 return "#truncated input" directly, which would be a little cleaner. As I said, though, decoder.decode_frame should already 'catch' "$short read", so it shouldn't make a difference, in practice.

This particular argument doesn't convince me.

If you're not convinced then we'll agree to disagree. I'm just telling you why it's the way it is.

The thread is about people trying to receive as much of an image as they can, and dealing with the possibility of receiving an error (warning) at the same time.

There's two concerns in that thread. One is about trying to receive as much of an image as they can, as you said. Two is about what error you get when it's truncated (a generic io error versus an image-format-specific jpeg error) which is relevant to your "All decoders have their own version of this error, is that necessary?" in the OP. A quote from that thread: "How do I know that io.ErrUnexpectedEOF has anything to do with "wrong" JPEGs?"