Re: [FFmpeg-devel] [PATCH v2] avformat/nutdec: Add check for avformat_new_stream

Re: [FFmpeg-devel] [PATCH v2] avformat/nutdec: Add check for avformat_new_stream

[Jiasheng Jiang]

Andreas Rheinhardt:
>> As the potential failure of the memory allocation,
>> the avformat_new_stream() could return NULL pointer.
>> Therefore, it should be better to check it and return
>> error if fails.
>> 
>> Fixes: 84ad31ff18 ("lavf: replace av_new_stream->avformat_new_stream part II.")
>
> This commit did not introduce this bug; it merely replaced the unchecked
> function.
>
> If you look at nut_read_header() you will see that it just retries even
> on allocation failure. So this is not a complete fix. And if it retries
> and finds a different packet header, it adds ever more streams, because
> the already created streams have not been deleted. A proper fix would
> need to check the return value of decode_main_header for ENOMEM, but if
> time_base_count were invalid and huge, one could get an allocation error
> even though there might be a valid header somewhere else. So one would
> need an equivalent of NUT_MAX_STREAMS for timebases or some other
> criterion to rule this out.

Fine, I have submit a v2 to fix the problems above.

Jiang

_______________________________________________
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 v2] avformat/nutdec: Add check for avformat_new_stream

[Jiasheng Jiang]

Michael Niedermayer:
>> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
>> index 0a8a700acf..4cbccb20d9 100644
>> --- a/libavformat/nutdec.c
>> +++ b/libavformat/nutdec.c
>> @@ -220,6 +220,10 @@ static int decode_main_header(NUTContext *nut)
>>      }
>>  
>>      GET_V(nut->time_base_count, tmp > 0 && tmp < INT_MAX / sizeof(AVRational) && tmp < length/2);
>> +
>> +    if (nut->time_base_count > NUT_MAX_STREAMS)
>> +        return AVERROR_INVALIDDATA;
>
> the code already checks against length/2. If you want to add to that
> that should be done at the same level and
> such a change should explain why the existing check is insufficent as
> well as why the new is correct
> and it should be in a patch seperate from other changes
> also a file with NUT_MAX_STREAMS streams could use more timebases in principle
> timebases need a lot less space than streams so they could have a slightly
> higher limit

Thanks, I will remove the check in v3.

>> +
>>      nut->time_base = av_malloc_array(nut->time_base_count, sizeof(AVRational));
>>      if (!nut->time_base)
>>          return AVERROR(ENOMEM);
>> @@ -351,8 +355,13 @@ static int decode_main_header(NUTContext *nut)
>>          ret = AVERROR(ENOMEM);
>>          goto fail;
>>      }
>> -    for (i = 0; i < stream_count; i++)
>> -        avformat_new_stream(s, NULL);
>> +    for (i = 0; i < stream_count; i++) {
>> +        if (!avformat_new_stream(s, NULL)) {
>> +            av_free(nut->stream);
> 
> freeing something and not clearing the pointer is a bad idea in general

You are right.
I will change av_free to av_freep.

Jiang

_______________________________________________
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 v2] avformat/nutdec: Add check for avformat_new_stream

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3026 bytes --]

On Wed, Feb 16, 2022 at 04:40:16PM +0800, Jiasheng Jiang wrote:
> As the potential failure of the memory allocation,
> the avformat_new_stream() could return NULL pointer.
> Therefore, it should be better to check it and return
> error if fails.
> Also, the caller, nut_read_header(), needs to deal with
> the return value of the decode_main_header() and return
> error if memory allocation fails.
> To avoid mishandling the invalid 'time_base_count', another
> check for the 'time_base_count' is needed and return different
> error if fails.
> 
> Fixes: 619d8e2e58 ("updating nut demuxer to latest spec no muxing yet no index yet no seeking yet libnuts crcs dont match mine (didnt investigate yet) samplerate is stored wrong by libnut (demuxer has a workaround) code is not clean or beautifull yet, but i thought its better to commit early before someone unneccesarily wastes his time duplicating the work demuxer split from muxer")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
> Changelog:
> 
> v1 -> v2
> 
> * Change 1. Add the error handling for ENOMEM from decode_main_header()
> in nut_read_header().
> * Change 2. Check for the 'time_base_count'.
> ---
>  libavformat/nutdec.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
> index 0a8a700acf..4cbccb20d9 100644
> --- a/libavformat/nutdec.c
> +++ b/libavformat/nutdec.c
> @@ -220,6 +220,10 @@ static int decode_main_header(NUTContext *nut)
>      }
>  
>      GET_V(nut->time_base_count, tmp > 0 && tmp < INT_MAX / sizeof(AVRational) && tmp < length/2);
> +
> +    if (nut->time_base_count > NUT_MAX_STREAMS)
> +        return AVERROR_INVALIDDATA;

the code already checks against length/2. If you want to add to that
that should be done at the same level and
such a change should explain why the existing check is insufficent as
well as why the new is correct
and it should be in a patch seperate from other changes
also a file with NUT_MAX_STREAMS streams could use more timebases in principle
timebases need a lot less space than streams so they could have a slightly
higher limit


> +
>      nut->time_base = av_malloc_array(nut->time_base_count, sizeof(AVRational));
>      if (!nut->time_base)
>          return AVERROR(ENOMEM);
> @@ -351,8 +355,13 @@ static int decode_main_header(NUTContext *nut)
>          ret = AVERROR(ENOMEM);
>          goto fail;
>      }
> -    for (i = 0; i < stream_count; i++)
> -        avformat_new_stream(s, NULL);
> +    for (i = 0; i < stream_count; i++) {
> +        if (!avformat_new_stream(s, NULL)) {
> +            av_free(nut->stream);

freeing something and not clearing the pointer is a bad idea in general

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] [PATCH v2] avformat/nutdec: Add check for avformat_new_stream

[Jiasheng Jiang]

As the potential failure of the memory allocation,
the avformat_new_stream() could return NULL pointer.
Therefore, it should be better to check it and return
error if fails.
Also, the caller, nut_read_header(), needs to deal with
the return value of the decode_main_header() and return
error if memory allocation fails.
To avoid mishandling the invalid 'time_base_count', another
check for the 'time_base_count' is needed and return different
error if fails.

Fixes: 619d8e2e58 ("updating nut demuxer to latest spec no muxing yet no index yet no seeking yet libnuts crcs dont match mine (didnt investigate yet) samplerate is stored wrong by libnut (demuxer has a workaround) code is not clean or beautifull yet, but i thought its better to commit early before someone unneccesarily wastes his time duplicating the work demuxer split from muxer")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
Changelog:

v1 -> v2

* Change 1. Add the error handling for ENOMEM from decode_main_header()
in nut_read_header().
* Change 2. Check for the 'time_base_count'.
---
 libavformat/nutdec.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
index 0a8a700acf..4cbccb20d9 100644
--- a/libavformat/nutdec.c
+++ b/libavformat/nutdec.c
@@ -220,6 +220,10 @@ static int decode_main_header(NUTContext *nut)
     }
 
     GET_V(nut->time_base_count, tmp > 0 && tmp < INT_MAX / sizeof(AVRational) && tmp < length/2);
+
+    if (nut->time_base_count > NUT_MAX_STREAMS)
+        return AVERROR_INVALIDDATA;
+
     nut->time_base = av_malloc_array(nut->time_base_count, sizeof(AVRational));
     if (!nut->time_base)
         return AVERROR(ENOMEM);
@@ -351,8 +355,13 @@ static int decode_main_header(NUTContext *nut)
         ret = AVERROR(ENOMEM);
         goto fail;
     }
-    for (i = 0; i < stream_count; i++)
-        avformat_new_stream(s, NULL);
+    for (i = 0; i < stream_count; i++) {
+        if (!avformat_new_stream(s, NULL)) {
+            av_free(nut->stream);
+            ret = AVERROR(ENOMEM);
+            goto fail;
+        }
+    }
 
     return 0;
 fail:
@@ -800,19 +809,23 @@ static int nut_read_header(AVFormatContext *s)
     NUTContext *nut = s->priv_data;
     AVIOContext *bc = s->pb;
     int64_t pos;
-    int initialized_stream_count;
+    int initialized_stream_count, ret;
 
     nut->avf = s;
 
     /* main header */
     pos = 0;
+    ret = 0;
     do {
+        if (ret == AVERROR(ENOMEM))
+            return ret;
+
         pos = find_startcode(bc, MAIN_STARTCODE, pos) + 1;
         if (pos < 0 + 1) {
             av_log(s, AV_LOG_ERROR, "No main startcode found.\n");
             return AVERROR_INVALIDDATA;
         }
-    } while (decode_main_header(nut) < 0);
+    } while ((ret = decode_main_header(nut)) < 0);
 
     /* stream headers */
     pos = 0;
-- 
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".