public inbox for [email protected]  

help / color / mirror / Atom feed

From: Alexander Lakhin <[email protected]>
To: Jeff Davis <[email protected]>
To: Tom Lane <[email protected]>
Cc: Daniel Verite <[email protected]>
Cc: Andrew Gierth <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Sandro Santilli <[email protected]>
Cc: Regina Obe <[email protected]>
Cc: [email protected]
Subject: Re: Order changes in PG16 since ICU introduction
Date: Tue, 16 May 2023 19:00:00 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Hi Jeff,

16.05.2023 00:03, Jeff Davis wrote:
> On Sat, 2023-05-13 at 13:00 +0300, Alexander Lakhin wrote:
>> On the current master (after 455f948b0, and before f7faa9976, of
>> course)
>> I get an ASAN-detected failure with the following query:
>> CREATE COLLATION col (provider = icu, locale = '123456789012');
>>
> Thank you for the report!
>
> ICU source specifically says:
>
>    /**
>     * Useful constant for the maximum size of the language
>       part of a locale ID.
>     * (including the terminating NULL).
>     * @stable ICU 2.0
>     */
>    #define ULOC_LANG_CAPACITY 12
>
> So the fact that it returning success without nul-terminating the
> result is an ICU bug in my opinion, and I reported it here:
>
> https://unicode-org.atlassian.net/browse/ICU-22394
>
> Unfortunately that means we need to be a bit more paranoid and always
> check for that warning, even if we have a preallocated buffer of the
> "correct" size. It also means that both U_STRING_NOT_TERMINATED_WARNING
> and U_BUFFER_OVERFLOW_ERROR will be user-facing errors (potentially
> scary), unless we check for those errors each time and report specific
> errors for them.
>
> Another approach is to always check the length and loop using dynamic
> allocation so that we never overflow (and forget about any static
> buffers). That seems like overkill given that the problem case is
> obviously invalid input; I think as long as we catch it and throw an
> ERROR it's fine. But I can do this if others think it's worthwhile.
>
> Patch attached. It just checks for the U_STRING_NOT_TERMINATED_WARNING
> in a few places and turns it into an ERROR. It also cleans up the loop
> for uloc_getLanguageTag() to check for U_STRING_NOT_TERMINATED_WARNING
> rather than (U_SUCCESS(status) && len >= buflen).

I'm not sure about the proposed change in icu_from_uchar(). It seems that
len_result + 1 bytes should always be enough for the result string terminated
with NUL. If that's not true (we want to protect from some ICU bug here),
then the change should be backpatched?

Best regards,
Alexander

view thread (12+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Order changes in PG16 since ICU introduction
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox