[FFmpeg-devel] av_strlcpy() size parameter signedness

Ivan Kalvachev ikalvachev
Sun Sep 30 12:22:33 CEST 2007


2007/9/29, Michael Niedermayer <michaelni at gmx.at>:
> Hi
>
> On Fri, Sep 28, 2007 at 09:27:02PM -0400, Rich Felker wrote:
> > On Sat, Sep 29, 2007 at 01:37:48AM +0200, Michael Niedermayer wrote:
> > > Hi
> > >
> > > currently the buffer size parameter for av_strlcpy() (and friends)
> > > is unsigned this makes sense and is logic but it has a flaw
> > > that is if a negative value is mistakely used something very bad happens
> > >
> > > how can a negative value be assigned?
> > >
> > > av_strlcpy(... FFMIN(buf_size, something))
> > > with something being <0 and buf_size signed
> >
> > This sounds like a bogus construct.
>
> what i had in mind was:
> av_strlcpy(buf, p, FFMIN(buf_size, strchr(p, ':') - p + 1));
> code like that was in the recently approved IPv6 patch
>
> and yes i agree buf_size should be changed to unsigend ...
>
>
>
> > We should find out why values that
> > can be negative are being used in sizes like this. A hack to make the
> > problem go away is not appropriate; it's just covering up the
> > underlying bug.
>
> sure should the underlaying bug be fixed but
> the underlaying bug is likely harmless compared to what av_strlcpy will do
> due to it, and we can only fix bugs which have been found ...
>
> also your argument could be used against the existence of strlcpy itself
> why not consider it a bug if a string is longer than the buffer?
> what the function does is trying to prevent buffer overflows at the
> expense of possibly hiding bugs ...
>
> really i do prefer a <0 size bug to be hidden instead of having a
> buffer ovreflow, also we could just call abort() ar just dereference NULL
> instead of using 0 that would not cover the bug up but still prevent the
> overflow

This is classical example where assert() should be used.
I do not mind placing permanent abort().

I am also against making functions recover from illegal input,
Microsoft have the habit of writing "protected" functions that would
handle even changes caused by cosmic rays. As result their code works
but is full of hidden bugs and they usually "fix" strange conditions
instead of hunting for the reason. (e.g. network throttling down when
sound is playing).

If we make strlcpy return NULL on negative size, then somebody will
see the implementation and will try to use it as it is valid.
In the given code we get negative result because -p is negative,
however if size_t is 32 bit and the pointer p is in the upper 2GB,
then "size" would be positive and the overflow would still happen.

I agree that strlcpy is function that is meant to hide bugs, I do
prefer using strdup() or strndup() /*gnu*/ just like I think
asprintf() /*gnu*/ should be used a lot more. As they are gnu
extensions we may need our own implementation, Rich already gave one
for vasprintf() :)




More information about the ffmpeg-devel mailing list