[FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Chris Ribble]

This reverts commit 03d81a044ad587ea83567f75dc36bc3d64278199.

This change broke the ability to read mp4 files which contain a trun
atom with a sample of size zero (FFmpeg exits while parsing the moof).

Signed-off-by: Chris Ribble <chris.ribble@resi.io>
---
 libavformat/mov.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 29bd3103e3..b67b7cd9d2 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5293,8 +5293,6 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
         distance++;
         if (av_sat_add64(dts, sample_duration) != dts + (uint64_t)sample_duration)
             return AVERROR_INVALIDDATA;
-        if (!sample_size)
-            return AVERROR_INVALIDDATA;
         dts += sample_duration;
         offset += sample_size;
         sc->data_size += sample_size;
-- 
2.37.4

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Marton Balint]

On Thu, 1 Dec 2022, Chris Ribble wrote:

> This reverts commit 03d81a044ad587ea83567f75dc36bc3d64278199.
>
> This change broke the ability to read mp4 files which contain a trun
> atom with a sample of size zero (FFmpeg exits while parsing the moof).

Can you explain why those files are considered valid, or why it makes 
sense to generate such files?

Thanks,
Marton

>
> Signed-off-by: Chris Ribble <chris.ribble@resi.io>
> ---
> libavformat/mov.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 29bd3103e3..b67b7cd9d2 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -5293,8 +5293,6 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>         distance++;
>         if (av_sat_add64(dts, sample_duration) != dts + (uint64_t)sample_duration)
>             return AVERROR_INVALIDDATA;
> -        if (!sample_size)
> -            return AVERROR_INVALIDDATA;
>         dts += sample_duration;
>         offset += sample_size;
>         sc->data_size += sample_size;
> -- 
> 2.37.4
>
> _______________________________________________
> 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".
>
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Chris Ribble]

On Thu, Dec 1, 2022 at 4:51 PM Marton Balint <cus@passwd.hu> wrote:
>
> Can you explain why those files are considered valid, or why it makes
> sense to generate such files?
>
> Thanks,
> Marton
>

As far as I can tell, the file that a user provided with this problem
was generated by an encoder (running FFmpeg 3.4) that started writing
zero-sized samples when their video switcher + capture card stopped
receiving audio input. I'm not arguing that it's good for files to be
generated like this, but it's nice for FFmpeg to be able to process
them all the same (i.e. the robustness principle).

With this patch reverted, FFmpeg can accept an input file that is
partially broken (with playback anomalies due to the presence of
zero-sized samples) and produce a valid, working output mp4 (or DASH
stream), just like it could in release 5.0 and older.

One of the best things about FFmpeg is that it can fix invalid
container metadata. I feel like losing that capability for this
scenario is a regression.

Thanks,
Chris
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Gyan Doshi]

On 2022-12-02 06:16 am, Chris Ribble wrote:
> On Thu, Dec 1, 2022 at 4:51 PM Marton Balint <cus@passwd.hu> wrote:
>> Can you explain why those files are considered valid, or why it makes
>> sense to generate such files?
>>
>> Thanks,
>> Marton
>>
> As far as I can tell, the file that a user provided with this problem
> was generated by an encoder (running FFmpeg 3.4) that started writing
> zero-sized samples when their video switcher + capture card stopped
> receiving audio input. I'm not arguing that it's good for files to be
> generated like this, but it's nice for FFmpeg to be able to process
> them all the same (i.e. the robustness principle).
>
> With this patch reverted, FFmpeg can accept an input file that is
> partially broken (with playback anomalies due to the presence of
> zero-sized samples) and produce a valid, working output mp4 (or DASH
> stream), just like it could in release 5.0 and older.
>
> One of the best things about FFmpeg is that it can fix invalid
> container metadata. I feel like losing that capability for this
> scenario is a regression.

FWIW, we don't discard regular MP4s with sample entries of 0 in stts, 
which is only permitted for the last solo sample in a track. So, I agree.

Regards,
Gyan
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Marton Balint]

On Fri, 2 Dec 2022, Gyan Doshi wrote:

>
>
> On 2022-12-02 06:16 am, Chris Ribble wrote:
>>  On Thu, Dec 1, 2022 at 4:51 PM Marton Balint <cus@passwd.hu> wrote:
>>>  Can you explain why those files are considered valid, or why it makes
>>>  sense to generate such files?
>>>
>>>  Thanks,
>>>  Marton
>>>
>>  As far as I can tell, the file that a user provided with this problem
>>  was generated by an encoder (running FFmpeg 3.4) that started writing
>>  zero-sized samples when their video switcher + capture card stopped
>>  receiving audio input. I'm not arguing that it's good for files to be
>>  generated like this, but it's nice for FFmpeg to be able to process
>>  them all the same (i.e. the robustness principle).
>>
>>  With this patch reverted, FFmpeg can accept an input file that is
>>  partially broken (with playback anomalies due to the presence of
>>  zero-sized samples) and produce a valid, working output mp4 (or DASH
>>  stream), just like it could in release 5.0 and older.
>>
>>  One of the best things about FFmpeg is that it can fix invalid
>>  container metadata. I feel like losing that capability for this
>>  scenario is a regression.
>
> FWIW, we don't discard regular MP4s with sample entries of 0 in stts, which 
> is only permitted for the last solo sample in a track. So, I agree.

More strict enforcement of sample size was introduced to avoid DOS/Timeout 
with crafted (fuzzed) files and disallow emitting zero sized packets.

Invalid file support is not something that is always worth doing, there 
are other, more important factors, like limiting code complexity or 
improving resiliance against denial of service. The problem here is that I 
honestly don't know if a zero sample size is against spec, just stupid, or 
there is a legitimate use for it.

So I sent a 2 patch series which fixes the original issue differently. 
Please test and review them if you can.

Thanks,
Marton
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Chris Ribble]

> More strict enforcement of sample size was introduced to avoid DOS/Timeout
> with crafted (fuzzed) files and disallow emitting zero sized packets.
>
> Invalid file support is not something that is always worth doing, there
> are other, more important factors, like limiting code complexity or
> improving resiliance against denial of service. The problem here is that I
> honestly don't know if a zero sample size is against spec, just stupid, or
> there is a legitimate use for it.
>
> So I sent a 2 patch series which fixes the original issue differently.
> Please test and review them if you can.

Marton,

Thank you for looking into this further.

I tried your patch series and FFmpeg still generates an error
(AVERROR_INVALIDDATA) while processing the moof fragment with
zero-sized samples.

It seems like !(flags & MOV_TRUN_SAMPLE_SIZE) evaluates to true when
the flag is set (512 is "inverted" to 1). Is that what you had in
mind?

If I change it to this, things work as expected for my input mp4:
if (entries && !frag->size && (flags & MOV_TRUN_SAMPLE_SIZE !=
MOV_TRUN_SAMPLE_SIZE))
  return AVERROR_INVALIDDATA;

Sorry if I was supposed to provide this feedback on the patch series
itself; I'm happy to do so there as needed.

Thanks,
Chris
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Marton Balint]

On Sun, 4 Dec 2022, Chris Ribble wrote:

>> More strict enforcement of sample size was introduced to avoid DOS/Timeout
>> with crafted (fuzzed) files and disallow emitting zero sized packets.
>>
>> Invalid file support is not something that is always worth doing, there
>> are other, more important factors, like limiting code complexity or
>> improving resiliance against denial of service. The problem here is that I
>> honestly don't know if a zero sample size is against spec, just stupid, or
>> there is a legitimate use for it.
>>
>> So I sent a 2 patch series which fixes the original issue differently.
>> Please test and review them if you can.
>
> Marton,
>
> Thank you for looking into this further.
>
> I tried your patch series and FFmpeg still generates an error
> (AVERROR_INVALIDDATA) while processing the moof fragment with
> zero-sized samples.

Hmm, strange.

>
> It seems like !(flags & MOV_TRUN_SAMPLE_SIZE) evaluates to true when
> the flag is set (512 is "inverted" to 1). Is that what you had in
> mind?

!(flags & 0x200) should evaluate to 0 if the flag is set, 1 otherwise.

>
> If I change it to this, things work as expected for my input mp4:
> if (entries && !frag->size && (flags & MOV_TRUN_SAMPLE_SIZE !=
> MOV_TRUN_SAMPLE_SIZE))

The precedence is funny here. != is evaluated first, not &.

>  return AVERROR_INVALIDDATA;
>
> Sorry if I was supposed to provide this feedback on the patch series
> itself; I'm happy to do so there as needed.

Share the sample please. If the patch I proposed does not fix it, I am not 
sure what is the best approach here.

Thanks,
Marton
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Chris Ribble]

On Mon, Dec 5, 2022 at 12:04 PM Marton Balint <cus@passwd.hu> wrote:
>
> Share the sample please. If the patch I proposed does not fix it, I am not
> sure what is the best approach here.
>

Sure, I will send you the URL to the file so that you can try it.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] Revert "avformat/mov: disallow a zero sample size in trun atoms"

[Marton Balint]

On Tue, 6 Dec 2022, Chris Ribble wrote:

> On Mon, Dec 5, 2022 at 12:04 PM Marton Balint <cus@passwd.hu> wrote:
>>
>> Share the sample please. If the patch I proposed does not fix it, I am not
>> sure what is the best approach here.
>>
>
> Sure, I will send you the URL to the file so that you can try it.

Ok, thanks, I will post a v2 patch which fixes the issue differently.

Regards,
Marton
_______________________________________________
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".