From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <ffmpeg-devel-bounces@ffmpeg.org>
Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100])
	by master.gitmailbox.com (Postfix) with ESMTPS id 8145C4E14F
	for <ffmpegdev@gitmailbox.com>; Mon, 28 Apr 2025 21:47:33 +0000 (UTC)
Received: from [127.0.1.1] (localhost [127.0.0.1])
	by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 9BA65689BC3;
	Tue, 29 Apr 2025 00:47:28 +0300 (EEST)
Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com
 [209.85.128.46])
 by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C93CE687CDD
 for <ffmpeg-devel@ffmpeg.org>; Tue, 29 Apr 2025 00:47:21 +0300 (EEST)
Received: by mail-wm1-f46.google.com with SMTP id
 5b1f17b1804b1-43d0c18e84eso25607985e9.3
 for <ffmpeg-devel@ffmpeg.org>; Mon, 28 Apr 2025 14:47:21 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=gmail.com; s=20230601; t=1745876841; x=1746481641; darn=ffmpeg.org;
 h=user-agent:in-reply-to:content-disposition:mime-version:references
 :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc
 :subject:date:message-id:reply-to;
 bh=rbOeKDaWtXPBZrQ1QUFZOqdwA3ph3sJQTFWBNgtKJN4=;
 b=K066h837zghPNtukQyI6ARIo0E8BN7MbhZE14t9ddsiTowJPjoYGevDPrnRltOiXuF
 i8L2K5e8jofNaEk3y0x1JvqlGVTutLj5hqbdVOCOMQ4mOa0wnOnaEipRP/B3f1Wu6G2g
 ZV30LasbqNWT32u5eAjx7IwkVfm7eCEqiF04cS7uwJ8lY6bXHHKGcI2/+QpmwfgEOztv
 x/gAHkLPMs4QQXXxM5XzQck8gwGM62o3v6gt7/JnL/lRnH1HTF92e2M27fjdoKukb5kJ
 QWHH93zKFWfdRJKpi8FG8XoLA0l0Nssz+qHZtdSwGiT5b0kZojE1JrzZCwSdXYrcvaRg
 iGbw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20230601; t=1745876841; x=1746481641;
 h=user-agent:in-reply-to:content-disposition:mime-version:references
 :mail-followup-to:message-id:subject:cc:to:from:date
 :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;
 bh=rbOeKDaWtXPBZrQ1QUFZOqdwA3ph3sJQTFWBNgtKJN4=;
 b=Zjger+bQIJUcVILMrw4yHsbWRWlk0fNKRgrHKW/xmTsX0F2N+yYeeYBPVUM6lmT0yu
 L1IYmohCq7PXrD1w38h1IMfY/9xBRso/C6D4yWjnT1bTh5u51fJ6FZbWccBJyyH3/GVE
 Cxn7iWmDagdKVUyKjtmA4VLWzzAVEQvtSyUjNNHh7s5A60aDIRD9z1PEgST0wQC7qof8
 4itSWwaXkfOl6DDtxUDp8WfZRtidqkCs7Hn6cN5NXcHATUpp123DL7snvRufVeYsZaSu
 BExxzwlefdyYsflRkiskOXXAwjHmJrjtRKVO2jWfxnqnivgVBDGtfOagtGAMDhyRjlp7
 u6dA==
X-Gm-Message-State: AOJu0Yyg2O4ZmT0rkex4BwjOPmmn2+VtvUnIVfcy2wAppeCzXe1c/elE
 ro7H/8GecOnwwEkU74eWZPHVA1sfW+PiLwmjNWZlVIF8US6ahMXOqJAUOw==
X-Gm-Gg: ASbGnctURRvLZSp+ovl667HaDw0+Vg61thnrBiCrBKJPTQsNrhpAZgDHmqUr+Qp0FgK
 ATJJsf3vJb3a+GZUsycgV0VT7KaMM/XYF/kwinP3xoTjtuup5ncyiI/KFpZeqelE9m/TzKF9a4C
 1zgG/8kJn/4Odo3L+cnRqmak5fF9cca9PoubatY/DT2odmoYr0TyYEo898lvcLm9ERJCfEcQuLk
 NDkRsPlXan61QhvgmLhlENxkB4R2nuz1RgCkMwgPr0yOtyZWrc+GA60jXHrG/Vsg8YHLECcEvwj
 6czUm/gqVjh/jgMCB7GTn9VQJ64etTZEzja8PmDuFToFKPshm2mt6KhZT6VgIokfBKFFpnUiV7t
 oQPUd
X-Google-Smtp-Source: AGHT+IHSY0pcxTBwYC4T+j2jKWGD4JUeCmA9ZA60PioLASk3WzNuksIPBHUAm0fMkqqcnveGp4rfDQ==
X-Received: by 2002:a05:600c:1f86:b0:43d:17f1:2640 with SMTP id
 5b1f17b1804b1-441ac88df46mr6116495e9.26.1745876840456; 
 Mon, 28 Apr 2025 14:47:20 -0700 (PDT)
Received: from mariano (dynamic-adsl-84-220-189-10.clienti.tiscali.it.
 [84.220.189.10]) by smtp.gmail.com with ESMTPSA id
 5b1f17b1804b1-4409d2ac26dsm168786105e9.21.2025.04.28.14.47.19
 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
 Mon, 28 Apr 2025 14:47:20 -0700 (PDT)
Received: by mariano (Postfix, from userid 1000)
 id 8BCA5BFCE8; Mon, 28 Apr 2025 23:47:18 +0200 (CEST)
Date: Mon, 28 Apr 2025 23:47:18 +0200
From: Stefano Sabatini <stefasab@gmail.com>
To: "softworkz ." <softworkz@hotmail.com>
Message-ID: <aA/3Zibi2BmYgR2o@mariano>
Mail-Followup-To: "softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>,
	FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
References: <pull.66.v6.ffstaging.FFmpeg.1745457192.ffmpegagent@gmail.com>
 <pull.66.v7.ffstaging.FFmpeg.1745623868.ffmpegagent@gmail.com>
 <1a4044ba238e9cdd13c8db56a344387748f64633.1745623868.git.ffmpegagent@gmail.com>
 <aA/dbPipiEq0ZPw1@mariano>
 <DM8P223MB03654FE01A37E77E0968DEC7BA812@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM>
 <aA/j8udHPaUshX70@mariano>
 <DM8P223MB036524D7359B4E45D4DCFBC0BA812@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <DM8P223MB036524D7359B4E45D4DCFBC0BA812@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM>
User-Agent: Mutt/2.1.4 (2021-12-11)
Subject: Re: [FFmpeg-devel] [PATCH v7 02/13] fftools/textformat: Apply
 quality improvements
X-BeenThere: ffmpeg-devel@ffmpeg.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: FFmpeg development discussions and patches <ffmpeg-devel.ffmpeg.org>
List-Unsubscribe: <https://ffmpeg.org/mailman/options/ffmpeg-devel>,
 <mailto:ffmpeg-devel-request@ffmpeg.org?subject=unsubscribe>
List-Archive: <https://ffmpeg.org/pipermail/ffmpeg-devel>
List-Post: <mailto:ffmpeg-devel@ffmpeg.org>
List-Help: <mailto:ffmpeg-devel-request@ffmpeg.org?subject=help>
List-Subscribe: <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>,
 <mailto:ffmpeg-devel-request@ffmpeg.org?subject=subscribe>
Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: ffmpeg-devel-bounces@ffmpeg.org
Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>
Archived-At: <https://master.gitmailbox.com/ffmpegdev/aA%2F3Zibi2BmYgR2o@mariano/>
List-Archive: <https://master.gitmailbox.com/ffmpegdev/>
List-Post: <mailto:ffmpegdev@gitmailbox.com>

On date Monday 2025-04-28 20:40:37 +0000, softworkz . wrote:
[...]
> > > > >      /* 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.

So I checked the code, a valid UTF8 sequence should be no longer than
six bytes, and reading the implementation of av_utf8_decode I could
not quickly find out the maximum possible size of an invalid sequence,
so I guess your argument has a point in case of crafted sequences -
although possibly we might compute the maximum size and it might be
small (assuming there are no mistakes in the implementation and no
regressions are introduced). Even in that case, the worst it might
happen would be that the sequence is truncated, so this should have no
security implications anyway - while allocation might fail (in case of
a very looong crafted string), leading again to truncation.

That said, I'm not against this change but it should be moved to a
dedicated change.
_______________________________________________
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".