setlocale as used inside pj_init_ctx is not threadsafe

[proj4-bot]

Reported by benharper on 4 Oct 2013 15:17 UTC
setlocale is used inside pj_init_ctx, but this is not thread safe.
I have not seen this to be a problem until upgrading to Visual Studio 2012, where it does manifest in heap corruption.

Possible solutions that I can think of:

1. Add a parameter to pj_init_ctx which means "leave the locale alone".
2. Avoid the use of any functions that depend on the locale.
3. Add a check inside pj_init_ctx such as this:

if defined(_MSC_VER) && _MSC_VER >= 1700

/* You must use _configthreadlocale(_ENABLE_PER_THREAD_LOCALE)
to ensure that each thread has its own locale, otherwise
this function will cause heap corruption if called from
multiple threads. */
if (_configthreadlocale(0) != _ENABLE_PER_THREAD_LOCALE) {
pj_ctx_set_errno(ctx, PJ_THREAD_LOCALE_RACE);
goto bum_call;
}

Migrated-From: https://trac.osgeo.org/proj/ticket/226

[proj4-bot]

Comment by warmerdam on 20 Oct 2013 18:09 UTC
I see the pj_init_ctx() code now looks like this:

    old_locale = setlocale(LC_NUMERIC, NULL);
    if (old_locale != NULL) {
       if (strcmp(old_locale,"C") != 0) {
      setlocale(LC_NUMERIC,"C");
      old_locale = strdup(old_locale);
       }else
      old_locale = NULL;
    }

So it seems that the old locale is copied with strdup() rather than assuming the internal string will last indefinately. Does that not mitigate the problem? Are you using this version? (I realize there is still a race condition, but it seems this would not hit often, and we could put a lock around it).

I'm a bit hesitant to put in the proposed code change partly because it is quite rude of a sublibrary like PROJ.4 to change whether or not locales are per-thread. Certainly applications running into this problem could do it in advance.

[proj4-bot]

Comment by whodevelopment on 13 Nov 2013 12:42 UTC
Hi all,

IMHO it is even more rude for a sublibrary to change the locale using setlocale in the first place. On posix systems uselocale can be used for thread-local locales, on windows benharper's suggestion (Or just write a floating-point parser).

Also "Putting a lock around" it is no fix for the problem: it would require any code that even reads the locale to acquire that lock (ie. also code in other libraries).

Regards,

See:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/setlocale.html
http://pubs.opengroup.org/onlinepubs/9699919799/functions/uselocale.html
http://www.lehman.cuny.edu/cgi-bin/man-cgi?setlocale+3 (Notes)

[wilhelmberg]

I haven't found the reason, why this problem is triggered on some machines and not on others (with basically the same setup - at least regarding locale and number format), but I've found two ways to work around it:

1. comment out reading and setting of locale
2. replace setlocale with _wsetlocale

---

@ 1. is a bit brute force and doesn't seem to be appropriate and I think, I was just lucky to not experience any side effects, as the faq states:
If a locale is in effect that modifies formatting of numbers, altering the role of commas and periods in numbers, then PROJ.4 will not work.

---

@ 2. was inspired by the comment of user TolonUK in MS Connect: VS2012: C++: Exception in multithreaded calls to setlocale:

..., the program runs fine if I change the setlocale() call to _wsetlocale(). I tried the same in VS2010 and the situation is reversed.

That is:

For VS2012/13, simultaneous calls to setlocale(LC_NUMERIC, NULL) cause an AV, but simultaneous calls to _wsetlocale(LC_NUMERIC, NULL) are fine.
For VS2010, simultaneous calls to setlocale(LC_NUMBER, NULL) are fine, but simultaneous calls to _wsetlocale(LC_NUMERIC, NULL) cause an AV.

---

The solution could be, to check for VS version (<2012 and >=2012) and then either call setlocale or _wsetlocale?

[rouault]

Aouch about the differences in behaviours between VS versions ! It seems to me that this is completely unspecified behaviour and might change again in the next releases.

I'd rather see 2 other solutions :

• messing up with per-thread locales, as I've done in GDAL very recently https://trac.osgeo.org/gdal/changeset/29412/#file3 (this is when interacting with external libraries to GDAL, eg mongo db connector). But such per-thread locale mechanisms are very OS dependant, I could only find proper solutions for Linux (uselocale) and Windows (_configthreadlocale(_ENABLE_PER_THREAD_LOCALE)) (no MacOSX, no Android, etc...)
• use a locale insensitive atof(). This is available in GDAL too and used extensively in the GDAL source tree : CPLAtof() in https://trac.osgeo.org/gdal/browser/trunk/gdal/port/cpl_strtod.cpp

Although it will require editing more files, I'd rather favor the local insensitive atof() approach

[wilhelmberg]

I'd rather favor the local insensitive atof() approach

👍

messing up with per-thread locales, as I've done in GDAL very recently

Oh, interesting, I also had tried ENABLE_PER_THREAD_LOCALE, but it didn't work for me.
Maybe I put it in the wrong place.