Issue 7602045: code review 7602045: image/gif: reject a GIF image if frame bounds larger th... (Closed)
|
Created:
12 years, 1 month ago by jeff.allen Modified:
12 years, 1 month ago Reviewers:
dave, albert.strasheim CC:
nigeltao, jra, r, bradfitz, golang-dev Visibility:
Public. |
Descriptionimage/gif: reject a GIF image if frame bounds larger than image bounds
The GIF89a spec says: "Each image must fit within the
boundaries of the Logical Screen, as defined in the
Logical Screen Descriptor." Also, do not accept
GIFs which have too much data for the image size.
Patch Set 1 #Patch Set 2 : diff -r 5957d9d08900 http://code.google.com/p/go #Patch Set 3 : diff -r 5957d9d08900 http://code.google.com/p/go #Patch Set 4 : diff -r 5957d9d08900 http://code.google.com/p/go #
Total comments: 19
Patch Set 5 : diff -r 786e094255c9 http://code.google.com/p/go #Patch Set 6 : diff -r 786e094255c9 http://code.google.com/p/go #
Total comments: 11
Patch Set 7 : diff -r 3246e13bf1ca http://code.google.com/p/go #Patch Set 8 : diff -r 3246e13bf1ca http://code.google.com/p/go #
Total comments: 10
Patch Set 9 : diff -r ee1b8339ab04 http://code.google.com/p/go #
Total comments: 1
Patch Set 10 : diff -r 9ca85035f95a http://code.google.com/p/go #
Total comments: 2
Patch Set 11 : diff -r 9ca85035f95a http://code.google.com/p/go #Patch Set 12 : diff -r 9ca85035f95a http://code.google.com/p/go #MessagesTotal messages: 28
This adds, at least implicitly, dependency on another tool to maintain the test. Can you not generate the test file in the test? All you need to do is pickle a header for some size and then dynamically create a bytes.Buffer with the wrong pixel size and point the decoder to it.
Sign in to reply to this message.
Sure thing, will put the gif inside of reader_test.go, but not until Tuesday.
Sign in to reply to this message.
I like this approach a lot more. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go File src/pkg/image/gif/reader.go (right): https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:352: width = d.width capping it to d.width instead of returning an error? won't it just return an error later anyway when it tries to read the pixel data?
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go File src/pkg/image/gif/reader.go (right): https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:352: width = d.width Browsers take a liberal view of GIFs and try to decode what can be decoded safely and ignore what can't. I think Go users expect it to do the same. When setting m.Pix below, I check to see if precisely the right amount of pixel data arrived. If not, I leave the frame set to all zeros and throw away the pixel data from the file that does not match the bounds.
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go File src/pkg/image/gif/reader.go (right): https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:185: if pix, err = ioutil.ReadAll(lzwr); err != nil { Revert this change from io.ReadFull to ioutil.ReadAll. ReadAll is less efficient if you already know the number of bytes to expect, and using ReadAll here means that the m.Pix created by image.NewPaletted is simply garbage to be collected. If you're concerned about the malloc requirements of large images, then that's a bigger issue, as issue 5050 discusses. But this does not fix issue 5050. Please drop that line from the CL description. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:339: func (d *decoder) boundsFromDescriptor() (image.Rectangle, error) { It seems to me that you can revert this function signature change, and return *image.Paletted instead of image.Rectangle. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:349: // Gif89: "Each image must fit within the // The GIF89a spec Section 20 (Image Descriptor) says: "Each image etc". https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:352: width = d.width On 2013/03/15 21:27:02, jeff.allen wrote: > Browsers take a liberal view of GIFs and try to decode what can be decoded > safely and ignore what can't. Can you point to where e.g. giflib does this clamp-to-bounds? https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:357: return image.Rect(left, top, left+width, top+height), nil Do you need to clamp left and top as well? Would it be better to use the image.Rectangle.Intersect method?
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader_tes... File src/pkg/image/gif/reader_test.go (right): https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader_tes... src/pkg/image/gif/reader_test.go:8: func TestDecodeGifError(t *testing.T) { s/Gif/GIF/, but even better is dropping GIF entirely. We're in package gif. It's redundant. func TestBoundsClamping(t *testing.T) { https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader_tes... src/pkg/image/gif/reader_test.go:9: r, err := os.Open("testdata/issue5050.gif") s/r/f/ is traditional. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader_tes... src/pkg/image/gif/reader_test.go:24: // on width/heigh of subsequent frames Typo in "height". Also, add a full stop to complete the sentence.
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go File src/pkg/image/gif/reader.go (right): https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:185: if pix, err = ioutil.ReadAll(lzwr); err != nil { On 2013/03/18 01:49:49, nigeltao wrote: > Revert this change from io.ReadFull to ioutil.ReadAll. ReadAll is less efficient > if you already know the number of bytes to expect, and using ReadAll here means > that the m.Pix created by image.NewPaletted is simply garbage to be collected. > > If you're concerned about the malloc requirements of large images, then that's a > bigger issue, as issue 5050 discusses. But this does not fix issue 5050. Please > drop that line from the CL description. Done. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:339: func (d *decoder) boundsFromDescriptor() (image.Rectangle, error) { On 2013/03/18 01:49:49, nigeltao wrote: > It seems to me that you can revert this function signature change, and return > *image.Paletted instead of image.Rectangle. Done. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:349: // Gif89: "Each image must fit within the On 2013/03/18 01:49:49, nigeltao wrote: > // The GIF89a spec Section 20 (Image Descriptor) says: "Each image etc". Done. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:352: width = d.width giflib does not clamp. For the gif in issue 5050, it tries to allocate 18446744073698482932 bytes, and malloc returns NULL. http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/g... shows how webkit does it. There are lots of workarounds for invalid width/height, but none do clamping. The image is read in row by row, and the pixel data is passed to the UI layer row by row, so the maximum exposure they have to an evil GIF is 2^16 = 65536 bytes. The UI layer may or may not protect itself some other way, dunno. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:357: return image.Rect(left, top, left+width, top+height), nil On 2013/03/18 01:49:49, nigeltao wrote: > Do you need to clamp left and top as well? Would it be better to use the > image.Rectangle.Intersect method? Done. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader_tes... File src/pkg/image/gif/reader_test.go (right): https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader_tes... src/pkg/image/gif/reader_test.go:8: func TestDecodeGifError(t *testing.T) { On 2013/03/18 01:53:12, nigeltao wrote: > s/Gif/GIF/, but even better is dropping GIF entirely. We're in package gif. It's > redundant. > > func TestBoundsClamping(t *testing.T) { Done. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader_tes... src/pkg/image/gif/reader_test.go:9: r, err := os.Open("testdata/issue5050.gif") On 2013/03/18 01:53:12, nigeltao wrote: > s/r/f/ is traditional. Done. https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader_tes... src/pkg/image/gif/reader_test.go:24: // on width/heigh of subsequent frames On 2013/03/18 01:53:12, nigeltao wrote: > Typo in "height". Also, add a full stop to complete the sentence. Done.
Sign in to reply to this message.
Hello nigeltao@golang.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com, r@golang.org), I'd like you to review this change to http://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... File src/pkg/image/gif/reader_test.go (right): https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:8: // a gif with two frames, where the second frame has been tweaked // bigGIF is a GIF image with two frames, etc. https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:9: // to have a width/height that's higher Higher than what? https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:10: var bigGif = []byte{ Go capitalization is s/Gif/GIF/. https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:27: 0x02, 0x04, 0x8c, 0x8f, 0x19, 0x05, 0x00, // lzw pixels In this case, the lzw-compressed form of the 16 pixels fit into a single block (it is less than 256 bytes). What if frame 2 was (0,0 - 4000,4000) and not very compressible? Does DecodeAll still do something reasonable? Similarly, this oversized frame is also the last frame in the GIF. What if there was a frame 3 after this? Does DecodeAll still do something reasonable? https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:36: t.Errorf("got error %v", err) Change Errorf to Fatalf. If DecodeAll fails, then there's no point continuing, and you'll probably crash otherwise.
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go File src/pkg/image/gif/reader.go (right): https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#... src/pkg/image/gif/reader.go:352: width = d.width On 2013/03/19 09:37:10, jeff.allen wrote: > giflib does not clamp. For the gif in issue 5050, it tries to allocate > 18446744073698482932 bytes, and malloc returns NULL. If neither giflib or WebKit clamp, then I'm not sure if Go should. For example, the eog program (which presumably uses giflib) on my Ubuntu desktop just shows an error on the GIF from issue 5050; it does not fall back on the first frame like WebKit does. The spec's "each image must fit" suggests that the GIF in issue 5050 is an invalid GIF, and I don't read it as a decoder must try to fix up an invalid image. Perhaps the right thing to do here is if bounds != bounds.Intersect(d.logicalScreen) { return nil, fmt.Errorf("gif: frame bounds is larger than image bounds") } instead of clamping and continuing without error.
Sign in to reply to this message.
I think this is now a philosophical question, but unlike most, the answer is clear. Should we be liberal, like a browser, or strict? The answer has always been clear for questions like this before. The std lib should do the simplest most standard thing. Users can always fork and hack images/* if their app needs to be more like a browser.
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... File src/pkg/image/gif/reader_test.go (right): https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:8: // a gif with two frames, where the second frame has been tweaked On 2013/03/20 02:57:45, nigeltao wrote: > // bigGIF is a GIF image with two frames, etc. Done. https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:9: // to have a width/height that's higher On 2013/03/20 02:57:45, nigeltao wrote: > Higher than what? Done. https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:10: var bigGif = []byte{ On 2013/03/20 02:57:45, nigeltao wrote: > Go capitalization is s/Gif/GIF/. Done. https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:27: 0x02, 0x04, 0x8c, 0x8f, 0x19, 0x05, 0x00, // lzw pixels The new tests (next CL update) now check for bounds > data size, bounds < data size. So I think your first question is covered. See if you agree. Bounds checking happens on a per-frame basis, in a loop that processes each section without respect to the previous or future sections, only with respect to the header. So I see no interest in testing other scenarios, like your second question. https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:36: t.Errorf("got error %v", err) On 2013/03/20 02:57:45, nigeltao wrote: > Change Errorf to Fatalf. If DecodeAll fails, then there's no point continuing, > and you'll probably crash otherwise. Done.
Sign in to reply to this message.
Hello nigeltao@golang.org, jra@nella.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com, r@golang.org), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... File src/pkg/image/gif/reader_test.go (right): https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:27: 0x02, 0x04, 0x8c, 0x8f, 0x19, 0x05, 0x00, // lzw pixels On 2013/03/20 10:23:06, jeff.allen wrote: > The new tests (next CL update) now check for bounds > data size, bounds < data > size. So I think your first question is covered. See if you agree. > > Bounds checking happens on a per-frame basis, in a loop that processes each > section without respect to the previous or future sections, only with respect to > the header. So I see no interest in testing other scenarios, like your second > question. The per-frame basis doesn't invalidate the question if you wanted to continue processing after encountering an invalid frame: unread blocks from frame F remain in the byte stream and would be mistakenly interpreted as part of frame F+1. But as you've changed it to error out early, I'm not so concerned. https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader.go File src/pkg/image/gif/reader.go (right): https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader.go... src/pkg/image/gif/reader.go:187: // try one more read to find out if we got it all Go comments should be complete sentences: start with a capital letter and end with a full stop. https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader.go... src/pkg/image/gif/reader.go:188: if _, err := lzwr.Read([]byte{}); err != io.EOF { A nil slice is equivalent to a []byte{}; they both have zero length. This could be lzwr.Read(nil). But a zero-length read isn't guaranteed to return io.EOF, even if there is no more data. Instead, I think you should read into a 1-byte buffer (say d.tmp[:1]) and expect to get back (0, EOF). This replaces the "There should be a "0" block remaining; drain that" check next, and you should also update the doc comment on type blockReader. https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader.go... src/pkg/image/gif/reader.go:349: if bounds != bounds.Intersect(d.logicalScreen) { Do we need a (redundant) d.logicalScreen field? This could just be if bounds != bounds.Intersect(image.Rect(0, 0, d.width, d.height)) { https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader_te... File src/pkg/image/gif/reader_test.go (right): https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:8: // A simple GIF that we can modify to test different scenarios. Go doc comments begin with the name of the thing being commented upon. Thus: // theGif is a simple GIF that etc. https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:15: // frame 1 (0,0 - 1,1) I would actually start counting from zero: frame 0 and frame 1, not frame 1 and frame 2. But I think you only need one frame. See below. https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:35: r := bytes.NewReader(b) Just inline r into the next line. https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:42: t.Fatalf("expected %v, got %v", exp, estr) The usual Go language (and order) is got and want, not expected and got. https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:50: // make the bounds too big, just by one Comments should be complete sentences. https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:51: theGIF[32] = 2 Offset 32 is the first frame. If you're expecting to error out in processing the first frame, then you don't actually need a second frame; removing it should simplify the test. Perhaps something like: theGIF is a valid 2x1 GIF with one 2x1 frame. Decoding it should return no error. Changing that frame's bounds to 1x1 should result in an error ("too much data"). Changing that frame's bounds to 3x1 should result in an error ("frame bounds bigger"). Keeping that frame's bounds as 2x1 but substituting only 1 pixel of LZW-compressed data should also result in an error (translate io.ErrUnexpectedEOF to "not enough data"). https://codereview.appspot.com/7602045/diff/32001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:57: // a huge memory allocation and associated problems Well, you're not really testing that a huge memory allocation was not made...
Sign in to reply to this message.
I've separated checking for too much / too little image data (compared to the declared image bounds) out as https://codereview.appspot.com/7938043 I figured it was easier to do the blockReader EOF changes myself than iterate over e-mail. This CL can then become just checking that each image's bounds is within the overall bounds.
Sign in to reply to this message.
Hello nigeltao@golang.org, jra@nella.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com, r@golang.org), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/38001/src/pkg/image/gif/reader.go File src/pkg/image/gif/reader.go (right): https://codereview.appspot.com/7602045/diff/38001/src/pkg/image/gif/reader.go... src/pkg/image/gif/reader.go:343: return nil, errors.New("gif: frame bounds is larger than image bounds") s/is/are/ or just s/is //
Sign in to reply to this message.
Please update the CL description to image/gif: reject a GIF image if frame bounds larger than image bounds. You'll also need to update reader_test.go when you "hg sync".
Sign in to reply to this message.
Hello nigeltao@golang.org, jra@nella.org, r@golang.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7602045/diff/45001/src/pkg/image/gif/reader_te... File src/pkg/image/gif/reader_test.go (right): https://codereview.appspot.com/7602045/diff/45001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:88: // theGIF is a simple GIF that we can modify to test different scenarios. the 'the' is not idiomatic in standard tests. how about 'testGIF'? https://codereview.appspot.com/7602045/diff/45001/src/pkg/image/gif/reader_te... src/pkg/image/gif/reader_test.go:107: var estr string s/estr/got/
Sign in to reply to this message.
Hello nigeltao@golang.org, jra@nella.org, r@golang.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM thanks
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=7f837c455456 *** image/gif: reject a GIF image if frame bounds larger than image bounds The GIF89a spec says: "Each image must fit within the boundaries of the Logical Screen, as defined in the Logical Screen Descriptor." Also, do not accept GIFs which have too much data for the image size. R=nigeltao, jra, r CC=bradfitz, golang-dev https://codereview.appspot.com/7602045 Committer: Rob Pike <r@golang.org>
Sign in to reply to this message.
On 2013/03/22 16:30:40, r wrote: > *** Submitted as https://code.google.com/p/go/source/detail?r=7f837c455456 *** > image/gif: reject a GIF image if frame bounds larger than image bounds TestBounds doesn't like multiple -cpu values: === RUN TestDecode --- PASS: TestDecode (0.00 seconds) === RUN TestBounds --- PASS: TestBounds (0.00 seconds) === RUN TestDecode-2 --- PASS: TestDecode-2 (0.00 seconds) === RUN TestBounds-2 --- FAIL: TestBounds-2 (0.00 seconds) reader_test.go:112: got gif: frame bounds larger than image bounds, want gif: too much image data === RUN TestDecode-4 --- PASS: TestDecode-4 (0.00 seconds) === RUN TestBounds-4 --- FAIL: TestBounds-4 (0.00 seconds) reader_test.go:112: got gif: frame bounds larger than image bounds, want gif: too much image data FAIL FAIL image/gif 0.029s
Sign in to reply to this message.
What is this "multi threading" you're talking about and what's the big deal about it anyway? :) Will post a CL tomorrow to fix this, sorry. (It should be as simple as moving testGIF into local scope.)
Sign in to reply to this message.
Hello On 2013/03/25 07:32:38, jra wrote: > What is this "multi threading" you're talking about and what's the big deal > about it anyway? :) At some point in the past there was a discussion with Dmitry which lead to me the conclusion that I should include the following in my stress tests: go test -v -short -cpu 1,2,4 std go test -v -cpu 1,2,4 std These tend to turn up tests that weren't written to run more than once in the same process. I think there's value in this, but given that two separate packages tripped over this in the last few days, it seems like we need to do something to help people realise sooner when they break this test mode. That's probably a discussion for a separate thread. Cheers Albert
Sign in to reply to this message.
https://codereview.appspot.com/7808045 PTAL. On Mon, Mar 25, 2013 at 6:38 PM, <fullung@gmail.com> wrote: > Hello > > > On 2013/03/25 07:32:38, jra wrote: >> >> What is this "multi threading" you're talking about and what's the big > > deal >> >> about it anyway? :) > > > At some point in the past there was a discussion with Dmitry which lead > to me the conclusion that I should include the following in my stress > tests: > > go test -v -short -cpu 1,2,4 std > go test -v -cpu 1,2,4 std > > These tend to turn up tests that weren't written to run more than once > in the same process. > > I think there's value in this, but given that two separate packages > tripped over this in the last few days, it seems like we need to do > something to help people realise sooner when they break this test mode. > That's probably a discussion for a separate thread. > > Cheers > > Albert > > https://codereview.appspot.com/7602045/ > > -- > > ---You received this message because you are subscribed to the Google Groups > "golang-dev" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > >
Sign in to reply to this message.
Albert, I would be grateful if you would lead off the discussion about testing with multiple cpu values. I see this as a question of test repeatably, ie, go test -cpu=1,1,1. I'm sure we have some gaps in the std library. Cheers Dave On Mon, Mar 25, 2013 at 6:38 PM, <fullung@gmail.com> wrote: > Hello > > > On 2013/03/25 07:32:38, jra wrote: >> >> What is this "multi threading" you're talking about and what's the big > > deal >> >> about it anyway? :) > > > At some point in the past there was a discussion with Dmitry which lead > to me the conclusion that I should include the following in my stress > tests: > > go test -v -short -cpu 1,2,4 std > go test -v -cpu 1,2,4 std > > These tend to turn up tests that weren't written to run more than once > in the same process. > > I think there's value in this, but given that two separate packages > tripped over this in the last few days, it seems like we need to do > something to help people realise sooner when they break this test mode. > That's probably a discussion for a separate thread. > > Cheers > > Albert > > https://codereview.appspot.com/7602045/ > > -- > > ---You received this message because you are subscribed to the Google Groups > "golang-dev" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > >
Sign in to reply to this message.
|
