Date: Wed, 10 Nov 2021 15:33:29 +0900
From: Michael Paquier <michael@paquier.xyz>
To: postgresql@zr40.nl, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17277: write past chunk when calling normalize() on an
 empty string
Message-ID: <YYtnue3sc7EXIIwI@paquier.xyz>
References: <17277-0c527a373794e802@postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="m7Uxtz/q/X++95BS"
Content-Disposition: inline
In-Reply-To: <17277-0c527a373794e802@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/YYtnue3sc7EXIIwI%40paquier.xyz>
Precedence: bulk


--m7Uxtz/q/X++95BS
Content-Type: multipart/mixed; boundary="GTEqRihXCUTKhTC0"
Content-Disposition: inline


--GTEqRihXCUTKhTC0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Nov 09, 2021 at 09:55:08PM +0000, PG Bug reporting form wrote:
> When calling normalize(''), that is, on an empty string, a warning is
> raised: "problem in alloc set ExprContext: detected write past chunk end".

Well, direct callers of unicode_normalize_kc() in ~12 would have the
same problem because this code was not written with this case in mind
as far as I recall, after looking at the git history (60f11b8) as
pg_saslprep() does not allow the case of empty passwords.

> I believe this is due to an error in unicode_norm.c. In unicode_normalize=
(),
> when recompose is true (that is, when using NFC or NFKC normalization) the
> loop on line 498 will iterate once before checking count < decomp_size. W=
hen
> the input is an empty string, this would cause a write outside of the mem=
ory
> allocated for recomp_chars.

No, the code does not take the recomposition loop in this case, but
the initialization of target_pos to 1 would cause recomp_chars to be
written past its allocation position by one byte.

As there could be callers of unicode_normalize[_kc]() outside core,
I'd rather fix that at the source and patch unicode_norm.c.  One way
to do that would be to leave once you know that there is nothing to
decompose after the loop over decompose_code() and return decomp_chars
that would be set with an empty set of points, as per the attached.

There may be a point in issuing an error if there is an empty string,
though.  Another thing would be to consider if is_normalized() should
return false for an empty string, but we have considered empty strings
as normalized since this has been released:
=3D# SELECT '' IS NFD NORMALIZED;
 is_normalized
---------------
  t
(1 row)

That feels more natural this way.  Still, I can see some perl modules
that would return false for such a case, by the way.  The
normalization docs don't seem to mention that directly, except for the
stream-safe text format:=20
https://www.unicode.org/faq/normalization.html
https://unicode.org/reports/tr15/tr15-51.html
--
Michael

--GTEqRihXCUTKhTC0
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="unicode-norm-fix.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/common/unicode_norm.c b/src/common/unicode_norm.c
index 36ff2aab21..06bf921e45 100644
--- a/src/common/unicode_norm.c
+++ b/src/common/unicode_norm.c
@@ -439,6 +439,10 @@ unicode_normalize(UnicodeNormalizationForm form, const=
 pg_wchar *input)
 	decomp_chars[decomp_size] =3D '\0';
 	Assert(decomp_size =3D=3D current_size);
=20
+	/* Leave if there is nothing to decompose */
+	if (decomp_size =3D=3D 0)
+		return decomp_chars;
+
 	/*
 	 * Now apply canonical ordering.
 	 */
diff --git a/src/test/regress/expected/unicode.out b/src/test/regress/expec=
ted/unicode.out
index 2a1e903696..88aef0fe08 100644
--- a/src/test/regress/expected/unicode.out
+++ b/src/test/regress/expected/unicode.out
@@ -8,6 +8,18 @@ SELECT U&'\0061\0308bc' <> U&'\00E4bc' COLLATE "C" AS sani=
ty_check;
  t
 (1 row)
=20
+SELECT normalize('');
+ normalize=20
+-----------
+=20
+(1 row)
+
+SELECT is_normalized('');
+ is_normalized=20
+---------------
+ t
+(1 row)
+
 SELECT normalize(U&'\0061\0308\24D1c') =3D U&'\00E4\24D1c' COLLATE "C" AS =
test_default;
  test_default=20
 --------------
diff --git a/src/test/regress/sql/unicode.sql b/src/test/regress/sql/unicod=
e.sql
index ccfc6fa77a..07cd2851fc 100644
--- a/src/test/regress/sql/unicode.sql
+++ b/src/test/regress/sql/unicode.sql
@@ -5,6 +5,8 @@ SELECT getdatabaseencoding() <> 'UTF8' AS skip_test \gset
=20
 SELECT U&'\0061\0308bc' <> U&'\00E4bc' COLLATE "C" AS sanity_check;
=20
+SELECT normalize('');
+SELECT is_normalized('');
 SELECT normalize(U&'\0061\0308\24D1c') =3D U&'\00E4\24D1c' COLLATE "C" AS =
test_default;
 SELECT normalize(U&'\0061\0308\24D1c', NFC) =3D U&'\00E4\24D1c' COLLATE "C=
" AS test_nfc;
 SELECT normalize(U&'\00E4bc', NFC) =3D U&'\00E4bc' COLLATE "C" AS test_nfc=
_idem;

--GTEqRihXCUTKhTC0--

--m7Uxtz/q/X++95BS
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmGLZ7kACgkQnvQgOdby
QH2gBBAAnhbp4njSGWgI6L0Jqk8cY6Y0DXbuLGX++MyNZ2+K2L9IQwpzLF9+ULIv
Qn///m01ldziGe7UWK47+ZZfY+ld21pornaNacSGtCbMFYQzfU6GUKHv1b2RBNRp
FsIuPutQ4qSidOssb0KlGJoi9XHLkUc4LaxqI4UXx7luCzJMWu+QG2uqAM5DTpQw
crUU3zBDWYb6dUvmYNgz1Xasi/ysWmVPcusRQxPiyl65Nex0jOM8F7sUSZ4TXrrI
39wX4Ul81lKzIH4dSrHloKvjmjO0es4No/IVahpCCZcPnUFUuqMS+vk7jNYaZ1ky
jkkLR1iQA0R8sTu39ABveIXftkomKop/lzfkxPvRpdhlcwXkprGYpvlqLv9GYzx2
iVaS1HiDZiskfnF5tdB2i0plrHQa5+ZJVVl3SfoOo3he91sJsin7osF6GF+H404w
3NWMEnOL6JnwHlOlA1E00KJM8sSVhROc4W3+V9bZ3kt4onnlB75kYyRy4fZaDLF0
he99BH1hMWo4mShq1wRdvkDt+dbO8POX638/LqN9cGpEQocGGz/OrntOkTaWJK1p
fCf+xtWoZLeKZKpMy2rCeJWPt63ztV7fqVuKKmytWJb9t/vlIsG87Bl5dEfCE4Pw
xr+Jl1bvd/vTNKImvmBiDbNSs5/skMjO6FQjszOPMt77dVnu49A=
=6Wob
-----END PGP SIGNATURE-----

--m7Uxtz/q/X++95BS--