| Message ID | 20211227002613.25069-1-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | fail | Make fate failed |
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@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. - Andreas PS: If the AVBPrint API had a documented way of marking data as used, one could avoid those stack buffers and use the AVBPrint buffer directly with av_bprint_get_buffer(). (Marking data as used would be equivalent to incrementing len and ensuring that the buffer stays zero-terminated.) If this were done, no already read data would be lost in case of a later allocation failure.
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@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. Regards, Marton > > - Andreas > > PS: If the AVBPrint API had a documented way of marking data as used, > one could avoid those stack buffers and use the AVBPrint buffer directly > with av_bprint_get_buffer(). (Marking data as used would be equivalent > to incrementing len and ensuring that the buffer stays zero-terminated.) > If this were done, no already read data would be lost in case of a later > allocation failure. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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@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
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@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
On Mon, 3 Jan 2022, Andreas Rheinhardt wrote: > 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@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. I don't feel strongly about it, it just looked convenient/consistent. But I will just skip this patch then and apply the rest. Regards, Marton
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;
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@passwd.hu> --- libavformat/aviobuf.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)