| Message ID | 20200919163610.1099233-21-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 1aee02c7c13ffd30135cac7d3d0c2fafdc922dc4 |
| Headers | show |
| Series | [FFmpeg-devel,01/21] avformat/dashdec: Avoid double free on error | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
> 2020年9月20日 上午12:36，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > There is still stuff left to fix in this demuxer after this patchset: > 1. resolve_content_path() is in bad shape; i.e. it can try to read > str[-1]. > 2. One can get a crash in get_current_fragment() (or rather in > ff_dash_fill_tmpl_params()) when one has a representation without fragment > and without fragment/url template. > 3. When one has a mismatch between old and new manifests in > refresh_manifest(), the old representations leak. One could fix this by > freeing the new representations and restoring the old ones, but this > feels wrong; freeing the old ones is not possible, because > refresh_manifests() is called indirectly by the read_packet function of > an AVFormatContext associated with an old representation, so freeing > the old representations would free the AVIOContext from within its > read_packet function. This would lead to use-after-frees. > > libavformat/dashdec.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index be67192b14..747b4e92e3 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -1183,7 +1183,6 @@ static int parse_manifest(AVFormatContext *s, const char *url, AVIOContext *in) > DASHContext *c = s->priv_data; > int ret = 0; > int close_in = 0; > - uint8_t *new_url = NULL; > int64_t filesize = 0; > AVBPrint buf; > AVDictionary *opts = NULL; > @@ -1212,11 +1211,8 @@ static int parse_manifest(AVFormatContext *s, const char *url, AVIOContext *in) > return ret; > } > > - if (av_opt_get(in, "location", AV_OPT_SEARCH_CHILDREN, &new_url) >= 0) { > - c->base_url = av_strdup(new_url); > - } else { > + if (av_opt_get(in, "location", AV_OPT_SEARCH_CHILDREN, (uint8_t**)&c->base_url) < 0) > c->base_url = av_strdup(url); > - } > > filesize = avio_size(in); > filesize = filesize > 0 ? filesize : DEFAULT_MANIFEST_SIZE; > @@ -1359,7 +1355,6 @@ cleanup: > xmlFreeNode(mpd_baseurl_node); > } > > - av_free(new_url); > av_bprint_finalize(&buf, NULL); > if (close_in) { > avio_close(in); > -- > 2.25.1 > > _______________________________________________ > 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". Patchset test ok, and lgtm. Thanks Steven Liu
diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c index be67192b14..747b4e92e3 100644 --- a/libavformat/dashdec.c +++ b/libavformat/dashdec.c @@ -1183,7 +1183,6 @@ static int parse_manifest(AVFormatContext *s, const char *url, AVIOContext *in) DASHContext *c = s->priv_data; int ret = 0; int close_in = 0; - uint8_t *new_url = NULL; int64_t filesize = 0; AVBPrint buf; AVDictionary *opts = NULL; @@ -1212,11 +1211,8 @@ static int parse_manifest(AVFormatContext *s, const char *url, AVIOContext *in) return ret; } - if (av_opt_get(in, "location", AV_OPT_SEARCH_CHILDREN, &new_url) >= 0) { - c->base_url = av_strdup(new_url); - } else { + if (av_opt_get(in, "location", AV_OPT_SEARCH_CHILDREN, (uint8_t**)&c->base_url) < 0) c->base_url = av_strdup(url); - } filesize = avio_size(in); filesize = filesize > 0 ? filesize : DEFAULT_MANIFEST_SIZE; @@ -1359,7 +1355,6 @@ cleanup: xmlFreeNode(mpd_baseurl_node); } - av_free(new_url); av_bprint_finalize(&buf, NULL); if (close_in) { avio_close(in);
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- There is still stuff left to fix in this demuxer after this patchset: 1. resolve_content_path() is in bad shape; i.e. it can try to read str[-1]. 2. One can get a crash in get_current_fragment() (or rather in ff_dash_fill_tmpl_params()) when one has a representation without fragment and without fragment/url template. 3. When one has a mismatch between old and new manifests in refresh_manifest(), the old representations leak. One could fix this by freeing the new representations and restoring the old ones, but this feels wrong; freeing the old ones is not possible, because refresh_manifests() is called indirectly by the read_packet function of an AVFormatContext associated with an old representation, so freeing the old representations would free the AVIOContext from within its read_packet function. This would lead to use-after-frees. libavformat/dashdec.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)