[FFmpeg-devel] [PATCH] Improve documentation for libavutil/base64.h

[Stefano Sabatini]

On date Sunday 2009-01-25 22:51:53 +0100, Michael Niedermayer encoded:
> On Sun, Jan 25, 2009 at 10:37:07PM +0100, Stefano Sabatini wrote:
> > On date Sunday 2009-01-25 18:10:05 +0100, Michael Niedermayer encoded:
> > > On Sun, Jan 25, 2009 at 02:04:52AM +0100, Stefano Sabatini wrote:
> > [...]
> > > > Can you confirm that the attached patch is correct? (At least I can
> > > > confirm that it works with the test program.)
> > > 
> > > I can confirm that
> > > * it could lead to a sechole if you missed something and i suspect you did
> > > * its more complex
> > > * you did not list any advantage of this change
> > 
> > Use the minimal size data required. But I'll check better for the
> > first point.
> > 
> > [...]
> > > >  /**
> > > > - * encodes base64
> > > > - * @param src data, not a string
> > > > - * @param buf output string
> > > > + * Encodes in base-64 the data in \p src and puts the resulting string
> > > > + * in \p dst.
> > > > + *
> > > > + * @param dst_len lenght of the \p dst string, it has to be at least
> > > > + * 4/3 * N, where N is the smaller multiple of 3 greater than or equal
> > > > + * to \p src_size.
> > > 
> > > this is even worse, you AGAIN document the current implementation limit
> > > but now its much more complex and even exploitable
> > 
> > Honestly, I cannot see your point here.
> > 
> > We have a function which fails if the buffer where to write cannot
> > contain the data to write into it.
> > 
> > We know which is the size of this data, so we have two choices:
> > 
> > 1) we can fail if the data size provided by the user is not big
> >    enough. In this case the documentation should give some hints
> >    regarding the size of the buffer to be provided, ideally this
> >    should be the minimal required, and this value shouldn't depend on
> >    the implementation, for this reason it looks safe to mention it in
> >    the docs.
> > 
> > 2) we can write in the buffer for its whole size, avoiding to go
> >    beyond its size. The problem with this is that the user isn't able
> >    to detect the failure, that's not a big problem since he can
> >    compute the required size by its own.
> > 
> > Problem with the actual implementation is that it requires an
> > (apparently, but maybe I'm wrong) arbitrary data size of len * 4 / 3 +
> > 12, so an user providing a buffer with the minimal required size will
> > fail.
> 
> Well my point is the user should neither depend on the implementation nor
> should he need precisse knowledge of base64 coding, and a len%3+1-len/123+98
> is a little much for just avoiding 1 or 2 bytes overallocation
> 
> anyway my wild guess is that (len+2)/3*4+1 might be minimal

floor (4/3 * (L + 2)) = floor (4/3 * (L'  + R + 2)) = 
                      = floor (4/3 * (L'' + R')     = 
                      = floor (4/3 * L'' + floor( 4/3 * R') =
                      = 4/3 * L'' + R'

L' being the smaller multiple of 3 not greater than L, R = L - L',
L'' being L' + 3, R' = L' + R + 2 - L''

in the worst case L' + R = L'', R' = 2, so we have two bytes
overallocated, which is better than 12.

Also you can demonstrate in this case that 4/3 * L + 2 = 4/3 * (L + 2).
 
> > But I'll check better the code and the spec, in the case I didn't
> > considered some special case.
> 
> in your last attempt you at least seem to have missed the 0 byte at the end

No I was considering len as a string length (in the sense of strlen())
rather than a buffer size.

I hope it is be more clear in the patch below.

Regards.
-- 
FFmpeg = Forgiving and Forgiving Minimalistic Picky Extended Game
-------------- next part --------------
A non-text attachment was scrubbed...
Name: doxy-for-base64-01.patch
Type: text/x-diff
Size: 1588 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090127/c11fe566/attachment.patch>