Hey all,
If my understanding is right, there's a serious big in vorbisfile.c, in
the routine _fetch_headers(), which will only show up when comment
packet spans multiple pages. The code to read the first 3 Vorbis
packets ogg_stream_pagein() once, then calls ogg_stream_packetout().
The problem is that ogg_stream_pagein() only adds a single page to the
ogg stream state, whereas ogg_stream_packetout(), loops until it finds a
non-255 lacing value. So, if the comment packet is larger than one
page, all lacing values will be 255, and ogg_stream_packetout will start
reading uninitialized memory.
The relevant part of _fetch_headers() is here:
i=0;
while(i<3){
ogg_stream_pagein(&vf->os,og_ptr);
while(i<3){
int result=ogg_stream_packetout(&vf->os,&op);
if(result==0)break;
if(result==-1){
ret=OV_EBADHEADER;
goto bail_header;
}
if((ret=vorbis_synthesis_headerin(vi,vc,&op))){
goto bail_header;
}
i++;
}
if(i<3)
if(_get_next_page(vf,og_ptr,1)<0){
ret=OV_EBADHEADER;
goto bail_header;
}
}
return 0;
Also, notice that ogg_stream_packetout() is called in a loop, but no
reading can happen during the loop. And ogg_stream_packetout() never
returns zero, it returns -1 on error and +1 if everything goes ok.
Which means either this code is confused, or I am. Which one is it?
Thanks,
Martin
--- >8 ----
List archives: http://www.xiph.org/archives/
Ogg project homepage: http://www.xiph.org/ogg/
To unsubscribe from this list, send a message to
'vorbis-dev-request@xiph.org'
containing only the word 'unsubscribe' in the body. No subject is
needed.
Unsubscribe messages sent to the list will be ignored/filtered.
On Sun, Feb 04, 2001 at 05:52:05PM -0500, Martin C. Martin wrote:> Hey all, > > If my understanding is right, there's a serious big in vorbisfile.c, in > the routine _fetch_headers(), which will only show up when comment > packet spans multiple pages. The code to read the first 3 Vorbis > packets ogg_stream_pagein() once, then calls ogg_stream_packetout(). > The problem is that ogg_stream_pagein() only adds a single page to the > ogg stream state, whereas ogg_stream_packetout(), loops until it finds a > non-255 lacing value. So, if the comment packet is larger than one > page, all lacing values will be 255, and ogg_stream_packetout will start > reading uninitialized memory.No, you've misunderstood how the buffering works. Packetout says "need more data" if the comment packet is > 1 page, the inner loop breaks, a new page is read and then packetout is called again. That continues for as long as necessary to get a complete packet. Monty --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/ To unsubscribe from this list, send a message to 'vorbis-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.
> > The relevant part of _fetch_headers() is here: > > i=0; > while(i<3){ > ogg_stream_pagein(&vf->os,og_ptr); > while(i<3){ > int result=ogg_stream_packetout(&vf->os,&op); > if(result==0)break; > if(result==-1){ > ret=OV_EBADHEADER; > goto bail_header; > } > if((ret=vorbis_synthesis_headerin(vi,vc,&op))){ > goto bail_header; > } > i++; > } > if(i<3) > if(_get_next_page(vf,og_ptr,1)<0){ > ret=OV_EBADHEADER; > goto bail_header; > } > } > return 0; > > Also, notice that ogg_stream_packetout() is called in a loop, but no > reading can happen during the loop. And ogg_stream_packetout() never > returns zero, it returns -1 on error and +1 if everything goes ok. > Which means either this code is confused, or I am. Which one is it? >You. Sorry ;-) That code loops over the entire thing until all the headers have been found (if this were buggy it would be pretty obvious - the third packet (codebooks) is frequently over 4k (which is what libogg currently chooses as the max page size). In each loop iteration, a page is streamed in (ogg_stream_pagein()). Then, any number of completed packets are streamed out (ogg_stream_packetout()) - note that ogg_stream_packetout() will only return completed packets, thus avoiding the problem you thought was there. The ogg_stream_packetout() returns 0 if there is insufficient information (i.e. there aren't any pending packets), thus breaking from the inner loop. If it was successful, it submits the header packet for processing. It then gets the next page, if it hasn't yet completed getting the headers, and loops around. Michael --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/ To unsubscribe from this list, send a message to 'vorbis-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.