FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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


Description:
fmt, encoding/gob: fix misuse of Read
Read() can return both 0,nil and len(buf),err.
Fixes issue 3472.

Please review this at https://codereview.appspot.com/6285050/

Affected files:
M src/pkg/encoding/gob/decode.go
M src/pkg/fmt/scan.go


Index: src/pkg/encoding/gob/decode.go
===================================================================
--- a/src/pkg/encoding/gob/decode.go
+++ b/src/pkg/encoding/gob/decode.go
@@ -62,15 +62,18 @@
// Used only by the Decoder to read the message length.
func decodeUintReader(r io.Reader, buf []byte) (x uint64, width int, err
error) {
width = 1
- _, err = r.Read(buf[0:width])
- if err != nil {
+ n, err := io.ReadFull(r, buf[0:width])
+ if n < 1 {
+ if err == nil {
+ err = io.EOF
+ }
return
}
b := buf[0]
if b <= 0x7f {
return uint64(b), width, nil
}
- n := -int(int8(b))
+ n = -int(int8(b))
if n > uint64Size {
err = errBadUint
return
Index: src/pkg/fmt/scan.go
===================================================================
--- a/src/pkg/fmt/scan.go
+++ b/src/pkg/fmt/scan.go
@@ -337,7 +337,12 @@
r.pending--
return
}
- _, err = r.reader.Read(r.pendBuf[0:1])
+ n, err := r.reader.Read(r.pendBuf[0:1])
+ if n == 1 {
+ err = nil
+ } else if n == 0 && err == nil {
+ err = io.EOF
+ }
return r.pendBuf[0], err
}

Search Discussions

  • Bradfitz at Dec 13, 2012 at 4:05 pm
    https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go
    File src/pkg/fmt/scan.go (right):

    https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode343
    src/pkg/fmt/scan.go:343: } else if n == 0 && err == nil {
    doe this ever happen? The way I've always read io.Reader's contract,

    "If some data is available but not len(p) bytes, Read conventionally
    returns what is available instead of waiting for more."

    ... is that you block until at least 1 byte.

    I know it doesn't say that, though.

    It feels like this file doesn't need changing. Or if so, why not
    ReadFull?

    https://codereview.appspot.com/6285050/
  • Minux at Dec 13, 2012 at 4:16 pm

    On Fri, Dec 14, 2012 at 12:05 AM, wrote:

    https://codereview.appspot.**com/6285050/diff/11002/src/**
    pkg/fmt/scan.go#newcode343<https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode343>
    src/pkg/fmt/scan.go:343: } else if n == 0 && err == nil {
    doe this ever happen? The way I've always read io.Reader's contract,

    "If some data is available but not len(p) bytes, Read conventionally
    returns what is available instead of waiting for more."

    ... is that you block until at least 1 byte.

    I know it doesn't say that, though.
    I originally though so too, but as Russ commented in
    http://code.google.com/p/go/issues/detail?id=3472#c1
    Read can return 0, nil (which I think most of Read users are not able to
    deal with), or
    len(buf), err
    It feels like this file doesn't need changing. Or if so, why not
    ReadFull?
    ReadFull has its own problems dealing with "naughty" io.Readers.

    I think the essential question is: what to do with a Read that returns 0,
    nil?
    Should we report an error or try again?
  • Minux at Dec 13, 2012 at 4:37 pm

    On Fri, Dec 14, 2012 at 12:16 AM, minux wrote:

    ReadFull has its own problems dealing with "naughty" io.Readers.
    consider a reader r which returns len(buf), err
    but err != io.EOF

    should io.ReadFull(r, buf) return an error or not?

    I tried to fix ReadFull by not returning the error if Read already
    fills the buf, however, at least one test failed.

    --- FAIL: TestDecoderIssue3577 (5.00 seconds)
    base64_test.go:275: timeout; Decoder blocked without returning an error
    FAIL

    maybe we should note for ReadFull that users should still look at n instead
    of
    err first, but unfortunately, a lot of code make the assumption that if
    ReadFull
    returns an error, the buf is not completely filled.
  • Russ Cox at Dec 13, 2012 at 4:49 pm
    I am not worried about Read returning 0, nil in these cases. That's
    allowed but if you are doing Fscanf on a UDP socket you kind of
    deserve whatever you get.

    Whether we should change ReadAtLeast and ReadFull to clear err when
    returning a full buffer is something I'd still like to think more
    about. I created 4544 for that.

    Russ
  • Brad Fitzpatrick at Dec 13, 2012 at 8:32 pm

    On Thu, Dec 13, 2012 at 8:49 AM, Russ Cox wrote:

    I am not worried about Read returning 0, nil in these cases. That's
    allowed but if you are doing Fscanf on a UDP socket you kind of
    deserve whatever you get.
    What does a UDP socket have to do with this?

    Either Read can return (0, nil) and we have to deal with it, or Read can't
    return (0, nil) and we don't.

    Whether it's a UDP socket or package foo or package bar doesn't matter.

    Even if pkg foo vs pkg bar were an issue, both fmt and encoding/gob can
    work with many types of Readers.
  • Russ Cox at Dec 13, 2012 at 8:30 pm

    On Thu, Dec 13, 2012 at 3:25 PM, Brad Fitzpatrick wrote:
    What does a UDP socket have to do with this?
    The readers that can return 0, nil are ones modeling a packet-based
    input stream, such as UDP datagrams.

    Russ
  • Brad Fitzpatrick at Dec 13, 2012 at 8:36 pm

    On Thu, Dec 13, 2012 at 12:30 PM, Russ Cox wrote:
    On Thu, Dec 13, 2012 at 3:25 PM, Brad Fitzpatrick wrote:
    What does a UDP socket have to do with this?
    The readers that can return 0, nil are ones modeling a packet-based
    input stream, such as UDP datagrams.

    Wow, crazy. Not the answer I was expecting.

    I thought io.Reader was a byte stream interface, not a message-stream
    interface.

    If somebody cares about UDP boundaries, they can just use package net and
    UDPConn directly, can't they?

    Can we update the docs on io.Reader to either a) make this explicit about
    UDP sockets, or b) outright ban (0, nil) from io.Readers and fix
    UDPConn.Read? (I would prefer b)
  • Minux at Dec 13, 2012 at 8:47 pm

    On Fri, Dec 14, 2012 at 4:36 AM, Brad Fitzpatrick wrote:
    On Thu, Dec 13, 2012 at 12:30 PM, Russ Cox wrote:

    On Thu, Dec 13, 2012 at 3:25 PM, Brad Fitzpatrick <bradfitz@golang.org>
    wrote:
    What does a UDP socket have to do with this?
    The readers that can return 0, nil are ones modeling a packet-based
    input stream, such as UDP datagrams.
    I thought io.Reader was a byte stream interface, not a message-stream
    interface.

    If somebody cares about UDP boundaries, they can just use package net and
    UDPConn directly, can't they?

    Can we update the docs on io.Reader to either a) make this explicit about
    UDP sockets, or b) outright ban (0, nil) from io.Readers and fix
    UDPConn.Read? (I would prefer b)
    i think option b will break (*net.UDPConn).Read's established Go 1
    semantics.
    Although I agree with you about the implied byte stream style interface for
    io.Reader.

    no matter which solution we choose, i think we need more extensive
    documentation
    on io.Reader, and the official and correct way to deal with its returned
    values.

    the docs for io.Writer is much clearer about error returns than io.Reader,
    i wonder
    where does the extra complexities for io.Reader come from?
  • Russ Cox at Dec 13, 2012 at 8:48 pm
    I need to think more about all this but I don't have any uninterrupted
    chunks of time today. Issue 4544 reminds me to do that later. :-)
  • Anthony Martin at Dec 14, 2012 at 12:24 am

    Brad Fitzpatrick once said:
    b) outright ban (0, nil) from io.Readers
    It's not just UDP. The preservation of message boundaries can be very
    useful if you're not stuck with Unix. On Plan 9 there are a few programs
    that take advantage of zero length writes and I want to be able to
    interface with them.

    Please don't add this artificial restriction to io.Readers.

    Anthony
  • Brad Fitzpatrick at Dec 14, 2012 at 4:47 am

    On Thu, Dec 13, 2012 at 4:24 PM, Anthony Martin wrote:

    Brad Fitzpatrick <bradfitz@golang.org> once said:
    b) outright ban (0, nil) from io.Readers
    It's not just UDP. The preservation of message boundaries can be very
    useful if you're not stuck with Unix. On Plan 9 there are a few programs
    that take advantage of zero length writes and I want to be able to
    interface with them.

    Please don't add this artificial restriction to io.Readers.
    This isn't a discussion about Plan 9 semantics. This is a discussion about
    io.Reader semantics.

    io.Reader does not document any message boundary guarantees today. Any you
    happen to get right now are a side-effect of the Plan 9 port's
    implementation, and not language guarantees.
  • Minux at Dec 14, 2012 at 4:56 am

    On Fri, Dec 14, 2012 at 8:24 AM, Anthony Martin wrote:

    Brad Fitzpatrick <bradfitz@golang.org> once said:
    b) outright ban (0, nil) from io.Readers
    It's not just UDP. The preservation of message boundaries can be very
    useful if you're not stuck with Unix. On Plan 9 there are a few programs
    that take advantage of zero length writes and I want to be able to
    interface with them.
    Could you please elaborate a bit?
    Please don't add this artificial restriction to io.Readers.
    I think the problem is: allowing (0, nil) return will complicate all users
    of
    io.Readers, while few programs actually require this capability.

    Allowing it means you are on your own if you're using this capability
    because
    even the standard library could not handle it (i think correctly handle it
    means
    we can't use Read() directly, and must resort to something like
    io.ReadAtLeast
    or io.ReadFull)
  • Russ Cox at Dec 17, 2012 at 4:17 pm
    Please change this code to use io.ReadFull. Then at least we can get
    this CL behind us. The other issue remains open.

    Thanks.
    Russ
  • Minux at Dec 13, 2012 at 8:38 pm

    On Fri, Dec 14, 2012 at 4:25 AM, Brad Fitzpatrick wrote:
    On Thu, Dec 13, 2012 at 8:49 AM, Russ Cox wrote:

    I am not worried about Read returning 0, nil in these cases. That's
    allowed but if you are doing Fscanf on a UDP socket you kind of
    deserve whatever you get.
    What does a UDP socket have to do with this?
    i guess it's because it's possible to get a 0-byte (payload) UDP packet, so
    it's ok for (*net.UDPConn).Read
    to return 0, nil for that case.
    Either Read can return (0, nil) and we have to deal with it, or Read can't
    return (0, nil) and we don't.
    I think the situation is: Read can return (0, nil) for the UDP case, but we
    really can't do much about it.
  • Rsc at Dec 13, 2012 at 4:15 pm
    https://codereview.appspot.com/6285050/diff/11002/src/pkg/encoding/gob/decode.go
    File src/pkg/encoding/gob/decode.go (right):

    https://codereview.appspot.com/6285050/diff/11002/src/pkg/encoding/gob/decode.go#newcode66
    src/pkg/encoding/gob/decode.go:66: if n < 1 {
    You can just use

    if n == 0 {
    return
    }

    The only goal is to avoid bailing when we get n==1, err != nil.

    https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go
    File src/pkg/fmt/scan.go (right):

    https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode341
    src/pkg/fmt/scan.go:341: if n == 1 {
    Same here: if n == 1 { err = nil }
    is all you need.

    https://codereview.appspot.com/6285050/
  • Minux at Dec 13, 2012 at 4:32 pm
    PTAL.
    On Fri, Dec 14, 2012 at 12:15 AM, wrote:

    https://codereview.appspot.**com/6285050/diff/11002/src/**
    pkg/encoding/gob/decode.go#**newcode66<https://codereview.appspot.com/6285050/diff/11002/src/pkg/encoding/gob/decode.go#newcode66>
    src/pkg/encoding/gob/decode.**go:66: if n < 1 {
    You can just use

    if n == 0 {
    return
    }

    The only goal is to avoid bailing when we get n==1, err != nil.

    <https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode341>
    https://codereview.appspot.**com/6285050/diff/11002/src/**
    pkg/fmt/scan.go#newcode341<https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode341>
    src/pkg/fmt/scan.go:341: if n == 1 {
    Same here: if n == 1 { err = nil }
    is all you need.
    So we choose to not handle the 0, nil case?
    If we can't find a way to deal with that case, can we say that Read
    shouldn't return 0, nil
    as imo it will break most of the code.
  • Minux Ma at Dec 17, 2012 at 4:33 pm

    On 2012/12/17 16:17:23, rsc wrote:
    Please change this code to use io.ReadFull. Then at least we can get
    this CL behind us. The other issue remains open.
    Sure. PTAL.

    https://codereview.appspot.com/6285050/
  • Rsc at Dec 17, 2012 at 5:01 pm
  • Minux Ma at Dec 17, 2012 at 5:27 pm
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=7ea3674ce4b5 ***

    fmt, encoding/gob: fix misuse of Read
    reader.Read() can return both 0,nil and len(buf),err.
    To be safe, we use io.ReadFull instead of doing reader.Read directly.

    Fixes issue 3472.

    R=bradfitz, rsc, ality
    CC=golang-dev
    https://codereview.appspot.com/6285050


    https://codereview.appspot.com/6285050/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 13, '12 at 2:18p
activeDec 17, '12 at 5:27p
posts20
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase