libinstpatch is hijacking the application's locale

[realnc]

libinstpatch/misc.c has this code in it:

if(!setlocale(LC_ALL, ""))
{
    g_critical("Error setting locale");
}

This effectively hijacks the locale handling of the application that uses libinstpatch. I believe it's a bad idea to call setlocale() in a library and it's pretty much impossible to fix this from the application side because setlocale() is not thread safe.

(In my case, I don't use libinstpatch directly, but I do use libfluidsynth, which in turn uses libinstpatch.)

[realnc]

Also, with that being said, doesn't setlocale(LC_ALL, "") make the preferences locale-dependent? It would mean that on a French locale decimal points in floating point settings should be , instead of . which doesn't sound like a good idea. The same settings would stop working on a system with a different locale. Could it be that what you wanted here is:

setlocale(LC_NUMERIC, "C");

(Even though this still hijacks the LC_NUMERIC locale of the application, mind you.)

[derselbst]

This originated from #26. @jjceresa ?

[realnc]

Example random breakage: https://github.com/realnc/dosbox-core/issues/7

[jjceresa]

@realnc

Example random breakage: realnc/dosbox-core#7

I understand what you said in previous message. However for the reason explained in https://github.com/swami/libinstpatch/pull/26 , for now, I don't see another solution that calling setlocale() inside libinstpatch. That means that if the application need another locale it should call setlocale() after calling ipatch_init() and this should be documented.

[realnc]

That means that if the application need another locale it should call setlocale() after calling ipatch_init() and this should be documented.

Well, I'm not calling that myself. Fluidsynth does. Also, it's not thread safe. If another thread in the application calls one of the many function that depend on current locale, you get race conditions. You can't do anything about it.

[jjceresa]

Well, I'm not calling that myself. Fluidsynth does ....

Not really, actually, fluidsynth library doesn't depend of the locale. But the application that use libinstpatchlibrary could depend of the locale. For example Swami is calling functionsof libinstpatch that are locale dependent.

[derselbst]

I must admit that I underestimated the scope of this setlocale change. I just had a first look into the IPatchXML code. It seems to me that sscanf() is the problematic part, used here.

https://github.com/swami/libinstpatch/blob/8c56777bb6e9711eb3c15dcf577c163d164c2481/libinstpatch/IpatchXmlObject.c#L852

I never had to deal with InvariantCulture file parsing in C before. So, the best solution I currently can come up with is:

char const *oldLocale = setlocale(LC_ALL, 0);
setlocale(LC_ALL, "C");
// read xmlfile
setlocale(LC_ALL, oldlocale);

But as already mentioned by @realnc, this is broken due to the missing thread-safety. One might look into glib, whether it provides some useful "sscanf()-locale-aware" alternative. Or we need to revert b45b2db and find an alternative way to support WinXP.

[realnc]

On POSIX you can use uselocale which exists to solve this exact issue. On Windows there's _configthreadlocale

[derselbst]

Aha, ok. So, in order to be absolutely bullet proof, we would need to do something like below, I'd guess:

#ifdef WIN32
char* oldLocale = setlocale(LC_NUMERIC, NULL);
int oldSetting = _configthreadlocale(0);
_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
setlocale(LC_NUMERIC, "C");
#else
locale_t oldLocale = uselocale((locale_t) 0);
locale_t newLocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
uselocale(newLocale);
#endif

// read xmlfile

#ifdef WIN32
setlocale(oldLocale)
_configthreadlocale(oldSetting);
#else
uselocale(oldLocale);
freelocale(newLocale);
#endif

Is that common practice? I've never seen code like that before.

[jjceresa]

Or we need to revert https://github.com/swami/libinstpatch/commit/b45b2db41d8a537d9e1ad9c8a904301a562e699e and find an alternative way to support WinXP.

Note this commit is just a way to give binaries for those that don't want to build from source. Also this static version of runtime library are know for having other side effects. For this reason I usually prefer using the DLL multithread option (/MD) who has also a significant lower size than the static one. So I would vote for reverting b45b2db. We could probably wait to find an other way to supply binaries.

---

It seems to me that sscanf() is the problematic part, used here.

Yes, I remember I was facing to sscanf that depends of the locale (note that printf() could depend of locale also). This locale dependency should also involved with libinstpatch functions writing xml file.

[jjceresa]

Is that common practice? I've never seen code like that before.

May be, this could be simpler ?.

#ifdef WIN32
int oldSetting = _configthreadlocale(0);
_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
setlocale(LC_NUMERIC, "C");
#else
locale_t oldLocale = uselocale(newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0));
#endif

// read / write xmlfile

#ifdef WIN32
_configthreadlocale(oldSetting);
#else
uselocale(oldLocale);
#endif

[derselbst]

That way you would leak the newlocale handle.

[jjceresa]

That way you would leak the newlocale handle.

Oups, yes!