On 06/02/2008, ogg.k.ogg.k@googlemail.com <ogg.k.ogg.k@googlemail.com> wrote:> I've had a second look, and I believe there really was a bug there, > though my patch may not be optimal. > > As an example of an off by one bug: > > On the first run through the code, message_header_fields will be NULL, > so _ogg_calloc will be called. Assume header_key and header_value > are both "X", so strlen of each is 1. message_size will then by 6, and a > block of 6 bytes is callocated. > Then snprintf is called with a byte limit of message_size+1 (7, one more > than the allocated size), with the string "X: X\r\n", 6 characters and a > terminating NULL. 7 bytes will be written, overwrite.yup, you're right. The (this_message_size+1) argument to snprintf, after only allocating (this_message_size) chars, was asking for trouble. However if we simply truncate to the correct length then the final LF is replaced with NUL by snprintf. So we need to allocate room for one extra char (the NUL) as you have done, but then not truncate the write: so we both allocate and print with length (this_message_size+1). Checked with valgrind, and committed in oggenc r14485 and liboggz r3448. cheers, Conrad.
> However if we simply truncate to the correct length then the final LF > is replaced with NUL by snprintf. So we need to allocate room for one > extra char (the NUL) as you have done, but then not truncate the > write: so we both allocate and print with length > (this_message_size+1).Ah ah, got me nicely there :) Thanks
On 2/12/08, Conrad Parker <conrad@metadecks.org> wrote:> Checked with valgrind, and committed in oggenc r14485 and liboggz r3448.Can you commit it in ffmpeg2theora? I think that and Speex are the only other places where skeleton.c is used, and ffmpeg2theora is likely to have Skeleton being used by default in the next release. -Ivo
> Can you commit it in ffmpeg2theora? I think that and Speex are the > only other places where skeleton.c is used, and ffmpeg2theora is > likely to have Skeleton being used by default in the next release.Then maybe it would be a good idea to do the API change needed for checking magic now ?
On 13/02/2008, Ivo Emanuel Gon?alves <justivo@gmail.com> wrote:> On 2/12/08, Conrad Parker <conrad@metadecks.org> wrote: > > Checked with valgrind, and committed in oggenc r14485 and liboggz r3448. > > Can you commit it in ffmpeg2theora? I think that and Speex are the > only other places where skeleton.c is used, and ffmpeg2theora is > likely to have Skeleton being used by default in the next release.it seems ffmpeg2theora doesn't have that function (add_message_header_field()), it just hardcodes the message headers appropriate for the streams it encodes. No change needed there. cheers, Conrad.