code review 178120043: image/jpeg: handle Read returning n > 0, err != nil in ...

image/jpeg: handle Read returning n > 0, err != nil in d.fill

Fixes issue 9127.

[rsc, 2014-11-21 20:17:26]

Hello bradfitz, r (cc: golang-codereviews@googlegroups.com, nigeltao),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2014-11-21 21:00:31]

I assume the error was io.EOF. Should we record non-nil && non-EOF errors
in the *decoder buf as a new stickyErr field for later? The io.Reader
definition only says that EOF must be sticky (implicitly: "The next Read
should return 0, EOF regardless") but doesn't say that other errors must be
returned repeatedly. So I'm afraid of this code failing when receiving a
non-EOF error from a reader once (with data), and then losing the error
value, and not getting it again in the next fill.

Test?

Documentation on the fill func on what an error return means? (An error is
returned iff no bytes were read into the buffer?)



On Fri, Nov 21, 2014 at 12:17 PM, <rsc@golang.org> wrote:

> Reviewers: bradfitz, r,
>
> Message:
> Hello bradfitz, r (cc: golang-codereviews@googlegroups.com, nigeltao),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> image/jpeg: handle Read returning n > 0, err != nil in d.fill
>
> Fixes issue 9127.
>
> Please review this at https://codereview.appspot.com/178120043/
>
> Affected files (+3, -0 lines):
>   M src/image/jpeg/reader.go
>
>
> Index: src/image/jpeg/reader.go
> ===================================================================
> --- a/src/image/jpeg/reader.go
> +++ b/src/image/jpeg/reader.go
> @@ -143,6 +143,9 @@
>         // Fill in the rest of the buffer.
>         n, err := d.r.Read(d.bytes.buf[d.bytes.j:])
>         d.bytes.j += n
> +       if n > 0 {
> +               err = nil
> +       }
>         return err
>  }
>
>
>
>

[r, 2014-11-21 21:05:49]

Brad, what error is it getting?

-rob

[rsc, 2014-11-21 21:06:07]

All Read errors should be sticky. I would hope that would go without
saying. If you say Read(p) and it says "I can't" it makes absolutely no
sense that if you say "Read(p)" again it says "oh, now I can!". Put another
way, if the next Read succeeds then the error can't have been that
important an error.

Will figure out a test later. Would like hear from Rob about whether this
is worth putting in 1.4. (If not, it's not worth putting in 1.4.1 either.)

[bradfitz, 2014-11-21 21:10:28]

On Fri, Nov 21, 2014 at 1:06 PM, Russ Cox <rsc@golang.org> wrote:

> All Read errors should be sticky. I would hope that would go without
> saying. If you say Read(p) and it says "I can't" it makes absolutely no
> sense that if you say "Read(p)" again it says "oh, now I can!". Put another
> way, if the next Read succeeds then the error can't have been that
> important an error.
>

Fair enough.


> Will figure out a test later. Would like hear from Rob about whether this
> is worth putting in 1.4. (If not, it's not worth putting in 1.4.1 either.)
>

FWIW I think it's worth it, because I think it's a regression: I've been
making a number of io.Readers return (n, EOF) instead of (n, nil)+(0, EOF)
recently for performance/garbage reasons (I can dig up the CLs/history if
needed).  So in older Go versions, this image/jpeg decoding from a
net/http.Response.Body would've worked (on accident), because I used to
never return (n, EOF) from http.

[r, 2014-11-21 21:14:00]

I'm fine with the fix but I'd still like to know what error Brad is seeing.

I would say it belongs in 1.4.

-rob


On Sat, Nov 22, 2014 at 8:10 AM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> On Fri, Nov 21, 2014 at 1:06 PM, Russ Cox <rsc@golang.org> wrote:
>
>> All Read errors should be sticky. I would hope that would go without
>> saying. If you say Read(p) and it says "I can't" it makes absolutely no
>> sense that if you say "Read(p)" again it says "oh, now I can!". Put another
>> way, if the next Read succeeds then the error can't have been that
>> important an error.
>>
>
> Fair enough.
>
>
>> Will figure out a test later. Would like hear from Rob about whether this
>> is worth putting in 1.4. (If not, it's not worth putting in 1.4.1 either.)
>>
>
> FWIW I think it's worth it, because I think it's a regression: I've been
> making a number of io.Readers return (n, EOF) instead of (n, nil)+(0, EOF)
> recently for performance/garbage reasons (I can dig up the CLs/history if
> needed).  So in older Go versions, this image/jpeg decoding from a
> net/http.Response.Body would've worked (on accident), because I used to
> never return (n, EOF) from http.
>
>
>

[r, 2014-11-21 21:15:15]

LGTM although a test would be good

https://codereview.appspot.com/178120043/diff/40001/src/image/jpeg/reader.go
File src/image/jpeg/reader.go (right):

https://codereview.appspot.com/178120043/diff/40001/src/image/jpeg/reader.go#...
src/image/jpeg/reader.go:147: err = nil
s;$; // Be sure to process these bytes.;

[bradfitz, 2014-11-21 21:19:58]

The error I saw was pasted into https://golang.org/issue/9127 :

mac:~ bradfitz$ go run d.go
2014/11/18 18:00:20 invalid JPEG format: missing 0xff00 sequence
exit status 1

I'm assuming that's because the http.(*Response).Body.Read was returning
(n>0, io.EOF), and jpeg's fill() was only seeing the EOF and ignoring the
bytes p[n:] that were read successfully, then causing the JPEG parser to
fail to see the "0xff00 sequence", wherever that is.


On Fri, Nov 21, 2014 at 1:13 PM, Rob Pike <r@golang.org> wrote:

> I'm fine with the fix but I'd still like to know what error Brad is seeing.
>
> I would say it belongs in 1.4.
>
> -rob
>
>
> On Sat, Nov 22, 2014 at 8:10 AM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
>
>> On Fri, Nov 21, 2014 at 1:06 PM, Russ Cox <rsc@golang.org> wrote:
>>
>>> All Read errors should be sticky. I would hope that would go without
>>> saying. If you say Read(p) and it says "I can't" it makes absolutely no
>>> sense that if you say "Read(p)" again it says "oh, now I can!". Put another
>>> way, if the next Read succeeds then the error can't have been that
>>> important an error.
>>>
>>
>> Fair enough.
>>
>>
>>> Will figure out a test later. Would like hear from Rob about whether
>>> this is worth putting in 1.4. (If not, it's not worth putting in 1.4.1
>>> either.)
>>>
>>
>> FWIW I think it's worth it, because I think it's a regression: I've been
>> making a number of io.Readers return (n, EOF) instead of (n, nil)+(0, EOF)
>> recently for performance/garbage reasons (I can dig up the CLs/history if
>> needed).  So in older Go versions, this image/jpeg decoding from a
>> net/http.Response.Body would've worked (on accident), because I used to
>> never return (n, EOF) from http.
>>
>>
>>
>

[bradfitz, 2014-11-21 21:20:28]

er, p[:n]

On Fri, Nov 21, 2014 at 1:19 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> The error I saw was pasted into https://golang.org/issue/9127 :
>
> mac:~ bradfitz$ go run d.go
> 2014/11/18 18:00:20 invalid JPEG format: missing 0xff00 sequence
> exit status 1
>
> I'm assuming that's because the http.(*Response).Body.Read was returning
> (n>0, io.EOF), and jpeg's fill() was only seeing the EOF and ignoring the
> bytes p[n:] that were read successfully, then causing the JPEG parser to
> fail to see the "0xff00 sequence", wherever that is.
>
>
> On Fri, Nov 21, 2014 at 1:13 PM, Rob Pike <r@golang.org> wrote:
>
>> I'm fine with the fix but I'd still like to know what error Brad is
>> seeing.
>>
>> I would say it belongs in 1.4.
>>
>> -rob
>>
>>
>> On Sat, Nov 22, 2014 at 8:10 AM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>>
>>> On Fri, Nov 21, 2014 at 1:06 PM, Russ Cox <rsc@golang.org> wrote:
>>>
>>>> All Read errors should be sticky. I would hope that would go without
>>>> saying. If you say Read(p) and it says "I can't" it makes absolutely no
>>>> sense that if you say "Read(p)" again it says "oh, now I can!". Put
another
>>>> way, if the next Read succeeds then the error can't have been that
>>>> important an error.
>>>>
>>>
>>> Fair enough.
>>>
>>>
>>>> Will figure out a test later. Would like hear from Rob about whether
>>>> this is worth putting in 1.4. (If not, it's not worth putting in 1.4.1
>>>> either.)
>>>>
>>>
>>> FWIW I think it's worth it, because I think it's a regression: I've been
>>> making a number of io.Readers return (n, EOF) instead of (n, nil)+(0, EOF)
>>> recently for performance/garbage reasons (I can dig up the CLs/history if
>>> needed).  So in older Go versions, this image/jpeg decoding from a
>>> net/http.Response.Body would've worked (on accident), because I used to
>>> never return (n, EOF) from http.
>>>
>>>
>>>
>>
>

[r, 2014-11-21 21:26:32]

I was wondering what error the Read got. I am asking to verify your
assumption.

-rob

[rsc, 2014-11-21 21:33:19]

net.Read can and does return realdata, EOF often. I am 99% sure that was
the problem. I reproduced Brad's failure, though I didn't print what the
error was. I'll do that when I write the test.

[bradfitz, 2014-11-21 21:34:58]

Verified. It is io.EOF.

        // Fill in the rest of the buffer.
        n, err := d.r.Read(d.bytes.buf[d.bytes.j:])
        if n > 0 && err != nil {
                log.Printf("JPEG ERROR: %v (%v)", err, err == io.EOF)
        }
        d.bytes.j += n
        return err

mac:~ bradfitz$ go run d.go
2014/11/21 13:34:16 JPEG ERROR: EOF (true)
2014/11/21 13:34:16 invalid JPEG format: missing 0xff00 sequence
exit status 1


On Fri, Nov 21, 2014 at 1:26 PM, Rob Pike <r@golang.org> wrote:

> I was wondering what error the Read got. I am asking to verify your
> assumption.
>
> -rob
>
>

[r, 2014-11-21 21:40:49]

Thanks.

[nigeltao, 2014-11-21 23:03:08]

On 22 Nov 2014 08:10, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
> FWIW I think it's worth it, because I think it's a regression:

Yeah, it's a regression.
https://code.google.com/p/go/source/detail?r=90c0d9c4a9ad landed in the 1.4
cycle.

[rsc, 2014-11-22 18:49:14]

There's now a test. Will submit shortly.

[rsc, 2014-11-22 18:55:44]

*** Submitted as https://code.google.com/p/go/source/detail?r=95f5614b4648 ***

image/jpeg: handle Read returning n > 0, err != nil in d.fill

Fixes issue 9127.

LGTM=r
R=bradfitz, r
CC=golang-codereviews, nigeltao
https://codereview.appspot.com/178120043