| Message ID | 20201008124521.4025534-1-ccom@randomderp.com |
|---|---|
| State | Accepted |
| Commit | 0117d5aa03aca0158ee54b806d420fb1a974b788 |
| Headers | show |
| Series | [FFmpeg-devel] libavformat/dashdec: Fix issue with dash on Windows | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Christopher Degawa: > Use xmlFree instead of av_freep > > snip from libxml2: > > * xmlGetProp: > ... > * Returns the attribute value or NULL if not found. > * It's up to the caller to free the memory with xmlFree(). > > According to libxml2, you are supposed to use xmlFree instead of free > on the pointer returned by it, and also using av_freep on Windows will > call _aligned_free instead of normal free, causing _aligned_free to raise > SIGTRAP and crashing ffmpeg and ffplay. > > To reproduce, download a build from gyan or BtBN for windows > (also happens with some of Zeranoe's builds) > > https://www.gyan.dev/ffmpeg/builds/ > https://github.com/BtbN/FFmpeg-Builds > > and run either > > ffplay.exe http://download.tsi.telecom-paristech.fr/gpac/DASH_CONFORMANCE/TelecomParisTech/mp4-live/mp4-live-mpd-AV-NBS.mpd > > or > > ffmpeg.exe -i http://download.tsi.telecom-paristech.fr/gpac/DASH_CONFORMANCE/TelecomParisTech/mp4-live/mp4-live-mpd-AV-NBS.mpd -f null - > > Signed-off-by: Christopher Degawa <ccom@randomderp.com> > --- > libavformat/dashdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 99b9c45439..42ea74635b 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -1145,7 +1145,7 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, > } > > err: > - av_freep(&c->adaptionset_lang); > + xmlFree(c->adaptionset_lang); > return ret; > } > > You should also reset c->adaptionset_lang to NULL after freeing it. (I remember finding this issue myself, but somehow forgot to fix it. Strange.) (Actually, the lifetime of adaptionset_lang does not extend beyond parse_manifest_adaptationset(), so one could even use a local variable for it.) - Andreas
On 10/8/2020 9:45 AM, Christopher Degawa wrote: > Use xmlFree instead of av_freep > > snip from libxml2: > > * xmlGetProp: > ... > * Returns the attribute value or NULL if not found. > * It's up to the caller to free the memory with xmlFree(). > > According to libxml2, you are supposed to use xmlFree instead of free > on the pointer returned by it, and also using av_freep on Windows will > call _aligned_free instead of normal free, causing _aligned_free to raise > SIGTRAP and crashing ffmpeg and ffplay. > > To reproduce, download a build from gyan or BtBN for windows > (also happens with some of Zeranoe's builds) > > https://www.gyan.dev/ffmpeg/builds/ > https://github.com/BtbN/FFmpeg-Builds > > and run either > > ffplay.exe http://download.tsi.telecom-paristech.fr/gpac/DASH_CONFORMANCE/TelecomParisTech/mp4-live/mp4-live-mpd-AV-NBS.mpd > > or > > ffmpeg.exe -i http://download.tsi.telecom-paristech.fr/gpac/DASH_CONFORMANCE/TelecomParisTech/mp4-live/mp4-live-mpd-AV-NBS.mpd -f null - > > Signed-off-by: Christopher Degawa <ccom@randomderp.com> > --- > libavformat/dashdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 99b9c45439..42ea74635b 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -1145,7 +1145,7 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, > } > > err: > - av_freep(&c->adaptionset_lang); > + xmlFree(c->adaptionset_lang); > return ret; > } Applied, thanks!
> You should also reset c->adaptionset_lang to NULL after freeing it. (I > remember finding this issue myself, but somehow forgot to fix it. Strange.) Ah, I forgot that's what av_freep does in addition to freeing, do you wish to make the change, or do you want me to? > (Actually, the lifetime of adaptionset_lang does not extend beyond > parse_manifest_adaptationset(), so one could even use a local variable > for it.) I do also see it being used in parse_manifest_representation, so that would be one more place you would need to make sure to pass the local variable, but that would save space on the struct.
Christopher Degawa: >> You should also reset c->adaptionset_lang to NULL after freeing it. (I >> remember finding this issue myself, but somehow forgot to fix it. Strange.) > > Ah, I forgot that's what av_freep does in addition to freeing, do you > wish to make the change, or do you want me to? > I already made a patch and sent it to the ML a few minutes ago, but somehow it is still stuck. I'll just apply it, it's trivial anyway. >> (Actually, the lifetime of adaptionset_lang does not extend beyond >> parse_manifest_adaptationset(), so one could even use a local variable >> for it.) > > I do also see it being used in parse_manifest_representation, so that > would be one more place you would need to make sure to pass the local > variable, but that would save space on the struct. Yeah, but parse_manifest_representation() is only called from parse_manifest_adaptationset(), so it is possible. (This is also the reason why I forgot to fix it: Initially I believed there was a memleak when adaptionset_lang is simply overwritten without checking whether it is not NULL, but then I saw that it actually always is NULL at that point and so I didn't change anything, despite noticing that it should use xmlFree().) - Andreas
diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c index 99b9c45439..42ea74635b 100644 --- a/libavformat/dashdec.c +++ b/libavformat/dashdec.c @@ -1145,7 +1145,7 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, } err: - av_freep(&c->adaptionset_lang); + xmlFree(c->adaptionset_lang); return ret; }
Use xmlFree instead of av_freep snip from libxml2: * xmlGetProp: ... * Returns the attribute value or NULL if not found. * It's up to the caller to free the memory with xmlFree(). According to libxml2, you are supposed to use xmlFree instead of free on the pointer returned by it, and also using av_freep on Windows will call _aligned_free instead of normal free, causing _aligned_free to raise SIGTRAP and crashing ffmpeg and ffplay. To reproduce, download a build from gyan or BtBN for windows (also happens with some of Zeranoe's builds) https://www.gyan.dev/ffmpeg/builds/ https://github.com/BtbN/FFmpeg-Builds and run either ffplay.exe http://download.tsi.telecom-paristech.fr/gpac/DASH_CONFORMANCE/TelecomParisTech/mp4-live/mp4-live-mpd-AV-NBS.mpd or ffmpeg.exe -i http://download.tsi.telecom-paristech.fr/gpac/DASH_CONFORMANCE/TelecomParisTech/mp4-live/mp4-live-mpd-AV-NBS.mpd -f null - Signed-off-by: Christopher Degawa <ccom@randomderp.com> --- libavformat/dashdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)