#6434 closed defect (fixed)
ALL: sscanf("%f"), atof() etc. not portable due to Locale
| Reported by: | eriktorbjorn | Owned by: | criezy |
|---|---|---|---|
| Priority: | normal | Component: | --Other-- |
| Version: | Keywords: | ||
| Cc: | Game: | Zork Nemesis |
Description
I noticed yesterday that Zork Nemesis fails to render properly. Fair enough, it's not yet a supported game, but the reason is worth bringing up: It uses sscanf() to parse floating-point values. This fails because the strings it parses use a period as decimal separator, while in my locale it's a comma. The result comes out looking less than the Great Underground Empire and more like the rings of Saturn.
Which raises the question, do these problems occur in any other parts of ScummVM, and how should we deal with them? A quick search suggests that the Wintermute engine may be affected, since it uses sscanf() and %f in base_persistence_manager.cpp, but there are several other format strings that are also used for floating-point values and I haven't looked for them. I assume atof() could also be problematic.
I'm not sure what the appropriate fix for this would be. Do we explicitly set the locale for ScummVM to use, or do we introduce or own locale-independent family of functions? Are there tools to scan for this kind of potential problems? (I'm actually a bit surprised that I haven't seen anything about it in our Coverity issues.)
Ticket imported from: #3615148. Ticket imported from: bugs/6434.
Change History (16)
comment:1 by , 11 years ago
| Summary: | Locale-related problems → General: Locale-related problems |
|---|
comment:2 by , 11 years ago
| Summary: | General: Locale-related problems → ALL: sscanf("%f"), atof() etc. not portable due to Locale |
|---|
comment:3 by , 11 years ago
| Status: | new → pending |
|---|
comment:4 by , 11 years ago
comment:5 by , 11 years ago
I don't know if changing locale would cause problems or not, or if that option is available on all platforms ScummVM supports. That's one reason I asked.
For what it's worth, I've seen a similar bug in another project handled by changing the locale, calling setlocale(LC_NUMERIC, "C") before the sensitive part and restoring it to its original value afterwards.
comment:6 by , 11 years ago
| Status: | pending → new |
|---|
comment:7 by , 11 years ago
Hmm... Probably another good reason to wrap sscanf and do this in just one place...
I'd suggest working this up into a Pull Request for comments...
comment:8 by , 11 years ago
The ANSI C standard says that by default a program starts with the 'C' locale which uses '.' as decimal separator. The issue here is that in the SDL backend on non-windows system, getSystemLanguage() calls setlocale(LC_ALL, "") to get the language. As fas as I know this is the only place where setlocale() is used.
So an alternative fix would be in OSystem_SDL::getSystemLanguage() to call setlocale(LC_ALL, "C") after we get the language to restore the C locale.
comment:10 by , 11 years ago
I can confirm that calling setlocale(LC_ALL, "C") in OSystem_SDL::getSystemLanguage() fixes my Zork Nemesis problem.
comment:11 by , 11 years ago
I would suggest that we implement this fix then... criezy, eriktorbjorn: You concur? If so, then I can commit the fix or criezy can...
comment:12 by , 11 years ago
As I said, it fixes my problem, so if it doesn't break anything else I'm fine with it. I'm not all that familiar with how locales work, but if "C" is the default I guess it should be ok.
comment:13 by , 11 years ago
Well, I don't think the side effect of setlocale was intended... and the other platforms will be using "C" locale or similar style... so this should improve portability and fixes the immediate issue.
I will commit.. and close this as fixed. Though this will be something we need to bear in mind in future.
comment:14 by , 11 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:15 by , 11 years ago
Fixed in commit de8da01b0e8a309b9ed3f5b0f152ebbcf8f4af37 to master. Closing as fixed...
comment:16 by , 6 years ago
| Component: | --Unset-- → --Other-- |
|---|---|
| Game: | → Zork Nemesis |
eriktorbjorn: Not sure that general problems like this should be filed as bugs as they tend to be very open ended and never get closed out... But to deal with the immediate issue, I'd suggest that we blacklist atof() and sscanf() as not portable and migrate all the relevant code to a scumm_sscanf() which wraps the system scanf, but errors if there are %f or %?f or %?.?f patterns in the format string... I would suggest that you look at a Pull Request of commits to implement this.
Apart from this, would setting the locale to specific cause major issues? At least then, the results would be defined and repeatable?