On Tue, Sep 16, 2014 at 12:30 AM, Diego Biurrun <[email protected]> wrote:
> apetag: Improve APE tag size check
>
> On Tue, Sep 16, 2014 at 01:40:24AM +0200, Katerina Barone-Adesi wrote:
> > The size variable is (correctly) unsigned, but is passed to several
> functions
> > which take signed parameters, such as avio_read, sometimes after having
> > numbers added to it. This patch prevents unwanted implementation-defined
> > behaviour, while retaining the idea behind the original bounds check.
>
> > diff --git a/libavformat/apetag.c b/libavformat/apetag.c
> > index 22884ef..bd8d0ed 100644
> > --- a/libavformat/apetag.c
> > +++ b/libavformat/apetag.c
> > @@ -57,8 +57,10 @@ static int ape_tag_read_field(AVFormatContext *s)
> > av_log(s, AV_LOG_WARNING, "Invalid APE tag key '%s'.\n", key);
> > return -1;
> > }
> > - if (size >= UINT_MAX)
> > - return -1;
> > + if (size > INT32_MAX - FF_INPUT_BUFFER_PADDING_SIZE) {
> > + av_log(s, AV_LOG_ERROR, "APE tag size too large.\n");
> > + return AVERROR_INVALIDDATA;
> > + }
>
> I don't see much that is implementation-defined. size is a uint32_t
> variable, so UINT_MAX can be considerably larger on 64-bit systems.
> Even on 32-bit systems, unsigned types can always hold larger positive
> values than signed types.
>
size is uint32_t.
It is used as a parameter like this: int avio_read(AVIOContext *s, unsigned
char *buf, int size);
This makes the maximum acceptable value INT_MAX.
In the spirit of having the maximum input range the same across platforms,
INT32_MAX is used instead of INT_MAX. FF_INPUT_BUFFER_PADDING_SIZE is
subtracted due to code like this:
av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE);
I do not understand your objection to this code, Diego. Clearly comparing
against UINT_MAX is incorrect, and this patch is strictly better than
status quo.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
