[FFmpeg-devel] [PATCH] Improve documentation for libavutil/base64.h
Sun Jan 25 18:10:05 CET 2009
On Sun, Jan 25, 2009 at 02:04:52AM +0100, Stefano Sabatini wrote:
> On date Sunday 2009-01-25 00:27:23 +0100, Michael Niedermayer encoded:
> > On Sat, Jan 24, 2009 at 09:26:43PM +0100, Stefano Sabatini wrote:
> > > Hi,
> > >
> > > in the patch I'm also changing the parameter names, I think they're
> > > more meaningful now, also I'm applying the distinction between size
> > > (size in bytes of a data buffer) and lenght, intended as the number of
> > > characters in a string *but* the last one, which looks consistent with
> > > the terminology of the C language (sizeof(...), strlen(...)).
> > >
> > > Regards.
> > > --
> > > FFmpeg = Fast and Fantastic Minimal Philosofic Entertaining God
> > > Index: libavutil/base64.h
> > > ===================================================================
> > > --- libavutil/base64.h (revision 16736)
> > > +++ libavutil/base64.h (working copy)
> > > @@ -25,16 +25,26 @@
> > > #include <stdint.h>
> > >
> > > /**
> > > - * decodes base64
> > > - * param order as strncpy()
> > > + * Decode the base-64 string in \p src and put the decoded data in \p
> > > + * dst.
> > av_base64_decode() decodes the base-64 string in ...
> > not
> > av_base64_decode() decode the base-64 string in ...
> I agree, and this had been already discussed mmhh.. almost two years
> ago and we defined to stick to the Javadoc recommendations, which in
> this specific case recommends the use of the third person forms,
> most libav* codes uses the impersonal form.
svn blame && flame :)
> Diego could you say which form do you prefer once and for all? Also
> setting it in the patch rules may help to keep consistency and avoid
> boring discussions.
> > > + *
> > > + * @param dst_size size in bytes of the \p dst buffer, it has to 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
> > > + * Encode in base-64 the data in \p src and put the resulting string
> > > + * in \p dst.
> > > + *
> > > + * @param dst_len lenght of the \p dst string, it has to be at least
> > > + * \p src_size * 4 / 3 + 12.
> > From where do you have this number?
> > Is it due tp base64 or our implementation?
> I read it in the implementation, and that limitation didn't made
> sense to me.
> Can you confirm that the attached patch is correct? (At least I can
> confirm that it works with the test program.)
I can confirm that
* it could lead to a sechole if you missed something and i suspect you did
* its more complex
* you did not list any advantage of this change
> > API and implementation are 2 seperate things. The API should
> > document exactly what is needed for both sides to interact, the implementation
> > details should not be documented in any way in the public header.
> OK, just let me note that in this case the implementation details
> caused an unexpected failure.
so you silently change the API to enforce the limits of the implementation
> - * encodes base64
> - * @param src data, not a string
> - * @param buf output string
> + * Encodes in base-64 the data in \p src and puts the resulting string
> + * in \p dst.
> + *
> + * @param dst_len lenght of the \p dst string, it has to be at least
> + * 4/3 * N, where N is the smaller multiple of 3 greater than or equal
> + * to \p src_size.
this is even worse, you AGAIN document the current implementation limit
but now its much more complex and even exploitable
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel