[FFmpeg-devel] [PATCH v5 02/14] fftools/textformat: Apply quality improvements

[softworkz <ffmpegagent@gmail.com>]

From: softworkz <softworkz@hotmail.com>

Perform multiple improvements to increase code robustness.
In particular:
- favor unsigned counters for loops
- add missing checks
- avoid possibly leaks
- move variable declarations to inner scopes when feasible
- provide explicit type-casting when needed

Signed-off-by: softworkz <softworkz@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);
+
     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);
+                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;
+
     tctx->level++;
     av_assert0(tctx->level < SECTION_MAX_NB_LEVELS);
 
@@ -268,6 +274,9 @@ void avtext_print_section_header(AVTextFormatContext *tctx, const void *data, in
 
 void avtext_print_section_footer(AVTextFormatContext *tctx)
 {
+    if (tctx->level < 0 || tctx->level >= SECTION_MAX_NB_LEVELS)
+        return;
+
     int section_id = tctx->section[tctx->level]->id;
     int parent_section_id = tctx->level
         ? tctx->section[tctx->level - 1]->id
@@ -285,7 +294,11 @@ void avtext_print_section_footer(AVTextFormatContext *tctx)
 
 void avtext_print_integer(AVTextFormatContext *tctx, const char *key, int64_t val)
 {
-    const struct AVTextFormatSection *section = tctx->section[tctx->level];
+    const AVTextFormatSection *section;
+
+    av_assert0(key && tctx->level >= 0 && tctx->level < SECTION_MAX_NB_LEVELS);
+
+    section = tctx->section[tctx->level];
 
     if (section->show_all_entries || av_dict_get(section->entries_to_show, key, NULL, 0)) {
         tctx->formatter->print_integer(tctx, key, val);
@@ -354,17 +367,18 @@ struct unit_value {
     const char *unit;
 };
 
-static char *value_string(AVTextFormatContext *tctx, char *buf, int buf_size, struct unit_value uv)
+static char *value_string(const AVTextFormatContext *tctx, char *buf, int buf_size, struct unit_value uv)
 {
     double vald;
-    int64_t vali;
+    int64_t vali = 0;
     int show_float = 0;
 
     if (uv.unit == unit_second_str) {
         vald = uv.val.d;
         show_float = 1;
     } else {
-        vald = vali = uv.val.i;
+        vald = (double)uv.val.i;
+        vali = uv.val.i;
     }
 
     if (uv.unit == unit_second_str && tctx->use_value_sexagesimal_format) {
@@ -383,17 +397,17 @@ static char *value_string(AVTextFormatContext *tctx, char *buf, int buf_size, st
             int64_t index;
 
             if (uv.unit == unit_byte_str && tctx->use_byte_value_binary_prefix) {
-                index = (int64_t) (log2(vald)) / 10;
-                index = av_clip(index, 0, FF_ARRAY_ELEMS(si_prefixes) - 1);
+                index = (int64_t)(log2(vald) / 10);
+                index = av_clip64(index, 0, FF_ARRAY_ELEMS(si_prefixes) - 1);
                 vald /= si_prefixes[index].bin_val;
                 prefix_string = si_prefixes[index].bin_str;
             } else {
-                index = (int64_t) (log10(vald)) / 3;
-                index = av_clip(index, 0, FF_ARRAY_ELEMS(si_prefixes) - 1);
+                index = (int64_t)(log10(vald) / 3);
+                index = av_clip64(index, 0, FF_ARRAY_ELEMS(si_prefixes) - 1);
                 vald /= si_prefixes[index].dec_val;
                 prefix_string = si_prefixes[index].dec_str;
             }
-            vali = vald;
+            vali = (int64_t)vald;
         }
 
         if (show_float || (tctx->use_value_prefix && vald != (int64_t)vald))
@@ -421,9 +435,13 @@ void avtext_print_unit_int(AVTextFormatContext *tctx, const char *key, int value
 
 int avtext_print_string(AVTextFormatContext *tctx, const char *key, const char *val, int flags)
 {
-    const struct AVTextFormatSection *section = tctx->section[tctx->level];
+    const AVTextFormatSection *section;
     int ret = 0;
 
+    av_assert0(key && val && tctx->level >= 0 && tctx->level < SECTION_MAX_NB_LEVELS);
+
+    section = tctx->section[tctx->level];
+
     if (tctx->show_optional_fields == SHOW_OPTIONAL_FIELDS_NEVER ||
         (tctx->show_optional_fields == SHOW_OPTIONAL_FIELDS_AUTO
             && (flags & AV_TEXTFORMAT_PRINT_STRING_OPTIONAL)
@@ -465,12 +483,11 @@ void avtext_print_rational(AVTextFormatContext *tctx, const char *key, AVRationa
 void avtext_print_time(AVTextFormatContext *tctx, const char *key,
                        int64_t ts, const AVRational *time_base, int is_duration)
 {
-    char buf[128];
-
     if ((!is_duration && ts == AV_NOPTS_VALUE) || (is_duration && ts == 0)) {
         avtext_print_string(tctx, key, "N/A", AV_TEXTFORMAT_PRINT_STRING_OPTIONAL);
     } else {
-        double d = ts * av_q2d(*time_base);
+        char buf[128];
+        double d = av_q2d(*time_base) * ts;
         struct unit_value uv;
         uv.val.d = d;
         uv.unit = unit_second_str;
@@ -491,7 +508,8 @@ void avtext_print_data(AVTextFormatContext *tctx, const char *name,
                        const uint8_t *data, int size)
 {
     AVBPrint bp;
-    int offset = 0, l, i;
+    unsigned offset = 0;
+    int l, i;
 
     av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
     av_bprintf(&bp, "\n");
@@ -518,25 +536,29 @@ void avtext_print_data(AVTextFormatContext *tctx, const char *name,
 void avtext_print_data_hash(AVTextFormatContext *tctx, const char *name,
                             const uint8_t *data, int size)
 {
-    char *p, buf[AV_HASH_MAX_SIZE * 2 + 64] = { 0 };
+    char buf[AV_HASH_MAX_SIZE * 2 + 64] = { 0 };
+    int len;
 
     if (!tctx->hash)
         return;
 
     av_hash_init(tctx->hash);
     av_hash_update(tctx->hash, data, size);
-    snprintf(buf, sizeof(buf), "%s:", av_hash_get_name(tctx->hash));
-    p = buf + strlen(buf);
-    av_hash_final_hex(tctx->hash, p, buf + sizeof(buf) - p);
+    len = snprintf(buf, sizeof(buf), "%s:", av_hash_get_name(tctx->hash));
+    av_hash_final_hex(tctx->hash, (uint8_t *)&buf[len], (int)sizeof(buf) - len);
     avtext_print_string(tctx, name, buf, 0);
 }
 
 void avtext_print_integers(AVTextFormatContext *tctx, const char *name,
-                                  uint8_t *data, int size, const char *format,
-                                  int columns, int bytes, int offset_add)
+                           uint8_t *data, int size, const char *format,
+                           int columns, int bytes, int offset_add)
 {
     AVBPrint bp;
-    int offset = 0, l, i;
+    unsigned offset = 0;
+    int l, i;
+
+    if (!name || !data || !format || columns <= 0 || bytes <= 0)
+        return;
 
     av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
     av_bprintf(&bp, "\n");
@@ -602,12 +624,15 @@ int avtextwriter_context_open(AVTextWriterContext **pwctx, const AVTextWriter *w
     AVTextWriterContext *wctx;
     int ret = 0;
 
-    if (!(wctx = av_mallocz(sizeof(AVTextWriterContext)))) {
+    if (!pwctx || !writer)
+        return AVERROR(EINVAL);
+
+    if (!((wctx = av_mallocz(sizeof(AVTextWriterContext))))) {
         ret = AVERROR(ENOMEM);
         goto fail;
     }
 
-    if (!(wctx->priv = av_mallocz(writer->priv_size))) {
+    if (writer->priv_size && !((wctx->priv = av_mallocz(writer->priv_size)))) {
         ret = AVERROR(ENOMEM);
         goto fail;
     }
diff --git a/fftools/textformat/avtextformat.h b/fftools/textformat/avtextformat.h
index c598af3450..aea691f351 100644
--- a/fftools/textformat/avtextformat.h
+++ b/fftools/textformat/avtextformat.h
@@ -21,9 +21,7 @@
 #ifndef FFTOOLS_TEXTFORMAT_AVTEXTFORMAT_H
 #define FFTOOLS_TEXTFORMAT_AVTEXTFORMAT_H
 
-#include <stddef.h>
 #include <stdint.h>
-#include "libavutil/attributes.h"
 #include "libavutil/dict.h"
 #include "libavformat/avio.h"
 #include "libavutil/bprint.h"
@@ -103,7 +101,7 @@ struct AVTextFormatContext {
     unsigned int nb_item_type[SECTION_MAX_NB_LEVELS][SECTION_MAX_NB_SECTIONS];
 
     /** section per each level */
-    const struct AVTextFormatSection *section[SECTION_MAX_NB_LEVELS];
+    const AVTextFormatSection *section[SECTION_MAX_NB_LEVELS];
     AVBPrint section_pbuf[SECTION_MAX_NB_LEVELS]; ///< generic print buffer dedicated to each section,
                                                   ///  used by various formatters
 
@@ -124,7 +122,7 @@ struct AVTextFormatContext {
 #define AV_TEXTFORMAT_PRINT_STRING_VALIDATE 2
 
 int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *formatter, AVTextWriterContext *writer_context, const char *args,
-                        const struct AVTextFormatSection *sections, int nb_sections,
+                        const AVTextFormatSection *sections, int nb_sections,
                         int show_value_unit,
                         int use_value_prefix,
                         int use_byte_value_binary_prefix,
diff --git a/fftools/textformat/tf_default.c b/fftools/textformat/tf_default.c
index 2c5047eafd..ad97173b0b 100644
--- a/fftools/textformat/tf_default.c
+++ b/fftools/textformat/tf_default.c
@@ -68,9 +68,10 @@ DEFINE_FORMATTER_CLASS(default);
 /* lame uppercasing routine, assumes the string is lower case ASCII */
 static inline char *upcase_string(char *dst, size_t dst_size, const char *src)
 {
-    int i;
+    unsigned i;
+
     for (i = 0; src[i] && i < dst_size - 1; i++)
-        dst[i] = av_toupper(src[i]);
+        dst[i] = (char)av_toupper(src[i]);
     dst[i] = 0;
     return dst;
 }
@@ -106,6 +107,9 @@ static void default_print_section_footer(AVTextFormatContext *wctx)
     const struct AVTextFormatSection *section = wctx->section[wctx->level];
     char buf[32];
 
+    if (!section)
+        return;
+
     if (def->noprint_wrappers || def->nested_section[wctx->level])
         return;
 
diff --git a/fftools/textformat/tf_ini.c b/fftools/textformat/tf_ini.c
index 88add0819a..dd77d0e8bf 100644
--- a/fftools/textformat/tf_ini.c
+++ b/fftools/textformat/tf_ini.c
@@ -91,7 +91,7 @@ static char *ini_escape_str(AVBPrint *dst, const char *src)
             /* fallthrough */
         default:
             if ((unsigned char)c < 32)
-                av_bprintf(dst, "\\x00%02x", c & 0xff);
+                av_bprintf(dst, "\\x00%02x", (unsigned char)c);
             else
                 av_bprint_chars(dst, c, 1);
             break;
diff --git a/fftools/textformat/tf_json.c b/fftools/textformat/tf_json.c
index b61d3740c6..50c3d90440 100644
--- a/fftools/textformat/tf_json.c
+++ b/fftools/textformat/tf_json.c
@@ -80,13 +80,18 @@ static const char *json_escape_str(AVBPrint *dst, const char *src, void *log_ctx
     static const char json_subst[]  = { '"', '\\',  'b',  'f',  'n',  'r',  't', 0 };
     const char *p;
 
+    if (!src) {
+        av_log(log_ctx, AV_LOG_WARNING, "Cannot escape NULL string, returning NULL\n");
+        return NULL;
+    }
+
     for (p = src; *p; p++) {
         char *s = strchr(json_escape, *p);
         if (s) {
             av_bprint_chars(dst, '\\', 1);
             av_bprint_chars(dst, json_subst[s - json_escape], 1);
         } else if ((unsigned char)*p < 32) {
-            av_bprintf(dst, "\\u00%02x", *p & 0xff);
+            av_bprintf(dst, "\\u00%02x", (unsigned char)*p);
         } else {
             av_bprint_chars(dst, *p, 1);
         }
@@ -100,11 +105,10 @@ static void json_print_section_header(AVTextFormatContext *wctx, const void *dat
 {
     JSONContext *json = wctx->priv;
     AVBPrint buf;
-    const struct AVTextFormatSection *section = wctx->section[wctx->level];
-    const struct AVTextFormatSection *parent_section = wctx->level ?
-        wctx->section[wctx->level-1] : NULL;
+    const AVTextFormatSection *section = wctx->section[wctx->level];
+    const AVTextFormatSection *parent_section = wctx->level ? wctx->section[wctx->level - 1] : NULL;
 
-    if (wctx->level && wctx->nb_item[wctx->level-1])
+    if (wctx->level && wctx->nb_item[wctx->level - 1])
         writer_put_str(wctx, ",\n");
 
     if (section->flags & AV_TEXTFORMAT_SECTION_FLAG_IS_WRAPPER) {
@@ -185,8 +189,7 @@ static void json_print_str(AVTextFormatContext *wctx, const char *key, const cha
 static void json_print_int(AVTextFormatContext *wctx, const char *key, int64_t value)
 {
     JSONContext *json = wctx->priv;
-    const struct AVTextFormatSection *parent_section = wctx->level ?
-        wctx->section[wctx->level-1] : NULL;
+    const AVTextFormatSection *parent_section = wctx->level ? wctx->section[wctx->level - 1] : NULL;
     AVBPrint buf;
 
     if (wctx->nb_item[wctx->level] || (parent_section && parent_section->flags & AV_TEXTFORMAT_SECTION_FLAG_NUMBERING_BY_TYPE))
diff --git a/fftools/textformat/tf_xml.c b/fftools/textformat/tf_xml.c
index befb39246d..28abfc6400 100644
--- a/fftools/textformat/tf_xml.c
+++ b/fftools/textformat/tf_xml.c
@@ -18,10 +18,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
-#include <limits.h>
-#include <stdarg.h>
 #include <stdint.h>
-#include <stdio.h>
 #include <string.h>
 
 #include "avtextformat.h"
diff --git a/fftools/textformat/tw_avio.c b/fftools/textformat/tw_avio.c
index 6034f74ec9..29889598bb 100644
--- a/fftools/textformat/tw_avio.c
+++ b/fftools/textformat/tw_avio.c
@@ -23,6 +23,7 @@
 #include <string.h>
 
 #include "avtextwriters.h"
+#include "libavutil/avassert.h"
 
 #include "libavutil/error.h"
 
@@ -53,7 +54,7 @@ static void io_w8(AVTextWriterContext *wctx, int b)
 static void io_put_str(AVTextWriterContext *wctx, const char *str)
 {
     IOWriterContext *ctx = wctx->priv;
-    avio_write(ctx->avio_context, str, strlen(str));
+    avio_write(ctx->avio_context, (const unsigned char *)str, (int)strlen(str));
 }
 
 static void io_printf(AVTextWriterContext *wctx, const char *fmt, ...)
@@ -78,10 +79,14 @@ const AVTextWriter avtextwriter_avio = {
 
 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;
 
-
     ret = avtextwriter_context_open(pwctx, &avtextwriter_avio);
     if (ret < 0)
         return ret;
@@ -103,6 +108,8 @@ int avtextwriter_create_file(AVTextWriterContext **pwctx, const char *output_fil
 
 int avtextwriter_create_avio(AVTextWriterContext **pwctx, AVIOContext *avio_ctx, int close_on_uninit)
 {
+    av_assert0(avio_ctx);
+
     IOWriterContext *ctx;
     int ret;
 
-- 
ffmpeg-codebot

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