On 01/02/2008, Conrad Parker <conrad@metadecks.org> wrote:> On 31/01/2008, ogg.k.ogg.k@googlemail.com <ogg.k.ogg.k@googlemail.com> wrote: > > This fixes an off by one bug in the user of snprintf, and returns > > negative if writing the > > header returns negative (otherwise we'd just get a short write, losing > > the error). > > This patch isn't tested though, but I compiled it :) > > ok, I'll test it out :-)I've checked the patch. The check for fwrite returning negative is ok, but i think the off-by-one check for snprintf is not necessary -- the intention is not to write a trailing NUL as this function is used to incrementally add lines of text to a buffer. Adapted in changeset 3396: http://trac.annodex.net/changeset/3396> > Also, I've not fixed this (yet ?) but I believe the filling of the ogg > > packets is bogus as it > > will fill it with values in whatever endianness the current host has. > > It should be using > > oggpack_write and friends instead, I think. I may be missing something though. > > eww, you're right, it's broken on big-endian hosts. I'll follow up > with a patch unless someone else does first ...I've written up a patch for this, applied to liboggz/src/tools/skeleton.c: http://trac.annodex.net/changeset/3395 Thank you very much for noticing the problems :-) Conrad.
> but i think the off-by-one check for snprintf is not necessary -- the > intention is not to write a trailing NUL as this function is used to > incrementally add lines of text to a buffer.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. Or am I missing a trick here ?
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.