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

Sun Feb 8 13:10:56 CET 2009

On Sun, Feb 08, 2009 at 11:31:48AM +0100, Stefano Sabatini wrote:
> On date Sunday 2009-02-08 02:36:57 +0100, Michael Niedermayer encoded:
> > On Sat, Feb 07, 2009 at 06:02:49PM +0100, Stefano Sabatini wrote:
> > > On date Saturday 2009-02-07 03:11:45 +0100, Michael Niedermayer encoded:
> > > > On Sat, Feb 07, 2009 at 01:27:15AM +0100, Stefano Sabatini wrote:
> > > > > On date Wednesday 2009-01-28 01:03:29 +0100, Stefano Sabatini encoded:
> > > > > [...]
> > > > > > Index: libavutil/base64.h
> > > > > > ===================================================================
> > > > > > --- libavutil/base64.h	(revision 16838)
> > > > > > +++ libavutil/base64.h	(working copy)
> > > > > > @@ -25,16 +25,29 @@
> > > > > >  #include <stdint.h>
> > > > > >  
> > > > > >  /**
> > > > > > - * decodes base64
> > > > > > - * param order as strncpy()
> > > > > > + * Decodes the base64-encoded string in \p src and puts the decoded
> > > > > > + * data in \p dst.
> > > > > > + *
> > > > > > + * @param dst_size size in bytes of the \p dst buffer, it should be at
> > > > > > + * least 3/4 of the length of \p src
> > > > > > + * @return the number of bytes written, or a negative value in case of
> > > > > > + * error
> > > > > >   */
> > > > > > -int av_base64_decode(uint8_t * out, const char *in, int out_length);
> > > > > > +int av_base64_decode(uint8_t *dst, const char *src, int dst_size);
> > > > > >  
> > > > > >  /**
> > > > > > - * encodes base64
> > > > > > - * @param src data, not a string
> > > > > > - * @param buf output string
> > > > > > + * Encodes in base64 the data in \p src and puts the resulting string
> > > > > > + * in \p dst.
> > > > > > + *
> > > > > > + * @param dst_size size in bytes of the \p dst string
> > > > > > + * @warning While the encoded string size is 4/3 * N + 1, N being the
> > > > > > + * smaller multiple of 3 greater than or equal to \p src_size, you may
> > > > > > + * need to overallocate by few bytes the \p dst buffer, or the
> > > > > > + * function may fail.
> > > > > > + * @param src_size size in bytes of the \p src buffer
> > > > > > + * @return the string containing the encoded data, or NULL in case of
> > > > > > + * error
> > > > > >   */
> > > > > > -char *av_base64_encode(char * buf, int buf_len, const uint8_t * src, int len);
> > > > > > +char *av_base64_encode(char *dst, int dst_size, const uint8_t *src, int src_size);
> > > > > >  
> > > > > >  #endif /* AVUTIL_BASE64_H */
> > > > > 
> > > > > Ping?
> > > > 
> > > > rejected :)
> > > > "While the encoded string size is 4/3 * N + 1, N being the
> > > > smaller multiple of 3 greater than or equal to \p src_size"
> > > > 
> > > > that be confusing
> > > > i dont remember exactly but possibly i suggested a clean variant already
> > > > somewhere in the thread but then maybe not ...
> > > 
> > > Well it was my fault to erroneosly infer that that variant was
> > > correct, it overallocates two bytes in the worst case.
> > > 
> > > Your variant: f(N) = floor(4 * (N+2) / 3)
> > 
> > my variant is
> > (len+2)/3*4+1
> > with c semantics of operations
> > 
> > if you want to proof it wrong the way to go is to provide a sequence
> > of bytes for which it is wrong not some obscure math that seems to
> > assert from the start that your claim is correct and that doesnt
> > even translate my variant correctly into your pseudo math
> > 
> > hint:
> > floor(4 * (3+2) / 3) = 6    (assuming real valued divide)
> >            (3+2)/3*4 = 4    (assuming integer round down divide)
> 
> Thanks for your patience, just a little effort and we'll be finally
> done with this.
> 
> Since we can express the constraint with C semantic in the base64.c
> code, then I think there is no point into keeping the warning notice,
> please check again.
> 
> Last patch makes the implementation params consistent with those in
> the declaration, also I think it improves readability, but I won't
> push on it if you think is overkill.
> 
> Regards.
> -- 
> FFmpeg = Fundamental and Fast Mournful Pitiful Experimenting Gem

> Index: libavutil/base64.c
> ===================================================================
> --- libavutil/base64.c	(revision 17046)
> +++ libavutil/base64.c	(working copy)
> @@ -79,7 +79,7 @@
>      int bytes_remaining = len;
>  
>      if (len >= UINT_MAX / 4 ||
> -        buf_len < len * 4 / 3 + 12)
> +        buf_len < (len+2) / 3 * 4 + 1)
>          return NULL;
>      ret = dst = buf;
>      while (bytes_remaining) {

iam still waiting for a proof that it is large enough, and yes i do not
consider "the ffmpeg leader said its likely ok a valid proof", this is
security relevant and the 10 bytes this safes on one hand is vs. possibly
thousands of users having their system hacked if we did miss something.

just write a 2 line loop that check sizes from 0..X and checks that the
last write was inside the buffer.

> Index: libavutil/base64.h
> ===================================================================
> --- libavutil/base64.h	(revision 17046)
> +++ libavutil/base64.h	(working copy)
> @@ -24,16 +24,26 @@
>  #include <stdint.h>
>  
>  /**
> - * Decodes Base64.
> - * Parameter order is the same as strncpy().
> + * Decodes the base64-encoded string in \p src and puts the decoded
> + * data in \p dst.
> + *
> + * @param dst_size size in bytes of the \p dst buffer, it should be at
> + * least 3/4 of the length of \p src
> + * @return the number of bytes written, or a negative value in case of
> + * error
>   */
> -int av_base64_decode(uint8_t * out, const char *in, int out_length);
> +int av_base64_decode(uint8_t *dst, const char *src, int dst_size);

addition of doxy, ad renamig params should be in seperate patches.

[...]

> Index: ffmpeg/libavutil/base64.c
> ===================================================================
> --- ffmpeg.orig/libavutil/base64.c	2009-02-08 11:11:29.000000000 +0100
> +++ ffmpeg/libavutil/base64.c	2009-02-08 11:18:30.000000000 +0100
> @@ -42,25 +42,25 @@
>      0x2c, 0x2d, 0x2e, 0x2f, 0x30, 0x31, 0x32, 0x33
>  };
>  
> -int av_base64_decode(uint8_t * out, const char *in, int out_length)
> +int av_base64_decode(uint8_t *dst, const char *src, int dst_size)

i dont see the point of this rename.

[...]
> -char *av_base64_encode(char * buf, int buf_len, const uint8_t * src, int len)
> +char *av_base64_encode(char *dst, int dst_size, const uint8_t *src, int src_size)
>  {

len vs size
buf vs dst vs out
len vs src len
are seperate changes

iam in favor of adding the src to len to make it clearer
iam also in favor of using in/out or if you prefer b64/bin names
i prefer in/out because its shorter and consistent to the other function

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090208/ce7645dd/attachment.pgp>