Date: Sat, 15 Feb 2025 10:38:52 -0800
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>,
	Andrew Dunstan <andrew@dunslane.net>, pgsql-hackers@postgresql.org,
	pgsql@j-davis.com
Subject: Re: Decision by Monday: PQescapeString() vs. encoding violation
Message-ID: <20250215183852.6f@rfd.leadboat.com>
References: <20250215012712.45@rfd.leadboat.com>
 <f9e447e5-d94f-45f0-8ccb-2570cd4419c9@dunslane.net>
 <77190.1739642883@sss.pgh.pa.us>
 <7vmfuavitlhjgruidll67y5cd4s3m66m4x7c4yxezkxlo4dxvv@4haeja2xdn2q>
 <80374.1739644510@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <80374.1739644510@sss.pgh.pa.us>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250215183852.6f%40rfd.leadboat.com>
Precedence: bulk

On Sat, Feb 15, 2025 at 01:23:51PM -0500, Andres Freund wrote:
> On 2025-02-15 13:08:03 -0500, Tom Lane wrote:
> > I studied the v3 patch a little and realized that it only fixes the
> > behavior for the case of a complete-but-invalid multibyte character.
> > If we have an incomplete character at the end of the string, the
> > whole thing still gets dropped.  Surely that's not what we want if
> > we are going to adopt this behavior.

Thanks for doing that.

> Good catch! The different behaviour made sense when we were dropping the
> entire multi-byte character, but not anymore.
> 
> 
> > Here's a v4 that fixes that.  As a bonus, we can get rid of
> > duplicative coding since the "incomplete" and "invalid" cases
> > are now treated identically.
> 
> > One minor point is that the error messages from PQescapeStringInternal
> > no longer distinguish "incomplete" from "invalid".  I don't find that
> > to be a terribly useful distinction, so that's fine with me.  But if
> > someone feels that's important to preserve, we could make it do
> > something like
> 
> I think there's some value in the distinction, because incomplete characters
> can happen a lot more easily due to truncating longer strings. I don't know if
> it's worth the code complexity though.

While I agree it doesn't feel like a terribly-important distinction, I'd
definitely keep that distinction today rather than drop it with so little
notice.  (I'd feel fine about dropping it in a major release.  I'd probably
not drop it in any minor release, but especially not this close to wrap.)

On Sat, Feb 15, 2025 at 01:35:10PM -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > It seems that nobody is arguing against the "just skip one byte" behaviour, so
> > I'm inclined to push this fairly soon, even if Noah's "24 hours" haven't quite
> > elapsed.  A few more cycles in the buildfarm wouldn't hurt.

Works for me; we can always revert in the event of further discussion.  For
what it's worth, I didn't intend this 24hr as a hard period for gating any
particular action.

> Agreed.  I thought there would be more discussion, but it seems
> nobody really objects to changing this.
> 
> The other thing that was discussed in the security thread was
> modifying PQescapeStringInternal and PQescapeInternal to produce
> no more than one complaint about invalid multibyte characters,
> on the grounds that input that's just plain in some other encoding
> would otherwise produce a ton of repetitive messages.  That seems
> trivial enough to mechanize with a bool already_complained flag,
> so I think we should incorporate that refinement while we're here.

+1