[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