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

Michael Niedermayer michaelni
Sun Feb 8 18:50:11 CET 2009


On Sun, Feb 08, 2009 at 05:19:50PM +0100, Stefano Sabatini wrote:
> On date Sunday 2009-02-08 13:10:56 +0100, Michael Niedermayer encoded:
> > On Sun, Feb 08, 2009 at 11:31:48AM +0100, Stefano Sabatini wrote:
> [...]
> > > 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.
> 
> Sure.
> 
> -------------------------8<--------------------------------------------
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> 
> #include <libavutil/base64.h>
> 
> #define MAX_BUF_SIZE 1024 * 1024
> 
> int main(void)
> {
>     char encoded[MAX_BUF_SIZE];
>     char data   [MAX_BUF_SIZE];
>     int i, data_size;
> 
>     for (data_size=0; data_size<100000; data_size++) {
>         int encoded_size = (data_size + 2) / 3 * 4 + 1;
>         if(!av_base64_encode(encoded, MAX_BUF_SIZE, data, data_size))
>             printf("failed!\n");
>         else
>             printf("%d\t%d\t%d\n", data_size, encoded_size, strlen(encoded) + 1);
>     }
> 
>     return 0;
> }
> -------------------------8<--------------------------------------------
> 
> the strlen of the encoded buffer results always equal to the value
> computed in encoded_size.

good and thanks

> 
> And now a math proof, for what are worth my demonstrations:
> L' = smallest multiple of 3 equal to or greater than L
> 
> L' = floor((L+2)/3) * 3
> 
> So we have:
> 4/3 * L' = 4/3 * floor((L+2) / 3) * 3 =
>          = 4 * floor((L+2) / 3) * 3 / 3 =
>          = 4 * floor((L+2) / 3)
> 
> which translated in C semantics is:
> (L+2) / 3 * 4;
> 
> > > 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
> 
> New series attached:
> 
> * base64-relax-constr.patch
>   Relax constraints required for the output buffer size of av_base64_encode().

see comments inline


> 
> * base64-size-vs-len.patch
>   Consistently prefer size vs len/length, length is an ambiguous term
>   since it can be associated to the length of a string in the strlen()
>   sense.

ok


> 
> * base64-in-out-vs-src-buf.patch.
>   For consistency/readability sake, also out is more meaningful than
>   buf.

ok


> 
> * base64-in-size-vs-size.patch
>   Prefer in_size against size in av_base64_encode(), for readability reason.

ok


> 
> * base64-space-nits.patch.
>   Apply spacing nits.

ok


> 
> * base64-doxy.patch
>   Gloriously document the base64 interface.

ok


> 
> Regards.
> -- 
> FFmpeg = Foolish Fostering Moronic Puritan Esoteric Geek

> Index: ffmpeg/libavutil/base64.c
> ===================================================================
> --- ffmpeg.orig/libavutil/base64.c	2009-02-08 16:34:34.000000000 +0100
> +++ ffmpeg/libavutil/base64.c	2009-02-08 17:10:22.000000000 +0100
> @@ -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)
>          return NULL;
>      ret = dst = buf;
>      while (bytes_remaining) {

the 0 byte at the end of the string ...

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/7724d3f3/attachment.pgp>



More information about the ffmpeg-devel mailing list