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

[James Almer]

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 at 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.