diff mbox series

[FFmpeg-devel,3/8] avcodec/decode: Check size before opening iconv

Message ID 20210304154233.934640-3-andreas.rheinhardt@gmail.com
State Accepted
Commit a272f382d3eb143e9da99537aec25dbbe5778614
Headers show
Series [FFmpeg-devel,1/8] avcodec/decode: constify the source packet parameter in extract_packet_props() | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt March 4, 2021, 3:42 p.m. UTC 
Avoids closing iconv when the size check fails.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/decode.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

James Almer March 4, 2021, 5:19 p.m. UTC | #1 
On 3/4/2021 12:42 PM, Andreas Rheinhardt wrote:
> Avoids closing iconv when the size check fails.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>   libavcodec/decode.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index db6ee9cb04..c976795311 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -884,18 +884,17 @@ static int recode_subtitle(AVCodecContext *avctx,
>           return 0;
>   
>   #if CONFIG_ICONV
> -    cd = iconv_open("UTF-8", avctx->sub_charenc);
> -    av_assert0(cd != (iconv_t)-1);
> -
>       inb = inpkt->data;
>       inl = inpkt->size;
>   
>       if (inl >= INT_MAX / UTF8_MAX_BYTES - AV_INPUT_BUFFER_PADDING_SIZE) {
>           av_log(avctx, AV_LOG_ERROR, "Subtitles packet is too big for recoding\n");
> -        ret = AVERROR(ENOMEM);
> -        goto end;
> +        return AVERROR(ERANGE);
>       }
>   
> +    cd = iconv_open("UTF-8", avctx->sub_charenc);
> +    av_assert0(cd != (iconv_t)-1);

Unrelated to this patch, but I don't think we should assert an external 
library return value. Asserts should be used to detect internal bugs, 
stuff we have control over, and we have no control over the behavior of 
iconv_open().
So this should be changed into a normal check, and return 
AVERROR_EXTERNAL on failure.

> +
>       ret = av_new_packet(&tmp, inl * UTF8_MAX_BYTES);
>       if (ret < 0)
>           goto end;
> 

LGTM.
diff mbox series

Patch

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index db6ee9cb04..c976795311 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -884,18 +884,17 @@  static int recode_subtitle(AVCodecContext *avctx,
         return 0;
 
 #if CONFIG_ICONV
-    cd = iconv_open("UTF-8", avctx->sub_charenc);
-    av_assert0(cd != (iconv_t)-1);
-
     inb = inpkt->data;
     inl = inpkt->size;
 
     if (inl >= INT_MAX / UTF8_MAX_BYTES - AV_INPUT_BUFFER_PADDING_SIZE) {
         av_log(avctx, AV_LOG_ERROR, "Subtitles packet is too big for recoding\n");
-        ret = AVERROR(ENOMEM);
-        goto end;
+        return AVERROR(ERANGE);
     }
 
+    cd = iconv_open("UTF-8", avctx->sub_charenc);
+    av_assert0(cd != (iconv_t)-1);
+
     ret = av_new_packet(&tmp, inl * UTF8_MAX_BYTES);
     if (ret < 0)
         goto end;