| Message ID | GV1P250MB0737867CB56CC9382D6D92EB8F479@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avutil/dict: Fix memleak when using AV_DICT_APPEND | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 9/13/22, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > If a key already exists in an AVDictionary and the AV_DICT_APPEND flag > is set, the old entry is at first discarded from the dictionary, but > a pointer to the value is kept. Lateron enough memory to store the > appended string is allocated; should this allocation fail, the old string > is not freed and hence leaks. This commit changes this by moving > creating the combined value to an earlier point in the function, > which also ensures that the AVDictionaty is unchanged in case of errors. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > This is an improved version of > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191111011259.27313-1-andreas.rheinhardt@gmail.com/ > > libavutil/dict.c | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/libavutil/dict.c b/libavutil/dict.c > index a4f638a1fc..c01165634e 100644 > --- a/libavutil/dict.c > +++ b/libavutil/dict.c > @@ -73,7 +73,7 @@ int av_dict_set(AVDictionary **pm, const char *key, const > char *value, > { > AVDictionary *m = *pm; > AVDictionaryEntry *tag = NULL; > - char *oldval = NULL, *copy_key = NULL, *copy_value = NULL; > + char *copy_key = NULL, *copy_value = NULL; > > if (!(flags & AV_DICT_MULTIKEY)) { > tag = av_dict_get(m, key, NULL, flags); > @@ -97,10 +97,17 @@ int av_dict_set(AVDictionary **pm, const char *key, > const char *value, > av_free(copy_value); > return 0; > } > - if (flags & AV_DICT_APPEND) > - oldval = tag->value; > - else > - av_free(tag->value); > + if (copy_value && flags & AV_DICT_APPEND) { > + size_t len = strlen(tag->value) + strlen(copy_value) + 1; > + char *newval = av_mallocz(len); > + if (!newval) > + goto err_out; > + av_strlcat(newval, tag->value, len); > + av_strlcat(newval, copy_value, len); > + av_freep(©_value); > + copy_value = newval; > + } > + av_free(tag->value); > av_free(tag->key); > *tag = m->elems[--m->count]; > } else if (copy_value) { > @@ -113,17 +120,6 @@ int av_dict_set(AVDictionary **pm, const char *key, > const char *value, > if (copy_value) { > m->elems[m->count].key = copy_key; > m->elems[m->count].value = copy_value; > - if (oldval && flags & AV_DICT_APPEND) { > - size_t len = strlen(oldval) + strlen(copy_value) + 1; > - char *newval = av_mallocz(len); > - if (!newval) > - goto err_out; > - av_strlcat(newval, oldval, len); > - av_freep(&oldval); > - av_strlcat(newval, copy_value, len); > - m->elems[m->count].value = newval; > - av_freep(©_value); > - } > m->count++; > } else { > av_freep(©_key); > -- > 2.34.1 > LGTM > _______________________________________________ > 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/libavutil/dict.c b/libavutil/dict.c index a4f638a1fc..c01165634e 100644 --- a/libavutil/dict.c +++ b/libavutil/dict.c @@ -73,7 +73,7 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, { AVDictionary *m = *pm; AVDictionaryEntry *tag = NULL; - char *oldval = NULL, *copy_key = NULL, *copy_value = NULL; + char *copy_key = NULL, *copy_value = NULL; if (!(flags & AV_DICT_MULTIKEY)) { tag = av_dict_get(m, key, NULL, flags); @@ -97,10 +97,17 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, av_free(copy_value); return 0; } - if (flags & AV_DICT_APPEND) - oldval = tag->value; - else - av_free(tag->value); + if (copy_value && flags & AV_DICT_APPEND) { + size_t len = strlen(tag->value) + strlen(copy_value) + 1; + char *newval = av_mallocz(len); + if (!newval) + goto err_out; + av_strlcat(newval, tag->value, len); + av_strlcat(newval, copy_value, len); + av_freep(©_value); + copy_value = newval; + } + av_free(tag->value); av_free(tag->key); *tag = m->elems[--m->count]; } else if (copy_value) { @@ -113,17 +120,6 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, if (copy_value) { m->elems[m->count].key = copy_key; m->elems[m->count].value = copy_value; - if (oldval && flags & AV_DICT_APPEND) { - size_t len = strlen(oldval) + strlen(copy_value) + 1; - char *newval = av_mallocz(len); - if (!newval) - goto err_out; - av_strlcat(newval, oldval, len); - av_freep(&oldval); - av_strlcat(newval, copy_value, len); - m->elems[m->count].value = newval; - av_freep(©_value); - } m->count++; } else { av_freep(©_key);
If a key already exists in an AVDictionary and the AV_DICT_APPEND flag is set, the old entry is at first discarded from the dictionary, but a pointer to the value is kept. Lateron enough memory to store the appended string is allocated; should this allocation fail, the old string is not freed and hence leaks. This commit changes this by moving creating the combined value to an earlier point in the function, which also ensures that the AVDictionaty is unchanged in case of errors. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- This is an improved version of https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191111011259.27313-1-andreas.rheinhardt@gmail.com/ libavutil/dict.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)