diff mbox series

[FFmpeg-devel,2/2] avformat/mxfdec: Error out on duplicated utf16 strings

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer June 14, 2020, 6:06 p.m. UTC
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(+)

Comments

Marton Balint June 14, 2020, 6:19 p.m. UTC | #1
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".
Michael Niedermayer June 14, 2020, 6:43 p.m. UTC | #2
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

[...]
Tomas Härdin June 15, 2020, 8:38 a.m. UTC | #3
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
Marton Balint June 15, 2020, 7:45 p.m. UTC | #4
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
Tomas Härdin June 16, 2020, 12:05 p.m. UTC | #5
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
Michael Niedermayer June 16, 2020, 8:34 p.m. UTC | #6
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 mbox series

Patch

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);