[FFmpeg-devel] [FFmpeg-cvslog] avcodec/avpacket: Avoid unspecific return -1 for av_grow_packet()

[Michael Niedermayer]

On Sun, Jan 06, 2019 at 10:43:53AM -0300, James Almer wrote:
> On 1/6/2019 10:36 AM, Carl Eugen Hoyos wrote:
> > 2019-01-02 0:23 GMT+01:00, Michael Niedermayer <git at videolan.org>:
> >> ffmpeg | branch: master | Michael Niedermayer <michael at niedermayer.cc> | Mon
> >> Dec 31 18:25:18 2018 +0100| [9520d51e21f9aa5adc807b0b89322bd822b06738] |
> >> committer: Michael Niedermayer
> >>
> >> avcodec/avpacket: Avoid unspecific return -1 for av_grow_packet()
> >>
> >> Reviewed-by: Paul B Mahol <onemda at gmail.com>
> >> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> >>
> >>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=9520d51e21f9aa5adc807b0b89322bd822b06738
> >> ---
> >>
> >>  libavcodec/avpacket.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >> index e160ad3033..11ac4e80cd 100644
> >> --- a/libavcodec/avpacket.c
> >> +++ b/libavcodec/avpacket.c
> >> @@ -112,7 +112,7 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
> >>      av_assert0((unsigned)pkt->size <= INT_MAX -
> >> AV_INPUT_BUFFER_PADDING_SIZE);
> >>      if ((unsigned)grow_by >
> >>          INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
> >> -        return -1;
> >> +        return AVERROR(ENOMEM);
> >>
> >>      new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
> >>      if (pkt->buf) {
> >> @@ -124,7 +124,7 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
> >>          } else {
> >>              data_offset = pkt->data - pkt->buf->data;
> >>              if (data_offset > INT_MAX - new_size)
> >> -                return -1;
> >> +                return AVERROR(ENOMEM);
> > 
> > Is this really correct?
> > At least on some 64bit systems, larger allocations should be possible,
> > we are simply assuming invalid data if such an allocation is tried, no?
> 
> Even if av_max_alloc() allows the API user to allocate more than INT_MAX
> bytes, AVPackets can't handle it as they use int for size.

> 
> And afaik, we have been using ERANGE as return error when an attempted
> allocation is outside the allowed range. So maybe that's what should be
> used here.

about error codes
allocation can fail for many reasons
for example lack of physical memory, per process or per allocation
limits, or others.

we could return something else if people want, but iam not sure ERANGE is
ideal here

from libc docs:
 -- Macro: int ERANGE
     Range error; used by mathematical functions when the result value
     is not representable because of overflow or underflow.

if one looks at a function like av_grow_packet() it makes some sense
but the error codes are often passed through many functions and
when a final demux/decode/encode/... function then returns the failure
I think ENOMEM is closer than ERANGE to what the error is about.

I mean if a encode() returns 
"Range error; used by mathematical functions when the result value is not representable because of overflow or underflow."
this doesnt lead directly to a allocation size issue but sounds more
like an issue in some math computation


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190107/454588cb/attachment.sig>