Thread: Thread-unsafe coding in ecpg
I've found that a couple of different OpenBSD 6.4 machines fall over
badly in the ecpg regression tests, with output like
test sql/parser ... ok
test thread/thread ... stdout stderr FAILED (test process was terminated by signal 6: Abort trap)
test thread/thread_implicit ... stdout FAILED (test process was terminated by signal 10: Bus error)
test thread/prep ... ok (test process was terminated by signal 10: Bus error)
test thread/alloc ... stderr FAILED (test process was terminated by signal 6: Abort trap)
test thread/descriptor ... ok
It's somewhat variable as to which tests fail, but it's always thread
tests. Examining the core dumps shows traces like
#0 thrkill () at -:3
#1 0x00000c04f427dd6e in _libc_abort () at /usr/src/lib/libc/stdlib/abort.c:51
#2 0x00000c04f425f7e9 in wrterror (d=Variable "d" is not available.
)
at /usr/src/lib/libc/stdlib/malloc.c:291
#3 0x00000c04f42628fb in find_chunknum (d=Variable "d" is not available.
)
at /usr/src/lib/libc/stdlib/malloc.c:1043
#4 0x00000c04f425fe23 in ofree (argpool=Variable "argpool" is not available.
)
at /usr/src/lib/libc/stdlib/malloc.c:1359
#5 0x00000c04f425f8ec in free (ptr=0xc04df0e26e0)
at /usr/src/lib/libc/stdlib/malloc.c:1419
#6 0x00000c04f427ec83 in freegl (oldgl=0xc05a022d080)
at /usr/src/lib/libc/locale/setlocale.c:32
#7 0x00000c04f427eb49 in _libc_setlocale (category=4,
locname=0xc059b605180 "C") at /usr/src/lib/libc/locale/setlocale.c:177
#8 0x00000c0531a6f955 in ecpg_do_epilogue (stmt=0xc0587bb0c00)
at execute.c:1986
#9 0x00000c0531a6fa65 in ecpg_do (lineno=Variable "lineno" is not available.
) at execute.c:2018
#10 0x00000c0531a6fb31 in ECPGdo (lineno=Variable "lineno" is not available.
) at execute.c:2037
#11 0x00000c02a9f00b19 in test_thread (arg=Variable "arg" is not available.
) at thread.pgc:131
#12 0x00000c04b180b26e in _rthread_start (v=Variable "v" is not available.
)
at /usr/src/lib/librthread/rthread.c:96
#13 0x00000c04f42ba77b in __tfork_thread ()
at /usr/src/lib/libc/arch/amd64/sys/tfork_thread.S:75
#14 0x0000000000000000 in ?? ()
or this one:
#0 _libc_strlcpy (dst=0x2c61e133ee0 "C",
src=0xdfdfdfdfdfdfdfdf <Address 0xdfdfdfdfdfdfdfdf out of bounds>,
dsize=256) at /usr/src/lib/libc/string/strlcpy.c:36
#1 0x000002c61de99a71 in _libc_setlocale (category=4, locname=0x0)
from /usr/lib/libc.so.92.5
#2 0x000002c5ae2693a8 in ecpg_do_prologue (lineno=59, compat=0,
force_indicator=1, connection_name=0x0, questionmarks=false,
statement_type=ECPGst_execute, query=0x2c333701418 "i",
args=0x2c61a96f6d0, stmt_out=0x2c61a96f5e0) at execute.c:1776
#3 0x000002c5ae269a20 in ecpg_do (lineno=Variable "lineno" is not available.
) at execute.c:2001
#4 0x000002c5ae269b31 in ECPGdo (lineno=Variable "lineno" is not available.
) at execute.c:2037
#5 0x000002c333600b47 in fn (arg=Variable "arg" is not available.
) at prep.pgc:59
#6 0x000002c56a00b26e in _rthread_start (v=Variable "v" is not available.
)
at /usr/src/lib/librthread/rthread.c:96
#7 0x000002c61ded577b in __tfork_thread ()
at /usr/src/lib/libc/arch/amd64/sys/tfork_thread.S:75
#8 0x0000000000000000 in ?? ()
The common denominator is always a call to setlocale(), and
that generally is calling malloc or free or some other libc
function that is unhappy. When output appears on stderr,
it's usually free complaining about double-frees or some such.
So my conclusion is that this version of setlocale() has some
thread-safety issues. I was all set to go file a bug report
when I noticed this in the POSIX spec for setlocale:
The setlocale() function need not be thread-safe.
as well as this:
The global locale established using setlocale() shall only be used
in threads for which no current locale has been set using
uselocale() or whose current locale has been set to the global
locale using uselocale(LC_GLOBAL_LOCALE).
IOW, not only is setlocale() not necessarily thread-safe itself,
but using it to change locales in a multithread program is seriously
unsafe because of concurrent effects on other threads.
Therefore, it's plain crazy for ecpg to be calling setlocale() inside
threaded code. It looks to me like what ecpg is doing is trying to defend
itself against non-C LC_NUMERIC settings, which is laudable, but this
implementation of that is totally unsafe.
Don't know what's the best way out of this. The simplest thing would
be to just remove that code and document that you'd better run ecpg
in LC_NUMERIC locale, but it'd be nice if we could do better.
regards, tom lane
> So my conclusion is that this version of setlocale() has some > thread-safety issues. I was all set to go file a bug report > when I noticed this in the POSIX spec for setlocale: > > The setlocale() function need not be thread-safe. > > as well as this: > > The global locale established using setlocale() shall only be > used > in threads for which no current locale has been set using > uselocale() or whose current locale has been set to the global > locale using uselocale(LC_GLOBAL_LOCALE). This one was new to me. > IOW, not only is setlocale() not necessarily thread-safe itself, > but using it to change locales in a multithread program is seriously > unsafe because of concurrent effects on other threads. Agreed. > Therefore, it's plain crazy for ecpg to be calling setlocale() inside > threaded code. It looks to me like what ecpg is doing is trying to > defend > itself against non-C LC_NUMERIC settings, which is laudable, but this > implementation of that is totally unsafe. > > Don't know what's the best way out of this. The simplest thing would > be to just remove that code and document that you'd better run ecpg > in LC_NUMERIC locale, but it'd be nice if we could do better. How about attached patch? According to my manpages it should only affect the calling threat. I only tested it on my own system so far. Could you please have a look and/or test on other systems? Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Attachment
Michael Meskes <meskes@postgresql.org> writes:
>> IOW, not only is setlocale() not necessarily thread-safe itself,
>> but using it to change locales in a multithread program is seriously
>> unsafe because of concurrent effects on other threads.
> Agreed.
> How about attached patch? According to my manpages it should only
> affect the calling threat. I only tested it on my own system so far.
> Could you please have a look and/or test on other systems?
Yeah, I was wondering about uselocale() myself. We cannot assume it's
available everywhere, but it should fix the problem where available.
On machines that don't have it, we could either
(a) have ecpg do nothing, and just hope you're not using a dangerous
locale; or
(b) consider the platform not thread-safe, forcing people to specify
--disable-thread-safety to build.
While (b) has more theoretical purity, I'm inclined to think it
doesn't really improve anybody's life compared to (a), because
--disable-thread-safety doesn't actually stop anyone from using
libpq or ecpglib in threaded environments. It just makes it
more likely to fail when they do.
The OpenBSD 6.4 platform where I found this problem has uselocale
(but the man page notes they only added it as of 6.2). I can test
out the patch there, but I think the interesting questions are all
about what to do on platforms without the function.
regards, tom lane
> While (b) has more theoretical purity, I'm inclined to think it > doesn't really improve anybody's life compared to (a), because > --disable-thread-safety doesn't actually stop anyone from using > libpq or ecpglib in threaded environments. It just makes it > more likely to fail when they do. The question is, what do we do on those platforms? Use setlocale() or fallback to (a) and document that ecpg has to run in a C locale? We could also rewrite the parsing of numbers to not be locale dependent. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Michael Meskes <meskes@postgresql.org> writes:
>> While (b) has more theoretical purity, I'm inclined to think it
>> doesn't really improve anybody's life compared to (a), because
>> --disable-thread-safety doesn't actually stop anyone from using
>> libpq or ecpglib in threaded environments. It just makes it
>> more likely to fail when they do.
> The question is, what do we do on those platforms? Use setlocale() or
> fallback to (a) and document that ecpg has to run in a C locale?
No, we shouldn't use setlocale(), because it clearly is hazardous
even on platforms where it doesn't fail outright. I don't see
anything so wrong with just documenting the hazard. The situation
isn't noticeably more dangerous than simple use of the C library;
sscanf, strtod, etc are all likely to do surprising things when
LC_NUMERIC isn't C.
> We could also rewrite the parsing of numbers to not be locale
> dependent.
Perhaps, but that seems like a giant undertaking. I'm not excited
about duplicating strtod(), for instance.
regards, tom lane
>>>>> "Michael" == Michael Meskes <meskes@postgresql.org> writes: >> Therefore, it's plain crazy for ecpg to be calling setlocale() >> inside threaded code. It looks to me like what ecpg is doing is >> trying to defend itself against non-C LC_NUMERIC settings, which is >> laudable, but this implementation of that is totally unsafe. >> >> Don't know what's the best way out of this. The simplest thing would >> be to just remove that code and document that you'd better run ecpg >> in LC_NUMERIC locale, but it'd be nice if we could do better. Would it help if we had non-locale-aware functions for both floating-point output _and_ input? i.e. import a known-working strtod() (allowing us to remove all the hacks that have grown up around it, for special-case input and wonky error handling) with locale functionality removed. -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Would it help if we had non-locale-aware functions for both
> floating-point output _and_ input? i.e. import a known-working strtod()
> (allowing us to remove all the hacks that have grown up around it, for
> special-case input and wonky error handling) with locale functionality
> removed.
Dunno, is there such a thing as a platform-independent strtod()?
I'd have thought that, for instance, typical implementations would
be pretty much in bed with the details of IEEE float format ---
your example where strtof() is different from (float) strtod()
makes it hard to believe that it can be written without assumptions
about the hardware's float format.
(Note that this concern is independent of whether we adopt the Ryu
code, which IIUC also depends on IEEE floats. Our answer for anyone
wanting to run on non-IEEE hardware can be to #ifdef out Ryu and use
the existing float output code. But doing the equivalent thing on the
input side wouldn't solve ecpg's problem.)
regards, tom lane
> > The question is, what do we do on those platforms? Use setlocale() > > or > > fallback to (a) and document that ecpg has to run in a C locale? > > No, we shouldn't use setlocale(), because it clearly is hazardous > even on platforms where it doesn't fail outright. I don't see > anything so wrong with just documenting the hazard. The situation Actually I meant using setlocale() and documenting that it must not be used with threads, or document it must not be used with locales? Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Michael Meskes <meskes@postgresql.org> writes:
>> No, we shouldn't use setlocale(), because it clearly is hazardous
>> even on platforms where it doesn't fail outright. I don't see
>> anything so wrong with just documenting the hazard. The situation
> Actually I meant using setlocale() and documenting that it must not be
> used with threads, or document it must not be used with locales?
I tend to think that has more downside than upside, in situations where
people don't read the manual closely and try to do it anyway.
First, there's the probable crash if setlocale() is thread-unsafe.
(Though the lack of previous reports suggests that on most platforms,
it isn't.)
Second, if the program is indeed trying to run with non-C LC_NUMERIC,
using setlocale() will have unsynchronized, hard-to-debug side effects
on other threads. Not using it will have no downside at all if ecpg
isn't trying to read numeric data, while if it does do so, the failures
will be reproducible and easy to understand/debug.
Admittedly, removing the setlocale() call will be a net negative for
single-threaded applications, which are likely the majority. But
I don't know any safe way to tell whether the app is multi threaded.
On the third hand, the lack of previous reports suggests that maybe
this whole thing is seldom a problem in practice. Maybe we should
just use uselocale() where available and otherwise hope it's OK
to keep on doing what we were doing.
regards, tom lane
I wrote:
> On the third hand, the lack of previous reports suggests that maybe
> this whole thing is seldom a problem in practice. Maybe we should
> just use uselocale() where available and otherwise hope it's OK
> to keep on doing what we were doing.
If we go with that approach, I think we need to adapt the patch
as attached. I autoconfiscated it and fixed a portability problem
(it didn't compile on macOS, which has these decls in <xlocale.h>).
I've verified that this fixes the problem I was seeing on OpenBSD 6.4.
I've not bothered to test on a platform lacking uselocale() --- I
think it's clear by inspection that the patch doesn't change anything
in that case.
Not sure if we need to document this or not. On platforms with
uselocale(), it should fix the problem without any need for user
attention. On platforms without, there's no change, and given
the lack of previous complaints I'm not sure it's really an issue.
regards, tom lane
diff --git a/configure b/configure
index 7602e65..1e69eda 100755
*** a/configure
--- b/configure
*************** fi
*** 15209,15215 ****
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll
posix_fallocateppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul
strsignalsymlink sync_file_range utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
--- 15209,15215 ----
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll
posix_fallocateppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul
strsignalsymlink sync_file_range uselocale utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.in b/configure.in
index d599ad8..556186c 100644
*** a/configure.in
--- b/configure.in
*************** AC_CHECK_FUNCS(m4_normalize([
*** 1618,1623 ****
--- 1618,1624 ----
strsignal
symlink
sync_file_range
+ uselocale
utime
utimes
wcstombs_l
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 9d99816..2c899a1 100644
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***************
*** 691,696 ****
--- 691,699 ----
/* Define to 1 if the system has the type `unsigned long long int'. */
#undef HAVE_UNSIGNED_LONG_LONG_INT
+ /* Define to 1 if you have the `uselocale' function. */
+ #undef HAVE_USELOCALE
+
/* Define to 1 if you have the `utime' function. */
#undef HAVE_UTIME
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 8a560ef..3964433 100644
*** a/src/include/pg_config.h.win32
--- b/src/include/pg_config.h.win32
***************
*** 545,550 ****
--- 545,553 ----
/* Define to 1 if you have the `unsetenv' function. */
/* #undef HAVE_UNSETENV */
+ /* Define to 1 if you have the `uselocale' function. */
+ /* #undef HAVE_USELOCALE */
+
/* Define to 1 if you have the `utime' function. */
#define HAVE_UTIME 1
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1c9bce1..22a0557 100644
*** a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
--- b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
***************
*** 12,17 ****
--- 12,20 ----
#ifndef CHAR_BIT
#include <limits.h>
#endif
+ #ifdef LOCALE_T_IN_XLOCALE
+ #include <xlocale.h>
+ #endif
enum COMPAT_MODE
{
*************** struct statement
*** 61,67 ****
--- 64,75 ----
bool questionmarks;
struct variable *inlist;
struct variable *outlist;
+ #ifdef HAVE_USELOCALE
+ locale_t clocale;
+ locale_t oldlocale;
+ #else
char *oldlocale;
+ #endif
int nparams;
char **paramvalues;
PGresult *results;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 3f5034e..d3e32d2 100644
*** a/src/interfaces/ecpg/ecpglib/execute.c
--- b/src/interfaces/ecpg/ecpglib/execute.c
*************** free_statement(struct statement *stmt)
*** 102,108 ****
--- 102,113 ----
free_variable(stmt->outlist);
ecpg_free(stmt->command);
ecpg_free(stmt->name);
+ #ifdef HAVE_USELOCALE
+ if (stmt->clocale)
+ freelocale(stmt->clocale);
+ #else
ecpg_free(stmt->oldlocale);
+ #endif
ecpg_free(stmt);
}
*************** ecpg_do_prologue(int lineno, const int c
*** 1771,1778 ****
/*
* Make sure we do NOT honor the locale for numeric input/output since the
! * database wants the standard decimal point
*/
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
--- 1776,1798 ----
/*
* Make sure we do NOT honor the locale for numeric input/output since the
! * database wants the standard decimal point. If available, use
! * uselocale() for this because it's thread-safe.
*/
+ #ifdef HAVE_USELOCALE
+ stmt->clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
+ if (stmt->clocale == (locale_t) 0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ stmt->oldlocale = uselocale(stmt->clocale);
+ if (stmt->oldlocale == (locale_t) 0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #else
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
*************** ecpg_do_prologue(int lineno, const int c
*** 1780,1785 ****
--- 1800,1806 ----
return false;
}
setlocale(LC_NUMERIC, "C");
+ #endif
#ifdef ENABLE_THREAD_SAFETY
ecpg_pthreads_init();
*************** ecpg_do_epilogue(struct statement *stmt)
*** 1982,1989 ****
--- 2003,2015 ----
if (stmt == NULL)
return;
+ #ifdef HAVE_USELOCALE
+ if (stmt->oldlocale)
+ uselocale(stmt->oldlocale);
+ #else
if (stmt->oldlocale)
setlocale(LC_NUMERIC, stmt->oldlocale);
+ #endif
free_statement(stmt);
}
On Windows, _configthreadlocale() enables us to restrict the effect of setlocale() only to the calling thread. We can callit in ecpg_do_prolog/epilog(). https://docs.microsoft.com/en-us/cpp/parallel/multithreading-and-locales?view=vs-2017 Regards Takayuki Tsunakawa
"Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> writes:
> On Windows, _configthreadlocale() enables us to restrict the effect of setlocale() only to the calling thread. We
cancall it in ecpg_do_prolog/epilog().
How far back does that exist?
regards, tom lane
From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > "Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> writes: > > On Windows, _configthreadlocale() enables us to restrict the effect of > setlocale() only to the calling thread. We can call it in > ecpg_do_prolog/epilog(). > > How far back does that exist? I couldn't find the relevant doc, but I've just confirmed I can use it with Visual Studio 2008 on Win7, which is my oldestcombination at hand. VS 2008 is already past its EOL, and the support for Win7 will end next year, so the combinationis practically enough. Regards Takayuki Tsunakawa
"Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> writes:
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
>> How far back does that exist?
> I couldn't find the relevant doc, but I've just confirmed I can use it with Visual Studio 2008 on Win7, which is my
oldestcombination at hand. VS 2008 is already past its EOL, and the support for Win7 will end next year, so the
combinationis practically enough.
Hm. Well, I suppose we can figure that the buildfarm should tell us
if there's anything too old that we still care about.
So like this ...
regards, tom lane
diff --git a/configure b/configure
index 7602e65..1e69eda 100755
*** a/configure
--- b/configure
*************** fi
*** 15209,15215 ****
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll
posix_fallocateppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul
strsignalsymlink sync_file_range utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
--- 15209,15215 ----
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll
posix_fallocateppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul
strsignalsymlink sync_file_range uselocale utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.in b/configure.in
index d599ad8..556186c 100644
*** a/configure.in
--- b/configure.in
*************** AC_CHECK_FUNCS(m4_normalize([
*** 1618,1623 ****
--- 1618,1624 ----
strsignal
symlink
sync_file_range
+ uselocale
utime
utimes
wcstombs_l
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 9d99816..2c899a1 100644
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***************
*** 691,696 ****
--- 691,699 ----
/* Define to 1 if the system has the type `unsigned long long int'. */
#undef HAVE_UNSIGNED_LONG_LONG_INT
+ /* Define to 1 if you have the `uselocale' function. */
+ #undef HAVE_USELOCALE
+
/* Define to 1 if you have the `utime' function. */
#undef HAVE_UTIME
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 8a560ef..3964433 100644
*** a/src/include/pg_config.h.win32
--- b/src/include/pg_config.h.win32
***************
*** 545,550 ****
--- 545,553 ----
/* Define to 1 if you have the `unsetenv' function. */
/* #undef HAVE_UNSETENV */
+ /* Define to 1 if you have the `uselocale' function. */
+ /* #undef HAVE_USELOCALE */
+
/* Define to 1 if you have the `utime' function. */
#define HAVE_UTIME 1
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1c9bce1..41851d5 100644
*** a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
--- b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
***************
*** 12,17 ****
--- 12,20 ----
#ifndef CHAR_BIT
#include <limits.h>
#endif
+ #ifdef LOCALE_T_IN_XLOCALE
+ #include <xlocale.h>
+ #endif
enum COMPAT_MODE
{
*************** struct statement
*** 61,67 ****
--- 64,78 ----
bool questionmarks;
struct variable *inlist;
struct variable *outlist;
+ #ifdef HAVE_USELOCALE
+ locale_t clocale;
+ locale_t oldlocale;
+ #else
char *oldlocale;
+ #ifdef WIN32
+ int oldthreadlocale;
+ #endif
+ #endif
int nparams;
char **paramvalues;
PGresult *results;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 3f5034e..f67d774 100644
*** a/src/interfaces/ecpg/ecpglib/execute.c
--- b/src/interfaces/ecpg/ecpglib/execute.c
*************** free_statement(struct statement *stmt)
*** 102,108 ****
--- 102,113 ----
free_variable(stmt->outlist);
ecpg_free(stmt->command);
ecpg_free(stmt->name);
+ #ifdef HAVE_USELOCALE
+ if (stmt->clocale)
+ freelocale(stmt->clocale);
+ #else
ecpg_free(stmt->oldlocale);
+ #endif
ecpg_free(stmt);
}
*************** ecpg_do_prologue(int lineno, const int c
*** 1771,1778 ****
/*
* Make sure we do NOT honor the locale for numeric input/output since the
! * database wants the standard decimal point
*/
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
--- 1776,1806 ----
/*
* Make sure we do NOT honor the locale for numeric input/output since the
! * database wants the standard decimal point. If available, use
! * uselocale() for this because it's thread-safe.
*/
+ #ifdef HAVE_USELOCALE
+ stmt->clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
+ if (stmt->clocale == (locale_t) 0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ stmt->oldlocale = uselocale(stmt->clocale);
+ if (stmt->oldlocale == (locale_t) 0)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #else
+ #ifdef WIN32
+ stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
+ if (stmt->oldthreadlocale == -1)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #endif
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
*************** ecpg_do_prologue(int lineno, const int c
*** 1780,1785 ****
--- 1808,1814 ----
return false;
}
setlocale(LC_NUMERIC, "C");
+ #endif
#ifdef ENABLE_THREAD_SAFETY
ecpg_pthreads_init();
*************** ecpg_do_epilogue(struct statement *stmt)
*** 1982,1989 ****
--- 2011,2028 ----
if (stmt == NULL)
return;
+ #ifdef HAVE_USELOCALE
+ if (stmt->oldlocale != (locale_t) 0)
+ uselocale(stmt->oldlocale);
+ #else
if (stmt->oldlocale)
+ {
setlocale(LC_NUMERIC, stmt->oldlocale);
+ #ifdef WIN32
+ _configthreadlocale(stmt->oldthreadlocale);
+ #endif
+ }
+ #endif
free_statement(stmt);
}
From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > Hm. Well, I suppose we can figure that the buildfarm should tell us if > there's anything too old that we still care about. > > So like this ... How quick! Thank you. I've reviewed the code for both Unix and Windows, and it looks OK. I haven't built the patch, butexpect the buildfarm to do the test. Regards Takayuki Tsunakawa
"Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> writes:
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
>> So like this ...
> How quick! Thank you. I've reviewed the code for both Unix and Windows, and it looks OK. I haven't built the
patch,but expect the buildfarm to do the test.
Thanks for reviewing! I've pushed this now (to HEAD only for the moment),
we'll see what the buildfarm thinks.
BTW, I found another spot in descriptor.c where ecpglib is using
setlocale() for the same purpose. Perhaps that one's not reachable
in threaded apps, but I didn't see any obvious reason to think so,
so I changed it too.
regards, tom lane
> Thanks for reviewing! I've pushed this now (to HEAD only for the > moment), > we'll see what the buildfarm thinks. > > BTW, I found another spot in descriptor.c where ecpglib is using > setlocale() for the same purpose. Perhaps that one's not reachable > in threaded apps, but I didn't see any obvious reason to think so, > so I changed it too. Thanks Tom. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
On 2019-01-21 12:09:30 -0500, Tom Lane wrote: > "Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> writes: > > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > >> So like this ... > > > How quick! Thank you. I've reviewed the code for both Unix and Windows, and it looks OK. I haven't built the patch,but expect the buildfarm to do the test. > > Thanks for reviewing! I've pushed this now (to HEAD only for the moment), > we'll see what the buildfarm thinks. > > BTW, I found another spot in descriptor.c where ecpglib is using > setlocale() for the same purpose. Perhaps that one's not reachable > in threaded apps, but I didn't see any obvious reason to think so, > so I changed it too. Seems jacana might not have like this change? https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-21%2019%3A01%3A28 Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > Seems jacana might not have like this change? > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-21%2019%3A01%3A28 Hmm. So mingw doesn't provide access to _configthreadlocale(). That's unfortunate, at least if we think that mingw is still a viable production platform, because it means we can't make ecpg thread-safe on that platform. Is there a newer version of mingw that does have this functionality? I'm not sure whether to install a version check or just assume that it's never there. regards, tom lane
On 1/21/19 12:05 PM, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> Seems jacana might not have like this change? >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-21%2019%3A01%3A28 > Hmm. So mingw doesn't provide access to _configthreadlocale(). > That's unfortunate, at least if we think that mingw is still a viable > production platform, because it means we can't make ecpg thread-safe > on that platform. > > Is there a newer version of mingw that does have this functionality? > I'm not sure whether to install a version check or just assume that > it's never there. Apparently this can be done with thee 64bit version: https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw JD > > regards, tom lane > -- Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc PostgreSQL centered full stack support, consulting and development. Advocate: @amplifypostgres || Learn and Network: https://postgresconf.org *** A fault and talent of mine is to tell it exactly how it is. ***
Hi, On 2019-01-21 15:05:23 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Seems jacana might not have like this change? > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-01-21%2019%3A01%3A28 > > Hmm. So mingw doesn't provide access to _configthreadlocale(). > That's unfortunate, at least if we think that mingw is still a viable > production platform, because it means we can't make ecpg thread-safe > on that platform. > > Is there a newer version of mingw that does have this functionality? > I'm not sure whether to install a version check or just assume that > it's never there. It does seem like newer versions do have it: https://sourceforge.net/p/mingw-w64/mailman/message/34765722/ https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/_configthreadlocale.c We could just refuse to support thread safety on mingw if that's not supported? Or is that too aggressive? Greetings, Andres Freund
"Joshua D. Drake" <jd@commandprompt.com> writes: > On 1/21/19 12:05 PM, Tom Lane wrote: >> Is there a newer version of mingw that does have this functionality? > Apparently this can be done with thee 64bit version: > https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw Hmm, the followup question makes it sound like it still didn't work :-(. However, since the mingw build is autoconf-based, seems like we can install a configure check instead of guessing. Will make it so. Task for somebody else: run a MinGW64 buildfarm member. regards, tom lane
Andres Freund <andres@anarazel.de> writes:
> We could just refuse to support thread safety on mingw if that's not
> supported? Or is that too aggressive?
Nah, we already had that discussion upthread. Given the lack of
prior complaints, we shouldn't break cases that are working today.
For instance, as long as setlocale() isn't actively thread-unsafe
and you are running with LC_NUMERIC=C as the prevailing locale,
the existing code doesn't pose a hazard even in a threaded app.
Forcing users for whom that's true to --disable-thread-safety
would turn an OK situation into a broken one.
regards, tom lane
On 1/21/19 3:25 PM, Tom Lane wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: >> On 1/21/19 12:05 PM, Tom Lane wrote: >>> Is there a newer version of mingw that does have this functionality? >> Apparently this can be done with thee 64bit version: >> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw > Hmm, the followup question makes it sound like it still didn't work :-(. > > However, since the mingw build is autoconf-based, seems like we can > install a configure check instead of guessing. Will make it so. > > Task for somebody else: run a MinGW64 buildfarm member. > > I could set that up in just about two shakes of a lamb's tail - I have a script to do so all tested on vagrant/aws within the last few months. What I don't have is resources. My Windows resources are pretty much tapped out. I would need either some modest hardware or a Windows VM somewhere in the cloud - could be anywhere but I'm most at home on AWS. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> BTW, I found another spot in descriptor.c where ecpglib is using
> setlocale() for the same purpose. Perhaps that one's not reachable
> in threaded apps, but I didn't see any obvious reason to think so,
> so I changed it too.
Ouch, thanks. And I'm sorry to annoy you by pointing out a trivial thing: in v3 patch, _configthreadlocale() is not
calledto restore the original value when setlocale() or ecpg_strdup() fails. I hope this is fixed in v4.
+ #ifdef WIN32
+ stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
+ if (stmt->oldthreadlocale == -1)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #endif
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
Regards
Takayuki Tsunakawa
On 1/21/19 10:00 PM, Andrew Dunstan wrote: > On 1/21/19 3:25 PM, Tom Lane wrote: >> "Joshua D. Drake" <jd@commandprompt.com> writes: >>> On 1/21/19 12:05 PM, Tom Lane wrote: >>>> Is there a newer version of mingw that does have this functionality? >>> Apparently this can be done with thee 64bit version: >>> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw >> Hmm, the followup question makes it sound like it still didn't work :-(. >> >> However, since the mingw build is autoconf-based, seems like we can >> install a configure check instead of guessing. Will make it so. >> >> Task for somebody else: run a MinGW64 buildfarm member. >> >> > > I could set that up in just about two shakes of a lamb's tail - I have a > script to do so all tested on vagrant/aws within the last few months. > > > What I don't have is resources. My Windows resources are pretty much > tapped out. I would need either some modest hardware or a Windows VM > somewhere in the cloud - could be anywhere but I'm most at home on AWS. > Incidentally, jacana *is* running mingw64, but possibly not a young enough version. I just ran a test on an up to date version and it found the config setting happily, and passed the make stage. I can look at upgrading jacana to a more modern compiler. The version it's running is 4.9.1, apparently built around Oct 2014, so it's not that old. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On January 22, 2019 9:50:19 AM PST, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > >On 1/21/19 10:00 PM, Andrew Dunstan wrote: >> On 1/21/19 3:25 PM, Tom Lane wrote: >>> "Joshua D. Drake" <jd@commandprompt.com> writes: >>>> On 1/21/19 12:05 PM, Tom Lane wrote: >>>>> Is there a newer version of mingw that does have this >functionality? >>>> Apparently this can be done with thee 64bit version: >>>> >https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw >>> Hmm, the followup question makes it sound like it still didn't work >:-(. >>> >>> However, since the mingw build is autoconf-based, seems like we can >>> install a configure check instead of guessing. Will make it so. >>> >>> Task for somebody else: run a MinGW64 buildfarm member. >>> >>> >> >> I could set that up in just about two shakes of a lamb's tail - I >have a >> script to do so all tested on vagrant/aws within the last few months. >> >> >> What I don't have is resources. My Windows resources are pretty much >> tapped out. I would need either some modest hardware or a Windows VM >> somewhere in the cloud - could be anywhere but I'm most at home on >AWS. >> > > >Incidentally, jacana *is* running mingw64, but possibly not a young >enough version. I just ran a test on an up to date version and it found >the config setting happily, and passed the make stage. > > >I can look at upgrading jacana to a more modern compiler. The version >it's running is 4.9.1, apparently built around Oct 2014, so it's not >that old. The thread about the introduction is from 2016, so that's kind of expected. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On 1/22/19 12:50 PM, Andrew Dunstan wrote: > On 1/21/19 10:00 PM, Andrew Dunstan wrote: >> On 1/21/19 3:25 PM, Tom Lane wrote: >>> "Joshua D. Drake" <jd@commandprompt.com> writes: >>>> On 1/21/19 12:05 PM, Tom Lane wrote: >>>>> Is there a newer version of mingw that does have this functionality? >>>> Apparently this can be done with thee 64bit version: >>>> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw >>> Hmm, the followup question makes it sound like it still didn't work :-(. >>> >>> However, since the mingw build is autoconf-based, seems like we can >>> install a configure check instead of guessing. Will make it so. >>> >>> Task for somebody else: run a MinGW64 buildfarm member. >>> >>> >> I could set that up in just about two shakes of a lamb's tail - I have a >> script to do so all tested on vagrant/aws within the last few months. >> >> >> What I don't have is resources. My Windows resources are pretty much >> tapped out. I would need either some modest hardware or a Windows VM >> somewhere in the cloud - could be anywhere but I'm most at home on AWS. >> > > Incidentally, jacana *is* running mingw64, but possibly not a young > enough version. I just ran a test on an up to date version and it found > the config setting happily, and passed the make stage. > > > I can look at upgrading jacana to a more modern compiler. The version > it's running is 4.9.1, apparently built around Oct 2014, so it's not > that old. > > I have just spent a large amount of time testing the committed fix with a number of versions of gcc. It blows up on any compiler modern enough to know about _configthreadlocale Essentially what happens is this: ============== running regression test queries ============== test compat_informix/dec_test ... ok test compat_informix/charfuncs ... ok test compat_informix/rfmtdate ... ok test compat_informix/rfmtlong ... ok test compat_informix/rnull ... stdout stderr FAILED test compat_informix/sqlda ... stdout stderr FAILED (test process exited with exit code 1) test compat_informix/describe ... stdout stderr FAILED (test process exited with exit code 1) test compat_informix/test_informix ... At this point the regression tests hang and the disk starts filling up rapidly. This has been tested with versions 5.4.0, 6.4.0, 7.3.0, 8.1.0 and 8.2.1. The first 4 are downloaded direct from the Mingw64 repo, the last is from Msys2's mingw64 toolchain. I'm trying to find out more info, but what we have right now is pretty broken. Reverting commits ee27584c4a and 8eb4a9312 cures[*] the problem. cheers andrew [*] FSVO "cure". I am still getting intermittent errors but no hangs. -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> I have just spent a large amount of time testing the committed fix with
> a number of versions of gcc. It blows up on any compiler modern enough
> to know about _configthreadlocale
Bleah. Since the regular Windows buildfarm members seem happy, this
evidently means that MinGW's _configthreadlocale is broken in some way.
I suppose we could just remove the autoconf test and build without
_configthreadlocale on MinGW, but that's kind of sad ...
Perhaps there's some sort of setup that MinGW's version needs that
we're not doing?
regards, tom lane
On 1/23/19 6:01 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> I have just spent a large amount of time testing the committed fix with >> a number of versions of gcc. It blows up on any compiler modern enough >> to know about _configthreadlocale > Bleah. Since the regular Windows buildfarm members seem happy, this > evidently means that MinGW's _configthreadlocale is broken in some way. > > I suppose we could just remove the autoconf test and build without > _configthreadlocale on MinGW, but that's kind of sad ... > > Perhaps there's some sort of setup that MinGW's version needs that > we're not doing? This seems to suggest something worse: https://reviews.llvm.org/D40181 Not sure I fully understand what's happening here, though. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > On 1/23/19 6:01 PM, Tom Lane wrote: >> Perhaps there's some sort of setup that MinGW's version needs that >> we're not doing? > This seems to suggest something worse: https://reviews.llvm.org/D40181 > Not sure I fully understand what's happening here, though. Me either, but I noted a couple of interesting extracts from that page: Normal mingw that uses msvcrt.dll doesn't have per-thread locales so it won't really work in any case (but I think it does define some sort of dummy functions that at least will allow it to build). Nowadays, mingw can be built to target ucrtbase.dll as well though, and there it should be possible to make it work just like for MSVC although it might need some patches. ... Looked into MinGW-w64 sources and this is indeed the case. _configthreadlocale will return -1 and will not do anything. This suggests that, rather than throwing up our hands if the initial _configthreadlocale call returns -1, we should act as though the function doesn't exist, and just soldier on the same as before. The code in there assumes that -1 is a can't-happen case and doesn't try to recover, but apparently that's over-optimistic. regards, tom lane
I wrote:
> This suggests that, rather than throwing up our hands if the initial
> _configthreadlocale call returns -1, we should act as though the function
> doesn't exist, and just soldier on the same as before. The code in there
> assumes that -1 is a can't-happen case and doesn't try to recover,
> but apparently that's over-optimistic.
I pushed a patch to fix that.
It looks to me like the reason that the ecpg tests went into an infinite
loop is that compat_informix/test_informix.pgc has not considered the
possibility of repeated statement failures:
while (1)
{
$fetch forward c into :i, :j, :c;
if (sqlca.sqlcode == 100) break;
else if (sqlca.sqlcode != 0) printf ("Error: %ld\n", sqlca.sqlcode);
if (risnull(CDECIMALTYPE, (char *)&j))
printf("%d NULL\n", i);
else
{
int a;
dectoint(&j, &a);
printf("%d %d \"%s\"\n", i, a, c);
}
}
I know zip about ecpg coding practices, but wouldn't it be a better idea
to break out of the loop on seeing an error?
regards, tom lane
Oh, I just noticed something else: several of the ecpg test programs
contain
#ifdef WIN32
#ifdef _MSC_VER /* requires MSVC */
_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
#endif
#endif
Surely this is a kluge that we could now remove? We've protected the
only setlocale calls that the ecpg tests could reach in threaded mode.
If there is still a problem, it'd behoove us to find it, because
ecpglib should not expect that the calling app has done this for it.
regards, tom lane
On 1/23/19 10:53 PM, Tom Lane wrote: > I wrote: >> This suggests that, rather than throwing up our hands if the initial >> _configthreadlocale call returns -1, we should act as though the function >> doesn't exist, and just soldier on the same as before. The code in there >> assumes that -1 is a can't-happen case and doesn't try to recover, >> but apparently that's over-optimistic. > I pushed a patch to fix that. jacana has been upgraded to gcc 8.1.0 so it knows about _configthreadlocale() and it's now happy. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> It looks to me like the reason that the ecpg tests went into an > infinite > loop is that compat_informix/test_informix.pgc has not considered the > possibility of repeated statement failures: > ... Correct, this was missing a safeguard. > I know zip about ecpg coding practices, but wouldn't it be a better > idea > to break out of the loop on seeing an error? I wonder if it would be better to make the test cases use the proper whenever command instead. That would give us a slightly better functionality testing I'd say. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Michael Meskes <meskes@postgresql.org> writes:
> I wonder if it would be better to make the test cases use the proper
> whenever command instead. That would give us a slightly better
> functionality testing I'd say.
I like having a hard limit on the number of loop iterations;
that should ensure that the test terminates no matter how confused
ecpglib is.
But certainly we could have more of the tests using "whenever"
as the intended method of getting out of the loop.
regards, tom lane
> I like having a hard limit on the number of loop iterations; > that should ensure that the test terminates no matter how confused > ecpglib is. I get your point and thus will only clean up the tests a little bit. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL