[FFmpeg-devel] [PATCH] Improve documentation for libavutil/base64.h
Michael Niedermayer
michaelni
Sun Feb 8 20:54:11 CET 2009
On Sun, Feb 08, 2009 at 07:12:06PM +0100, Stefano Sabatini wrote:
> 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?
ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/00d82926/attachment.pgp>
More information about the ffmpeg-devel
mailing list