[FFmpeg-devel] [PATCH] Improve documentation for libavutil/base64.h
Stefano Sabatini
stefano.sabatini-lala
Sun Feb 8 19:12:06 CET 2009
On date Sunday 2009-02-08 18:50:11 +0100, Michael Niedermayer encoded:
> 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 ...
Gosh!
New patch attached, yes I'll fix the other patches as well, OK to
apply?
Regards.
--
FFmpeg = Foolish Fierce Multipurpose Programmable Everlasting Guru
-------------- next part --------------
A non-text attachment was scrubbed...
Name: base64-relax-constr.patch
Type: text/x-diff
Size: 474 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090208/47e86976/attachment.patch>
More information about the ffmpeg-devel
mailing list