[FFmpeg-devel] [PATCH v7 02/13] fftools/textformat: Apply quality improvements

[Stefano Sabatini]

On date Friday 2025-04-25 23:30:57 +0000, softworkz wrote:
> From: softworkz <softworkz at hotmail.com>
> 
> Perform multiple improvements to increase code robustness.
> In particular:
> - favor unsigned counters for loops
> - add missing checks
> - avoid possible leaks
> - move variable declarations to inner scopes when feasible
> - provide explicit type-casting when needed
> 
> Signed-off-by: softworkz <softworkz at hotmail.com>
> ---
>  fftools/textformat/avtextformat.c | 85 ++++++++++++++++++++-----------
>  fftools/textformat/avtextformat.h |  6 +--
>  fftools/textformat/tf_default.c   |  8 ++-
>  fftools/textformat/tf_ini.c       |  2 +-
>  fftools/textformat/tf_json.c      | 17 ++++---
>  fftools/textformat/tf_xml.c       |  3 --
>  fftools/textformat/tw_avio.c      | 11 +++-
>  7 files changed, 83 insertions(+), 49 deletions(-)
> 
> diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
> index 74d179c516..1939a1f739 100644
> --- a/fftools/textformat/avtextformat.c
> +++ b/fftools/textformat/avtextformat.c
> @@ -93,9 +93,8 @@ static const AVClass textcontext_class = {
>  
>  static void bprint_bytes(AVBPrint *bp, const uint8_t *ubuf, size_t ubuf_size)
>  {
> -    int i;
>      av_bprintf(bp, "0X");
> -    for (i = 0; i < ubuf_size; i++)
> +    for (unsigned i = 0; i < ubuf_size; i++)
>          av_bprintf(bp, "%02X", ubuf[i]);
>  }
>  
> @@ -137,6 +136,9 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
>      AVTextFormatContext *tctx;
>      int i, ret = 0;
>  

> +    if (!ptctx || !formatter)
> +        return AVERROR(EINVAL);

assert0?

> +
>      if (!(tctx = av_mallocz(sizeof(AVTextFormatContext)))) {
>          ret = AVERROR(ENOMEM);
>          goto fail;
> @@ -209,25 +211,26 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
>                      av_log(NULL, AV_LOG_ERROR, " %s", n);
>                  av_log(NULL, AV_LOG_ERROR, "\n");
>              }
> -            return ret;
> +            goto fail;
>          }
>  
>      /* validate replace string */
>      {
> -        const uint8_t *p = tctx->string_validation_replacement;
> -        const uint8_t *endp = p + strlen(p);
> +        const uint8_t *p = (uint8_t *)tctx->string_validation_replacement;
> +        const uint8_t *endp = p + strlen((const char *)p);
>          while (*p) {
>              const uint8_t *p0 = p;
>              int32_t code;
>              ret = av_utf8_decode(&code, &p, endp, tctx->string_validation_utf8_flags);
>              if (ret < 0) {

>                  AVBPrint bp;
> -                av_bprint_init(&bp, 0, AV_BPRINT_SIZE_AUTOMATIC);
> +                av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
>                  bprint_bytes(&bp, p0, p - p0),
>                      av_log(tctx, AV_LOG_ERROR,
>                             "Invalid UTF8 sequence %s found in string validation replace '%s'\n",
>                             bp.str, tctx->string_validation_replacement);
> -                return ret;
> +                av_bprint_finalize(&bp, NULL);

I see no advantage in this, since it's adding dynamic allocation and
the need to free which was previously not needed

> +                goto fail;
>              }
>          }
>      }
> @@ -255,6 +258,9 @@ static const char unit_bit_per_second_str[] = "bit/s";
>  

>  void avtext_print_section_header(AVTextFormatContext *tctx, const void *data, int section_id)
>  {
> +    if (section_id < 0 || section_id >= tctx->nb_sections)
> +        return;

Given this is a check on the input data, we should probably make it
fail with a log message, meaning we should also check the return value
from the callee.

Given this is not a public API I'm not against the assert though.

[...]
>  int avtextwriter_create_file(AVTextWriterContext **pwctx, const char *output_filename)
>  {
> +    if (!output_filename || !output_filename[0]) {
> +        av_log(NULL, AV_LOG_ERROR, "The output_filename cannot be NULL or empty\n");
> +        return AVERROR(EINVAL);
> +    }
> +
>      IOWriterContext *ctx;
>      int ret;

nit here and below: move declarations before actual code for stylistic consistency

[...]