| Message ID | 20200614180647.7024-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/4xm: Check that a video stream was created before returning packets for it | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Sun, 14 Jun 2020, Michael Niedermayer wrote: > Alternatively we could free the already allocated element Yeah, I kind of prefer that, we potentially allow non-string values to occur multiple times, so I'd say let's allow string values as well, even if that is not common. (I am not sure if it is strictly invalid or just uncommon). Regards, Marton > Fixes: memleak > Fixes: 23415/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5124814510751744 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/mxfdec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > index a60bdfeade..3b354864d9 100644 > --- a/libavformat/mxfdec.c > +++ b/libavformat/mxfdec.c > @@ -867,6 +867,8 @@ static inline int mxf_read_utf16_string(AVIOContext *pb, int size, char** str, i > return AVERROR(EINVAL); > > buf_size = size + size / 2 + 1; > + if (*str) > + return AVERROR_INVALIDDATA; > *str = av_malloc(buf_size); > if (!*str) > return AVERROR(ENOMEM); > -- > 2.17.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".
On Sun, Jun 14, 2020 at 08:19:18PM +0200, Marton Balint wrote: > > > On Sun, 14 Jun 2020, Michael Niedermayer wrote: > > > Alternatively we could free the already allocated element > > Yeah, I kind of prefer that, we potentially allow non-string values to occur > multiple times, so I'd say let's allow string values as well, even if that > is not common. (I am not sure if it is strictly invalid or just uncommon). ok then ill replace the error return by a av_free() and will apply soon thx [...]
sön 2020-06-14 klockan 20:19 +0200 skrev Marton Balint: > > On Sun, 14 Jun 2020, Michael Niedermayer wrote: > > > Alternatively we could free the already allocated element > > Yeah, I kind of prefer that, we potentially allow non-string values to > occur multiple times, so I'd say let's allow string values as well, even > if that is not common. (I am not sure if it is strictly invalid or just > uncommon). Are there files in the wild that do this? Being stricter is often the better option when it comes to MXF. A muxer that outputs metadata more than once might be doing something wrong. /Tomas
On Mon, 15 Jun 2020, Tomas Härdin wrote: > sön 2020-06-14 klockan 20:19 +0200 skrev Marton Balint: >> >> On Sun, 14 Jun 2020, Michael Niedermayer wrote: >> >> > Alternatively we could free the already allocated element >> >> Yeah, I kind of prefer that, we potentially allow non-string values to >> occur multiple times, so I'd say let's allow string values as well, even >> if that is not common. (I am not sure if it is strictly invalid or just >> uncommon). > > Are there files in the wild that do this? Being stricter is often the > better option when it comes to MXF. A muxer that outputs metadata more > than once might be doing something wrong. I am not sure, but I wanted to be safe rather than sorry, because it is not unusual in MXF to repeat certain metadata in the footer partition which is already present in the header, and I did not want to take the risk of rejecting a valid file. Regards, Marton
mån 2020-06-15 klockan 21:45 +0200 skrev Marton Balint: > > On Mon, 15 Jun 2020, Tomas Härdin wrote: > > > sön 2020-06-14 klockan 20:19 +0200 skrev Marton Balint: > > > On Sun, 14 Jun 2020, Michael Niedermayer wrote: > > > > > > > Alternatively we could free the already allocated element > > > > > > Yeah, I kind of prefer that, we potentially allow non-string values to > > > occur multiple times, so I'd say let's allow string values as well, even > > > if that is not common. (I am not sure if it is strictly invalid or just > > > uncommon). > > > > Are there files in the wild that do this? Being stricter is often the > > better option when it comes to MXF. A muxer that outputs metadata more > > than once might be doing something wrong. > > I am not sure, but I wanted to be safe rather than sorry, because it is > not unusual in MXF to repeat certain metadata in the footer partition > which is already present in the header, and I did not want to take the > risk of rejecting a valid file. True, that is not unheard of. Maybe we could warn if the strings don't match? /Tomas
On Tue, Jun 16, 2020 at 02:05:04PM +0200, Tomas Härdin wrote: > mån 2020-06-15 klockan 21:45 +0200 skrev Marton Balint: > > > > On Mon, 15 Jun 2020, Tomas Härdin wrote: > > > > > sön 2020-06-14 klockan 20:19 +0200 skrev Marton Balint: > > > > On Sun, 14 Jun 2020, Michael Niedermayer wrote: > > > > > > > > > Alternatively we could free the already allocated element > > > > > > > > Yeah, I kind of prefer that, we potentially allow non-string values to > > > > occur multiple times, so I'd say let's allow string values as well, even > > > > if that is not common. (I am not sure if it is strictly invalid or just > > > > uncommon). > > > > > > Are there files in the wild that do this? Being stricter is often the > > > better option when it comes to MXF. A muxer that outputs metadata more > > > than once might be doing something wrong. > > > > I am not sure, but I wanted to be safe rather than sorry, because it is > > not unusual in MXF to repeat certain metadata in the footer partition > > which is already present in the header, and I did not want to take the > > risk of rejecting a valid file. > > True, that is not unheard of. Maybe we could warn if the strings don't > match? such a warning seem like a good idea to me thx [...]
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index a60bdfeade..3b354864d9 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -867,6 +867,8 @@ static inline int mxf_read_utf16_string(AVIOContext *pb, int size, char** str, i return AVERROR(EINVAL); buf_size = size + size / 2 + 1; + if (*str) + return AVERROR_INVALIDDATA; *str = av_malloc(buf_size); if (!*str) return AVERROR(ENOMEM);
Alternatively we could free the already allocated element Fixes: memleak Fixes: 23415/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5124814510751744 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mxfdec.c | 2 ++ 1 file changed, 2 insertions(+)