Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(43)

Issue 6285050: code review 6285050: fmt, encoding/gob: fix misuse of Read (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years ago by minux1
Modified:
9 years, 6 months ago
Reviewers:
CC:
bradfitz, rsc, ality, golang-dev
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r 685b1b62a34f https://code.google.com/p/go/ #

Patch Set 3 : diff -r 685b1b62a34f https://code.google.com/p/go/ #

Patch Set 4 : diff -r 685b1b62a34f https://code.google.com/p/go/ #

Patch Set 5 : diff -r e61b6119272b https://code.google.com/p/go/ #

Patch Set 6 : diff -r e61b6119272b https://code.google.com/p/go/ #

Total comments: 3

Patch Set 7 : diff -r ac06fe42df6d https://code.google.com/p/go/ #

Patch Set 8 : diff -r 917109e5566b https://code.google.com/p/go/ #

Patch Set 9 : diff -r 25ed37450fc9 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -4 lines) Patch
M src/pkg/encoding/gob/decode.go View 1 2 3 4 5 6 1 chunk +3 lines, -3 lines 0 comments Download
M src/pkg/fmt/scan.go View 1 2 3 4 5 6 7 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 20
minux1
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/
9 years, 6 months ago (2012-12-13 14:18:57 UTC) #1
bradfitz
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 == ...
9 years, 6 months ago (2012-12-13 16:05:37 UTC) #2
rsc
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 ...
9 years, 6 months ago (2012-12-13 16:15:00 UTC) #3
minux1
On Fri, Dec 14, 2012 at 12:05 AM, <bradfitz@golang.org> 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> > ...
9 years, 6 months ago (2012-12-13 16:16:47 UTC) #4
minux1
PTAL. On Fri, Dec 14, 2012 at 12:15 AM, <rsc@golang.org> 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> ...
9 years, 6 months ago (2012-12-13 16:25:58 UTC) #5
minux1
On Fri, Dec 14, 2012 at 12:16 AM, minux <minux.ma@gmail.com> wrote: > ReadFull has its ...
9 years, 6 months ago (2012-12-13 16:37:48 UTC) #6
rsc
I am not worried about Read returning 0, nil in these cases. That's allowed but ...
9 years, 6 months ago (2012-12-13 16:49:22 UTC) #7
bradfitz
On Thu, Dec 13, 2012 at 8:49 AM, Russ Cox <rsc@golang.org> wrote: > I am ...
9 years, 6 months ago (2012-12-13 20:25:29 UTC) #8
rsc
On Thu, Dec 13, 2012 at 3:25 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > What does ...
9 years, 6 months ago (2012-12-13 20:30:50 UTC) #9
minux1
On Fri, Dec 14, 2012 at 4:25 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > On Thu, Dec ...
9 years, 6 months ago (2012-12-13 20:31:25 UTC) #10
bradfitz
On Thu, Dec 13, 2012 at 12:30 PM, Russ Cox <rsc@golang.org> wrote: > On Thu, ...
9 years, 6 months ago (2012-12-13 20:36:14 UTC) #11
rsc
I need to think more about all this but I don't have any uninterrupted chunks ...
9 years, 6 months ago (2012-12-13 20:40:24 UTC) #12
minux1
On Fri, Dec 14, 2012 at 4:36 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > > On Thu, ...
9 years, 6 months ago (2012-12-13 20:47:53 UTC) #13
ality
Brad Fitzpatrick <bradfitz@golang.org> once said: > b) outright ban (0, nil) from io.Readers It's not ...
9 years, 6 months ago (2012-12-14 00:24:36 UTC) #14
bradfitz
On Thu, Dec 13, 2012 at 4:24 PM, Anthony Martin <ality@pbrane.org> wrote: > Brad Fitzpatrick ...
9 years, 6 months ago (2012-12-14 04:47:07 UTC) #15
minux1
On Fri, Dec 14, 2012 at 8:24 AM, Anthony Martin <ality@pbrane.org> wrote: > Brad Fitzpatrick ...
9 years, 6 months ago (2012-12-14 04:56:36 UTC) #16
rsc
Please change this code to use io.ReadFull. Then at least we can get this CL ...
9 years, 6 months ago (2012-12-17 16:17:23 UTC) #17
minux1
On 2012/12/17 16:17:23, rsc wrote: > Please change this code to use io.ReadFull. Then at ...
9 years, 6 months ago (2012-12-17 16:33:00 UTC) #18
rsc
LGTM Thanks.
9 years, 6 months ago (2012-12-17 17:01:32 UTC) #19
minux1
9 years, 6 months ago (2012-12-17 17:27:04 UTC) #20
*** 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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b