GNOME Bugzilla – Bug 109446
Gnome Print shouldn't use setlocale
Last modified: 2005-08-15 01:53:42 UTC
Call isn't threadsafe, can wreak havoc with calling application, etc... It's generally bad form to use these in libraries. In most cases you can call g_ascii_dtostr and g_ascii_strtod instead
Created attachment 15278 [details] [review] Patch that addresses some of the problem
Created attachment 15281 [details] [review] obsoletes older patch
fortunately, the # of %f and %g formatting commands are few in number. should be possible to use g_ascii_dtostr or g_ascii_formatd in their places, use %s in the format strings, and remove the setlocale calls inside of the _printf functions.
Is this too controversial for 2.2.x?
I don't think it is controversial, though it seems that it might not be complete from Dom's comment.
no, it's certainly not controversial. my work here isn't complete, though it is a big step in the right direction. it should still probably be committed - in this case a partial solution is preferable to none at all.
Looking at the proposed patch, I don't think there is a guarantee that in gnome_print_config_set_length sprintf does not write beyond the length of the buffer. (It is very likely that unit->abbr is shorter than G_ASCII_DTOSTR_BUF_SIZE but that is the way to get into trouble.) We should be using snprintf instead of sprintf to avoid these silly problems.
fixed the easily removable occurrences of setlocale and added warnings to the others.
Getting rid of setlocale isn't going to play well with working around bugs in cups. (Although setlocale is thread-safe when used to query.)
I haven't touched those workarounds since we depend on them. There were various other places were using g_ascii_... could avoid them or the locale didn't really got into play.
I imagine that we could do... old = g_strdup (setlocale (..., NULL)); some_cups_call (); if (strcmp (old, setlocale (..., NULL)) { static gboolean warned = FALSE; if (!warned) { warned = TRUE; g_warning ("Working around locale bug in cups."); } setlocale (..., old); } g_free (old); ...which is almost thread-safe assuming that cups stops messing with the locale. ("Almost" because setlocale is not required to return identical strings even for identical locales.) It would also improve the chances of getting cups fixed, I bet. (Last I looked I couldn't even find a bug reporting address.)
I have implemented Morten's suggestion where we need to call setlocale to protect against cups bugs. Setlocale is still used in: gf_pso_sprintf gnome_print_pdf_fprintf gnome_print_pdf_page_fprintf gnome_print_ps2_fprintf gnome_print_transport_printf These are more difficult to replace. I guess we may have to look at where they are called and avoid using them with locale sensitive format strings.
There seems to be just two problematic ones: gnome-font-face.c: gf_pso_sprintf (pso, "%%!PS-TrueTypeFont-%g-%g\n", TTVersion, MfrRevision); gnome-font-face.c: gf_pso_sprintf (pso, "/FontBBox [%g %g %g %g] def\n", bbox->x0, bbox->y0, bbox->x1, bbox->y1); The former is trivial. The latter does not look hard either.
Upgrading the priority to High because of the patches attached.
this doesn't need to be "high" any more. andreas and morten have done a lot of work inside of gnome-print's CVS, and these patches are now obsolete.
Is it safe to say that a patch is needed, then?
it's safe to say that some more work needs to be done, but the work needed isn't "critical"
All right, so it would be okay if I switch out the PATCH keyword for PATCH_NEEDED?
What really needs to be done is to persuade the libcups people not to change the locale in their library.
See http://www.cups.org/str.php?L509+P0+S-2+C0+I0+E0+Q
Okay all setlocale calls have been removed except: 1) calls that only retrieve locale information (eg. for LC_MESSAGES) 2) the cups bug workaround calls. Unfortunately since cups changes the locale we have to reset it to the original value.
The CUPS people (Michael Sweet in particular) tells me that current versions of CUPS indeed do save and restore the locale across calls. We should verify this and simply require such a fixed CUPS library, IMHO.
It seems that libcups 1.2 does not exhibit the problem (it did still happen in 1.1.19).
libcups 1.1.20 also suffices. We have bumped the version requirement of libcups and removed all setlocale calls with non-NULL second argument.