> -----Original Message-----
> From: Stefano Sabatini <stefa...@gmail.com>
> Sent: Montag, 28. April 2025 22:24
> To: softworkz . <softwo...@hotmail.com>
> Cc: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v7 02/13] fftools/textformat: Apply quality
> improvements
>
> 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?
Are we sure that it's not? Because unless we are 100% sure, we need
to check the return value and when that check indicates an overflow
of the buffer - what then? How to remedy?
The check for the return value alone - even without any remediation -
is more complicated code (more lines) than a single line with
av_bprint_finalize().
That's my general rationale behind favoring _UNLIMITED.
There are more cases where I have that, and where it's really relevant
but for this one, I'll just remove the change, it's not worth the
discussion.
Thank you
sww
_______________________________________________
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".
