Horton

Thread

Commits

  1. Remove _configthreadlocale() calls in ecpg test suite.

  2. Remove infinite-loop hazards in ecpg test suite.

  3. Blind attempt to fix _configthreadlocale() failures on MinGW.

  4. Avoid thread-safety problem in ecpglib.

  5. Second try at fixing ecpglib thread-safety problem.

  1. Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-18T03:54:57Z

    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

  2. Re: Thread-unsafe coding in ecpg

    Michael Meskes <meskes@postgresql.org> — 2019-01-19T19:15:37Z

    > 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

  3. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-20T00:10:59Z

    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

  4. Re: Thread-unsafe coding in ecpg

    Michael Meskes <meskes@postgresql.org> — 2019-01-20T00:23:07Z

    > 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

  5. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-20T00:37:29Z

    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

  6. Re: Thread-unsafe coding in ecpg

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-01-20T01:32:50Z

    >>>>> "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)

  7. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-20T02:00:37Z

    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

  8. Re: Thread-unsafe coding in ecpg

    Michael Meskes <meskes@postgresql.org> — 2019-01-20T17:08:40Z

    > > 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

  9. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-20T17:30:48Z

    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

  10. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-20T20:04:36Z

    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

  11. RE: Thread-unsafe coding in ecpg

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-21T02:14:17Z

    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

  12. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T02:20:55Z

    "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?

			regards, tom lane

  13. RE: Thread-unsafe coding in ecpg

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-21T02:57:34Z

    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 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

  14. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T03:16:01Z

    "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 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

  15. RE: Thread-unsafe coding in ecpg

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-21T06:07:50Z

    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, but expect the buildfarm to do the test.


Regards
Takayuki Tsunakawa

  16. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T17:09:30Z

    "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

  17. Re: Thread-unsafe coding in ecpg

    Michael Meskes <meskes@postgresql.org> — 2019-01-21T17:51:56Z

    > 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

  18. Re: Thread-unsafe coding in ecpg

    Andres Freund <andres@anarazel.de> — 2019-01-21T19:35:12Z

    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

  19. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T20:05:23Z

    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

  20. Re: Thread-unsafe coding in ecpg

    Joshua D. Drake <jd@commandprompt.com> — 2019-01-21T20:18:08Z

    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.  ***

  21. Re: Thread-unsafe coding in ecpg

    Andres Freund <andres@anarazel.de> — 2019-01-21T20:21:50Z

    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

  22. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T20:25:32Z

    "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

  23. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T20:45:04Z

    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

  24. Re: Thread-unsafe coding in ecpg

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-01-22T03:00:57Z

    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

  25. RE: Thread-unsafe coding in ecpg

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-22T06:57:22Z

    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 called to 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

  26. Re: Thread-unsafe coding in ecpg

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-01-22T17:50:19Z

    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

  27. Re: Thread-unsafe coding in ecpg

    Andres Freund <andres@anarazel.de> — 2019-01-22T17:56:35Z

    
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.

  28. Re: Thread-unsafe coding in ecpg

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-01-23T22:37:25Z

    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

  29. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-23T23:01:35Z

    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

  30. Re: Thread-unsafe coding in ecpg

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-01-24T00:28:36Z

    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

  31. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-24T03:10:04Z

    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

  32. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-24T03:53:24Z

    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

  33. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-24T05:23:04Z

    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

  34. Re: Thread-unsafe coding in ecpg

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-01-24T16:17:46Z

    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

  35. Re: Thread-unsafe coding in ecpg

    Michael Meskes <meskes@postgresql.org> — 2019-01-26T04:25:03Z

    > 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

  36. Re: Thread-unsafe coding in ecpg

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-26T14:57:10Z

    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

  37. Re: Thread-unsafe coding in ecpg

    Michael Meskes <meskes@postgresql.org> — 2019-01-29T11:58:59Z

    > 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