Thread-unsafe coding in ecpg

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Thread-unsafe coding in ecpg

Tom Lane-2
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

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Michael Meskes-3
> 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

ecpg_locale.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
Michael Meskes <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Michael Meskes-3
> 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


Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
Michael Meskes <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Andrew Gierth
In reply to this post by Michael Meskes-3
>>>>> "Michael" == Michael Meskes <[hidden email]> 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)

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
Andrew Gierth <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Michael Meskes-3
In reply to this post by Tom Lane-2
> > 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


Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
Michael Meskes <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
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_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink 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_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink 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);
  }
Reply | Threaded
Open this post in threaded view
|

RE: Thread-unsafe coding in ecpg

Tsunakawa, Takayuki
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().

https://docs.microsoft.com/en-us/cpp/parallel/multithreading-and-locales?view=vs-2017

Regards
Takayuki Tsunakawa



Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
"Tsunakawa, Takayuki" <[hidden email]> 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?

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

RE: Thread-unsafe coding in ecpg

Tsunakawa, Takayuki
From: Tom Lane [mailto:[hidden email]]
> "Tsunakawa, Takayuki" <[hidden email]> 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 oldest combination at hand.  VS 2008 is already past its EOL, and the support for Win7 will end next year, so the combination is practically enough.

Regards
Takayuki Tsunakawa



Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
"Tsunakawa, Takayuki" <[hidden email]> writes:
> From: Tom Lane [mailto:[hidden email]]
>> 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 oldest combination at hand.  VS 2008 is already past its EOL, and the support for Win7 will end next year, so the combination is 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_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink 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_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink 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);
  }
Reply | Threaded
Open this post in threaded view
|

RE: Thread-unsafe coding in ecpg

Tsunakawa, Takayuki
From: Tom Lane [mailto:[hidden email]]
> 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, but expect the buildfarm to do the test.


Regards
Takayuki Tsunakawa




Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
"Tsunakawa, Takayuki" <[hidden email]> writes:
> From: Tom Lane [mailto:[hidden email]]
>> 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

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Michael Meskes-3
> 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


Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Andres Freund
In reply to this post by Tom Lane-2
On 2019-01-21 12:09:30 -0500, Tom Lane wrote:

> "Tsunakawa, Takayuki" <[hidden email]> writes:
> > From: Tom Lane [mailto:[hidden email]]
> >> 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

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Tom Lane-2
Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Thread-unsafe coding in ecpg

Joshua D. Drake
On 1/21/19 12:05 PM, Tom Lane wrote:

> Andres Freund <[hidden email]> 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.  ***


12