| Message ID | 20210726130506.374-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/concat: finalize the AVBprint buffer immediately | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Mon, 26 Jul 2021, James Almer wrote: > Don't attempt to read its contents in place. > Fixes invalid reads when run under Valgrind. As far as I remember AVBPrint buffer CAN be read in place by design, zero terminator is always guaranteed, not only after finalizing. So this should not be needed. What is causing the invalid reads exactly? Thanks, Marton > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/concat.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/libavformat/concat.c b/libavformat/concat.c > index aec1f52d8e..64ac03e1d8 100644 > --- a/libavformat/concat.c > +++ b/libavformat/concat.c > @@ -211,6 +211,7 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags) > struct concat_data *data = h->priv_data; > AVIOContext *in = NULL; > const char *cursor; > + char *buf; > int64_t total_size = 0; > unsigned int nodes_size = 0; > size_t i = 0; > @@ -238,7 +239,11 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags) > return err; > } > > - cursor = bp.str; > + err = av_bprint_finalize(&bp, &buf); > + if (err < 0) > + return err; > + > + cursor = buf; > while (*cursor) { > struct concat_nodes *nodes; > URLContext *uc; > @@ -286,7 +291,7 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags) > data->nodes[i++].size = size; > total_size += size; > } > - av_bprint_finalize(&bp, NULL); > + av_free(buf); > data->length = i; > > if (err < 0) > -- > 2.32.0 > > _______________________________________________ > 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". >
Marton Balint (12021-07-26): > As far as I remember AVBPrint buffer CAN be read in place by design, zero > terminator is always guaranteed, not only after finalizing. So this should > not be needed. What is causing the invalid reads exactly? I confirm your memory. The documentation states: * The string buffer grows as necessary and is always 0-terminated. And I checked: there is no obvious bug in the avio_read_to_bprint() code path, it ends with av_bprint_grow() adding the final 0. Regards,
On 7/26/2021 6:49 PM, Marton Balint wrote: > > > On Mon, 26 Jul 2021, James Almer wrote: > >> Don't attempt to read its contents in place. >> Fixes invalid reads when run under Valgrind. > > As far as I remember AVBPrint buffer CAN be read in place by design, > zero terminator is always guaranteed, not only after finalizing. So this > should not be needed. What is causing the invalid reads exactly? > > Thanks, > Marton It happens only when you use a URI string that doesn't end with a line break. I just noticed that after this patch some invalid reads still happen in any case. I think i found out the culprit, and the following seems to fix it: > diff --git a/libavformat/concat.c b/libavformat/concat.c > index aec1f52d8e..94917840c6 100644 > --- a/libavformat/concat.c > +++ b/libavformat/concat.c > @@ -251,7 +251,7 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags) > err = AVERROR(ENOMEM); > break; > } > - cursor++; > + if (*cursor) cursor++; > > if (++len == SIZE_MAX / sizeof(*nodes)) { > av_free(node_uri); Basically, av_get_token() returns a pointer to the terminating character, so cursor++ for the next loop when said character was \0 was wrong.
On Mon, 26 Jul 2021, James Almer wrote: > On 7/26/2021 6:49 PM, Marton Balint wrote: >> >> >> On Mon, 26 Jul 2021, James Almer wrote: >> >>> Don't attempt to read its contents in place. >>> Fixes invalid reads when run under Valgrind. >> >> As far as I remember AVBPrint buffer CAN be read in place by design, zero >> terminator is always guaranteed, not only after finalizing. So this should >> not be needed. What is causing the invalid reads exactly? >> >> Thanks, >> Marton > > It happens only when you use a URI string that doesn't end with a line break. > I just noticed that after this patch some invalid reads still happen in any > case. > > I think i found out the culprit, and the following seems to fix it: > >> diff --git a/libavformat/concat.c b/libavformat/concat.c >> index aec1f52d8e..94917840c6 100644 >> --- a/libavformat/concat.c >> +++ b/libavformat/concat.c >> @@ -251,7 +251,7 @@ static av_cold int concatf_open(URLContext *h, const >> char *uri, int flags) >> err = AVERROR(ENOMEM); >> break; >> } >> - cursor++; >> + if (*cursor) cursor++; Yeah, I suspected the same. So this seems to be the correct patch to apply. Thanks, Marton >> >> if (++len == SIZE_MAX / sizeof(*nodes)) { >> av_free(node_uri); > > Basically, av_get_token() returns a pointer to the terminating character, so > cursor++ for the next loop when said character was \0 was wrong. > _______________________________________________ > 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". >
diff --git a/libavformat/concat.c b/libavformat/concat.c index aec1f52d8e..64ac03e1d8 100644 --- a/libavformat/concat.c +++ b/libavformat/concat.c @@ -211,6 +211,7 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags) struct concat_data *data = h->priv_data; AVIOContext *in = NULL; const char *cursor; + char *buf; int64_t total_size = 0; unsigned int nodes_size = 0; size_t i = 0; @@ -238,7 +239,11 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags) return err; } - cursor = bp.str; + err = av_bprint_finalize(&bp, &buf); + if (err < 0) + return err; + + cursor = buf; while (*cursor) { struct concat_nodes *nodes; URLContext *uc; @@ -286,7 +291,7 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags) data->nodes[i++].size = size; total_size += size; } - av_bprint_finalize(&bp, NULL); + av_free(buf); data->length = i; if (err < 0)
Don't attempt to read its contents in place. Fixes invalid reads when run under Valgrind. Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/concat.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)