On date Tuesday 2025-04-22 21:55:31 +0000, softworkz wrote:
> From: softworkz <softwo...@hotmail.com>
>
> Perform multiple improvements to increase code robustness.
> In particular:
> - favor unsigned counters for loops
> - add missing checks
> - avoid possibly leaks
my typo: possible leaks
> - move variable declarations to inner scopes when feasible
> - provide explicit type-casting when needed
>
> Signed-off-by: softworkz <softwo...@hotmail.com>
> ---
General nit about headline caseing: from the log most commit use
all lowercase in headline (I personally only use that form and at some
point everybody was using that).
> 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),
Is this really needed? AUTOMATIC should be faster since it will avoid
dynamic allocation and the need to finalize (although there is no
practical need for such optimization, this still seems the simplest
possible path). Besides, an UTF8 sequence cannot be longer than a few
bytes, so possibly av_utf8_decode cannot decode more than a few bytes.
> 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;
In general I see some confusion about how to deal with invalid
parameters (this is probably an issue with the whole codebase), since
it's not always clear what's the right thing to do - consider that as
a programmer unrecoverable mistake and assert or let the user handle
that or just silently perform a no-op. It's probably fine to go with
the current patch and later refine in case we elaborate a set of
guidelines to apply.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
