Message-ID: <53085e53-7148-1707-1643-71a4969728cf@gmail.com>
Date: Tue, 11 Jun 2024 11:00:01 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #18483: Segmentation fault in tests modules
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, a.kozhemyakin@postgrespro.ru,
 pgsql-bugs@lists.postgresql.org
References: <18483-30bfff42de238000@postgresql.org>
 <Zl5GS6SHBXRAkl_c@paquier.xyz> <219049.1717792376@sss.pgh.pa.us>
 <ZmOJ3jfeltZQnN_M@paquier.xyz>
 <439c9310-1d71-f2c6-b213-a9a2ea732ba9@gmail.com>
 <336833.1717859243@sss.pgh.pa.us>
 <250a21e5-d677-6b2a-2692-cd4233785e37@gmail.com>
 <ZmaUKZOmjv-L9EvK@paquier.xyz>
 <d959bd09-3bce-6a5e-7ae0-dff761f8ad81@gmail.com>
 <ZmeH9lDIUWTsJ-Uk@paquier.xyz>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <ZmeH9lDIUWTsJ-Uk@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/53085e53-7148-1707-1643-71a4969728cf%40gmail.com>
Precedence: bulk

11.06.2024 02:10, Michael Paquier wrote:
> So, valgrind is noisy here because the incomplete byte sequence
> \x8b passed as input of the function after a valid byte sequence
> causes pg_encoding_mblen() to return 2, which is 1 byte passed the end
> of the actual string.  report_invalid_encoding() is careful to not
> show any data passed in its input.  I am not really sure that this is
> worth bothering with here, TBH.  As far as I know this is related to
> the fact that we want test_enc_conversion() to be flexible enough to
> accept any kind of input, working as a cheap shortcut to call these
> internal encoding routines.  What do you think?

I think that maybe this case illustrates an API shortcoming. The
report_invalid_encoding function takes len as an argument, but then passes
mbstr as a null-terminated string to pg_encoding_mblen():
/*
  * report_invalid_encoding: complain about invalid multibyte character
  *
  * note: len is remaining length of string, not length of character;
  * len must be greater than zero, as we always examine the first byte.
  */
void
report_invalid_encoding(int encoding, const char *mbstr, int len)
{
     int         l = pg_encoding_mblen(encoding, mbstr);
...

Maybe the comment should say something about null-termination expected?

I could not find other report_invalid_encoding() callers that pass not
null-terminated strings to it,  so maybe leave test_enc_conversion() as-is,
just keep in mind that it is an imperfect example of
report_invalid_encoding() usage and it might require some modifications
for fuzz testing.

Best regards,
Alexander