On date Monday 2025-04-28 20:05:25 +0000, softworkz . wrote:
>
>
> > -----Original Message-----
> > From: Stefano Sabatini <stefa...@gmail.com>
> > Sent: Montag, 28. April 2025 21:56
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Cc: softworkz <softwo...@hotmail.com>
> > Subject: Re: [FFmpeg-devel] [PATCH v7 02/13] fftools/textformat: Apply
> > quality
> > improvements
> >
> > On date Friday 2025-04-25 23:30:57 +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 possible leaks
> > > - move variable declarations to inner scopes when feasible
> > > - provide explicit type-casting when needed
> > >
> > > Signed-off-by: softworkz <softwo...@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?
>
> OK.
>
>
> >
> > > +
> > > 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
>
> The dynamic allocation does not happen until the buffer content size
> does not fit anymore into the stack-alloced memory.
Yes, but is this ever happening? Since in this case the size of the
buffer is guaranteed to be short (since it's the max return size of
the UTF8 decode function). It won't hurt, but it won't help either so
I'd rather keep as is and fix the indent weirdness (the , in place of
; for instance) and the ret.
> >
> > > + 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.
>
> Let's discuss this in the other thread (Shaping...).
> Did you see my recent reply?
Yes, ok.
_______________________________________________
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".
