Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libinstpatch is hijacking the application's locale #37

Closed
realnc opened this issue Mar 28, 2020 · 13 comments · Fixed by #38
Closed

libinstpatch is hijacking the application's locale #37

realnc opened this issue Mar 28, 2020 · 13 comments · Fixed by #38

Comments

@realnc
Copy link
Contributor

realnc commented Mar 28, 2020

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
Copy link
Contributor Author

realnc commented Mar 28, 2020

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
Copy link
Member

This originated from #26. @jjceresa ?

@realnc
Copy link
Contributor Author

realnc commented Mar 28, 2020

Example random breakage: realnc/dosbox-core#7

@jjceresa
Copy link
Contributor

@realnc

Example random breakage: realnc/dosbox-core#7

I understand what you said in previous message. However for the reason explained in #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
Copy link
Contributor Author

realnc commented Mar 28, 2020

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
Copy link
Contributor

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 libinstpatch library could depend of the locale. For example Swami is calling functions of libinstpatch that are locale dependent.

@derselbst
Copy link
Member

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.

ipatch_xml_default_decode_value_func(GNode *node, GObject *object,

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
Copy link
Contributor Author

realnc commented Mar 28, 2020

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

@derselbst
Copy link
Member

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
Copy link
Contributor

Or we need to revert b45b2db 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
Copy link
Contributor

jjceresa commented Mar 28, 2020

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
Copy link
Member

That way you would leak the newlocale handle.

@jjceresa
Copy link
Contributor

That way you would leak the newlocale handle.

Oups, yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants