[FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Fri, 31 Dec 2021, Marton Balint wrote:
> 
>>
>>
>> On Fri, 31 Dec 2021, Andreas Rheinhardt wrote:
>>
>>>  Marton Balint:
>>>>  This makes sure the error condition is kept in AVIOContext even if the
>>>>  user
>>>>  does not check the return value of avio_read_to_bprint or
>>>>  ff_read_line_to_bprint.
>>>>
>>>>  Signed-off-by: Marton Balint <cus at passwd.hu>
>>>>  ---
>>>>   libavformat/aviobuf.c | 8 ++++++--
>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>>  diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>>>  index 29d4bd7510..6f8a822ee3 100644
>>>>  --- a/libavformat/aviobuf.c
>>>>  +++ b/libavformat/aviobuf.c
>>>>  @@ -875,8 +875,10 @@ static int64_t
>>>>  read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
>>>>       if (ret < 0)
>>>>           return ret;
>>>>
>>>>  -    if (!av_bprint_is_complete(bp))
>>>>  +    if (!av_bprint_is_complete(bp)) {
>>>>  +        s->error = AVERROR(ENOMEM);
>>>>           return AVERROR(ENOMEM);
>>>>  +    }
>>>>
>>>>       return bp->len;
>>>>  }
>>>>  @@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h,
>>>> AVBPrint
>>>>  *pb, size_t max_size)
>>>>           if (ret <= 0)
>>>>               return ret;
>>>>           av_bprint_append_data(pb, buf, ret);
>>>>  -        if (!av_bprint_is_complete(pb))
>>>>  +        if (!av_bprint_is_complete(pb)) {
>>>>  +            h->error = AVERROR(ENOMEM);
>>>>               return AVERROR(ENOMEM);
>>>>  +        }
>>>>           max_size -= ret;
>>>>       }
>>>>       return 0;
>>>>
>>>
>>>  I don't really see the point of this: It is not a real read error that
>>>  should stick to the AVIOContext (which can still be used afterwards
>>>  without any issue).
>>>  If the user does not check the errors, then the user
>>>  has no one to blame but himself for missing errors.
>>
>> AVIO read/write behaviour is to store IO errors in the context so the
>> user does not have to check for them in every call. It is not well
>> documented which calls should be checked always, so the user might be
>> under the impression that errors during read/write may be checked
>> sometime later.
>>
>> Admittedly, ENOMEM is not an IO error, but I think it is better to
>> store that as well in the context to keep the behaviour consistent,
>> because in case of ENOMEM avio_read_to_bprint reads and drops
>> undefined amount of data, so the context will also be in an undefined
>> state.
>>
>> Other possibilities:
>> - make avio_read_to_bprint read all the data regardless of AVBPrint
>> fullness
>>  - mark avio_read_to_bprint av_warn_unused_result.
>>  - both :)
>>
>> But these also forces the user to check return values... So I kind of
>> like my original approach better, because it maintains avio_read/write
>> call behaviour that it is safe to check errors sometime later.
> 
> Any more comments about this or the rest of the series? I plan to apply
> it tomorrow.
> 

I still don't like storing ENOMEM in the AVIOContext and I don't see why
having to check the error is so burdensome. But if you want it so bad,
then go ahead.

- Andreas