Make uselocale protection more consistent
This is a patch which implements an issue discussed in bug #17946[0]/messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.
[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
v1-0001-Make-uselocale-protection-more-consistent.patchtext/x-patch; charset=utf-8; name=v1-0001-Make-uselocale-protection-more-consistent.patchDownload
From 02a2cdb83405b5aecaec2af02e379a81161f8372 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Wed, 14 Jun 2023 11:36:00 -0500
Subject: [PATCH v1] Make uselocale protection more consistent
In ecpg, uselocale uses are protected by checking if HAVE_USELOCALE is
defined. Use the same check in pg_locale.c. Since HAVE_USELOCALE implies
HAVE_LOCALE_T, the code should be the same on _all_ platforms that
Postgres supports. Otherwise, I am sure there would have been a bug
report with pg_locale.c failing to build due to the system having
locale_t, but not uselocale.
---
src/backend/utils/adt/pg_locale.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 31e3b16ae0..3585afb298 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -2972,7 +2972,7 @@ wchar2char(char *to, const wchar_t *from, size_t tolen, pg_locale_t locale)
}
else
{
-#ifdef HAVE_LOCALE_T
+#ifdef HAVE_USELOCALE
#ifdef HAVE_WCSTOMBS_L
/* Use wcstombs_l for nondefault locales */
result = wcstombs_l(to, from, tolen, locale->info.lt);
@@ -2984,11 +2984,11 @@ wchar2char(char *to, const wchar_t *from, size_t tolen, pg_locale_t locale)
uselocale(save_locale);
#endif /* HAVE_WCSTOMBS_L */
-#else /* !HAVE_LOCALE_T */
- /* Can't have locale != 0 without HAVE_LOCALE_T */
+#else /* !HAVE_USELOCALE */
+ /* Can't have locale != 0 without HAVE_LOCALE_T, which HAVE_USELOCALE implies */
elog(ERROR, "wcstombs_l is not available");
result = 0; /* keep compiler quiet */
-#endif /* HAVE_LOCALE_T */
+#endif /* HAVE_USELOCALE */
}
return result;
@@ -3049,7 +3049,7 @@ char2wchar(wchar_t *to, size_t tolen, const char *from, size_t fromlen,
}
else
{
-#ifdef HAVE_LOCALE_T
+#ifdef HAVE_USELOCALE
#ifdef HAVE_MBSTOWCS_L
/* Use mbstowcs_l for nondefault locales */
result = mbstowcs_l(to, str, tolen, locale->info.lt);
@@ -3061,11 +3061,11 @@ char2wchar(wchar_t *to, size_t tolen, const char *from, size_t fromlen,
uselocale(save_locale);
#endif /* HAVE_MBSTOWCS_L */
-#else /* !HAVE_LOCALE_T */
- /* Can't have locale != 0 without HAVE_LOCALE_T */
+#else /* !HAVE_USELOCALE */
+ /* Can't have locale != 0 without HAVE_LOCALE_T, which HAVE_USELOCALE implies */
elog(ERROR, "mbstowcs_l is not available");
result = 0; /* keep compiler quiet */
-#endif /* HAVE_LOCALE_T */
+#endif /* HAVE_USELOCALE */
}
pfree(str);
--
Tristan Partin
Neon (https://neon.tech)
On 27.06.23 17:02, Tristan Partin wrote:
This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi
I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.
I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.
On Mon, Jul 3, 2023 at 6:13 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 27.06.23 17:02, Tristan Partin wrote:
This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi
I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.
I propose[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().
[1]: /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
On Mon Jul 3, 2023 at 1:24 AM CDT, Thomas Munro wrote:
On Mon, Jul 3, 2023 at 6:13 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 27.06.23 17:02, Tristan Partin wrote:
This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi
I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().[1] /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.
--
Tristan Partin
Neon (https://neon.tech)
On 03.07.23 08:24, Thomas Munro wrote:
I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().[1] /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
Ok, it appears your patch is imminent, so let's wait for that and see if
this patch here is still required afterwards.
On 03.07.23 15:21, Tristan Partin wrote:
I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.
The current code is structured
#ifdef HAVE_LOCALE_T
#ifdef HAVE_WCSTOMBS_L
wcstombs_l(...);
#else
uselocale(...);
#endif
#else
elog(ERROR);
#endif
If you just replace HAVE_LOCALE_T with HAVE_USELOCALE, then this would
penalize a platform that has wcstombs_l(), but not uselocale(). I think
the correct structure would be
#if defined(HAVE_WCSTOMBS_L)
wcstombs_l(...);
#elif defined(HAVE_USELOCALE)
uselocale(...);
#else
elog(ERROR);
#endif
On Mon Jul 3, 2023 at 9:21 AM CDT, Peter Eisentraut wrote:
On 03.07.23 15:21, Tristan Partin wrote:
I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.The current code is structured
#ifdef HAVE_LOCALE_T
#ifdef HAVE_WCSTOMBS_L
wcstombs_l(...);
#else
uselocale(...);
#endif
#else
elog(ERROR);
#endifIf you just replace HAVE_LOCALE_T with HAVE_USELOCALE, then this would
penalize a platform that has wcstombs_l(), but not uselocale(). I think
the correct structure would be#if defined(HAVE_WCSTOMBS_L)
wcstombs_l(...);
#elif defined(HAVE_USELOCALE)
uselocale(...);
#else
elog(ERROR);
#endif
That makes sense to me. I gave it some more thought. Maybe it makes more
sense to just completely drop HAVE_USELOCALE as mentioned, and protect
calls to it with #ifdef WIN32 or whatever the macro is. HAVE_USELOCALE
might be more descriptive, but I don't really care that much either way.
--
Tristan Partin
Neon (https://neon.tech)