| Message ID | 20190903230300.927-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
James Almer: > Speeds up the process considerably. > > Fixes ticket #8109. > > Suggested-by: nevcairiel > Suggested-by: cehoyos > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/matroskadec.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index 439ee462a5..0f227eb33d 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax { > > typedef struct EbmlList { > int nb_elem; > + unsigned int alloc_elem_size; > void *elem; > } EbmlList; > > @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska, > data = (char *) data + syntax->data_offset; > if (syntax->list_elem_size) { > EbmlList *list = data; > - void *newelem = av_realloc_array(list->elem, list->nb_elem + 1, > - syntax->list_elem_size); > + void *newelem; > + > + if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size) > + return AVERROR(ENOMEM); > + > + newelem = av_fast_realloc(list->elem, > + &list->alloc_elem_size, > + (list->nb_elem + 1) * syntax->list_elem_size); If this fails, list->alloc_elem_size will be zero, yet list->elem and list->nb_elem might be non NULL/zero. I actually think that this will probably work, but it is nevertheless ugly. - Andreas
On 9/3/2019 8:36 PM, Andreas Rheinhardt wrote: > James Almer: >> Speeds up the process considerably. >> >> Fixes ticket #8109. >> >> Suggested-by: nevcairiel >> Suggested-by: cehoyos >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/matroskadec.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >> index 439ee462a5..0f227eb33d 100644 >> --- a/libavformat/matroskadec.c >> +++ b/libavformat/matroskadec.c >> @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax { >> >> typedef struct EbmlList { >> int nb_elem; >> + unsigned int alloc_elem_size; >> void *elem; >> } EbmlList; >> >> @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska, >> data = (char *) data + syntax->data_offset; >> if (syntax->list_elem_size) { >> EbmlList *list = data; >> - void *newelem = av_realloc_array(list->elem, list->nb_elem + 1, >> - syntax->list_elem_size); >> + void *newelem; >> + >> + if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size) >> + return AVERROR(ENOMEM); >> + >> + newelem = av_fast_realloc(list->elem, >> + &list->alloc_elem_size, >> + (list->nb_elem + 1) * syntax->list_elem_size); > If this fails, list->alloc_elem_size will be zero, yet list->elem and > list->nb_elem might be non NULL/zero. I actually think that this will > probably work, but it is nevertheless ugly. It should work. I'll push this tomorrow if no one objects, then backport to supported branches.
Von meinem iPhone gesendet > Am 04.09.2019 um 01:03 schrieb James Almer <jamrial@gmail.com>: > > Speeds up the process considerably. > > Fixes ticket #8109. > > Suggested-by: nevcairiel > Suggested-by: cehoyos > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/matroskadec.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index 439ee462a5..0f227eb33d 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax { > > typedef struct EbmlList { > int nb_elem; > + unsigned int alloc_elem_size; > void *elem; > } EbmlList; > > @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska, > data = (char *) data + syntax->data_offset; > if (syntax->list_elem_size) { > EbmlList *list = data; > - void *newelem = av_realloc_array(list->elem, list->nb_elem + 1, > - syntax->list_elem_size); > + void *newelem; > + if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size) > + return AVERROR(ENOMEM); I would have naively expected this to use INT_MAX. And please reconsider backporting random fixes for issues that are not regressions. Carl Eugen
On 9/4/2019 1:21 AM, Carl Eugen Hoyos wrote: > > > Von meinem iPhone gesendet > >> Am 04.09.2019 um 01:03 schrieb James Almer <jamrial@gmail.com>: >> >> Speeds up the process considerably. >> >> Fixes ticket #8109. >> >> Suggested-by: nevcairiel >> Suggested-by: cehoyos >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/matroskadec.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >> index 439ee462a5..0f227eb33d 100644 >> --- a/libavformat/matroskadec.c >> +++ b/libavformat/matroskadec.c >> @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax { >> >> typedef struct EbmlList { >> int nb_elem; >> + unsigned int alloc_elem_size; >> void *elem; >> } EbmlList; >> >> @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska, >> data = (char *) data + syntax->data_offset; >> if (syntax->list_elem_size) { >> EbmlList *list = data; >> - void *newelem = av_realloc_array(list->elem, list->nb_elem + 1, >> - syntax->list_elem_size); >> + void *newelem; > >> + if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size) >> + return AVERROR(ENOMEM); > > I would have naively expected this to use INT_MAX. The size parameter in av_fast_realloc() is unsigned int. av_add_index_entry() does the exact same check. > > And please reconsider backporting random fixes for issues that are not regressions. Despite not being a regression, the speed up from this fix on real world files is considerable, so it's worth backporting to at least release/4.2. > > Carl Eugen > _______________________________________________ > 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 9/4/2019 1:37 AM, James Almer wrote: > On 9/4/2019 1:21 AM, Carl Eugen Hoyos wrote: >> >> >> Von meinem iPhone gesendet >> >>> Am 04.09.2019 um 01:03 schrieb James Almer <jamrial@gmail.com>: >>> >>> Speeds up the process considerably. >>> >>> Fixes ticket #8109. >>> >>> Suggested-by: nevcairiel >>> Suggested-by: cehoyos >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavformat/matroskadec.c | 12 ++++++++++-- >>> 1 file changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >>> index 439ee462a5..0f227eb33d 100644 >>> --- a/libavformat/matroskadec.c >>> +++ b/libavformat/matroskadec.c >>> @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax { >>> >>> typedef struct EbmlList { >>> int nb_elem; >>> + unsigned int alloc_elem_size; >>> void *elem; >>> } EbmlList; >>> >>> @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska, >>> data = (char *) data + syntax->data_offset; >>> if (syntax->list_elem_size) { >>> EbmlList *list = data; >>> - void *newelem = av_realloc_array(list->elem, list->nb_elem + 1, >>> - syntax->list_elem_size); >>> + void *newelem; >> >>> + if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size) >>> + return AVERROR(ENOMEM); >> >> I would have naively expected this to use INT_MAX. > > The size parameter in av_fast_realloc() is unsigned int. > av_add_index_entry() does the exact same check. > >> >> And please reconsider backporting random fixes for issues that are not regressions. > > Despite not being a regression, the speed up from this fix on real world > files is considerable, so it's worth backporting to at least release/4.2. Pushed.
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 439ee462a5..0f227eb33d 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -110,6 +110,7 @@ typedef const struct EbmlSyntax { typedef struct EbmlList { int nb_elem; + unsigned int alloc_elem_size; void *elem; } EbmlList; @@ -1236,8 +1237,14 @@ static int ebml_parse(MatroskaDemuxContext *matroska, data = (char *) data + syntax->data_offset; if (syntax->list_elem_size) { EbmlList *list = data; - void *newelem = av_realloc_array(list->elem, list->nb_elem + 1, - syntax->list_elem_size); + void *newelem; + + if ((unsigned)list->nb_elem + 1 >= UINT_MAX / syntax->list_elem_size) + return AVERROR(ENOMEM); + + newelem = av_fast_realloc(list->elem, + &list->alloc_elem_size, + (list->nb_elem + 1) * syntax->list_elem_size); if (!newelem) return AVERROR(ENOMEM); list->elem = newelem; @@ -1490,6 +1497,7 @@ static void ebml_free(EbmlSyntax *syntax, void *data) ebml_free(syntax[i].def.n, ptr); av_freep(&list->elem); list->nb_elem = 0; + list->alloc_elem_size = 0; } else ebml_free(syntax[i].def.n, data_off); default:
Speeds up the process considerably. Fixes ticket #8109. Suggested-by: nevcairiel Suggested-by: cehoyos Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/matroskadec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)