Folks: Granted, this slipped past my review too, but do *not* submit patches that have not been tested. This bug was a no brainer; running the code only even once would have found it. The mainline of Vorbis is our face to the world. A simple mistake like this costs us credibility when we're already going to be fighting hard against some of the most powerful media organizations in the world. We're in generally excellent shape on the code quality front-- I want us to stay that way. I'm not pointing any fingers: I just want all of us to be properly careful. Testing new code is not suggested, it's required. Monty ------- Begin Forwarded Message [...] From: "Mike Nordell" <tamlin@algonet.se> To: <webmaster@xiph.org> Subject: ov_read bugfix Date: Sat, 13 May 2000 12:18:48 +0100 Sorry for possibly posting to the wrong address. In vorbisfile.c, ov_read around line 1045 there's short *dest; that's redefined inside the loop. The statement buffer=(char *)dest; at around line 1057 and 1071 doesn't initialize buffer with anything sensible, so SEGV and sorrow is likely to follow. :-) I fixed it by just removing the (re) definition of 'dest' from within the loops, though I don't know if it's the correct fix. /Mike ------- End of Forwarded Message --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/
> Granted, this slipped past my review too, but do *not* submit patches that > have not been tested. This bug was a no brainer; running the code only even > once would have found it.(BTW, fairness requires mention that the erroneous code is originally *mine*; I didn't include it in the new codebase, but it is my code taken more or less in-tact from Stormbringer where it was also apparently broken.) Testing still gotta happen when new code goes in. Monty --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/
Oh, whoop, take it back... that code isn't mine. * waves hands frantically to dispell the tangent confusion * Anyway, the point about testing still holds :-) Monty --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/
Monty wrote:> > Folks: > > Granted, this slipped past my review too, but do *not* submit patches that > have not been tested. This bug was a no brainer; running the code only even > once would have found it.Hmm, it looks like this was my bug. It's clearly wrong. But, I tested it, and it "seemed to work fine"... I was able to play back 5-minute-long songs. I hate computers. -Jonathan. --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/
At 03:09 PM 5/15/00 -0700, you wrote:> >Folks: > >Granted, this slipped past my review too, but do *not* submit patches that >have not been tested. This bug was a no brainer; running the code only even >once would have found it.I noticed this bug a couple of days ago, but forgot to commit a fix - it CANNOT cause a crash (it just does a trivial amount of useless redundant work, and is unclear). The code definately works, but I'll commit the correct fix (which is the removal of 4 lines in ov_read() tonight.>I'm not pointing any fingers: I just want all of us to be properly careful. >Testing new code is not suggested, it's required.I suspected this had been tested - just not looked at very carefully ;) Michael --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/