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

[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.

Thanks,
Marton