Opened 6 years ago

Closed 6 years ago

Last modified 13 months ago

#6434 closed defect (fixed)

ALL: sscanf("%f"), atof() etc. not portable due to Locale

Reported by: eriktorbjorn Owned by: criezy
Priority: normal Component: --Other--
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 eriktorbjorn, 6 years ago

Summary: Locale-related problemsGeneral: Locale-related problems

comment:2 by digitall, 6 years ago

Summary: General: Locale-related problemsALL: sscanf("%f"), atof() etc. not portable due to Locale

comment:3 by digitall, 6 years ago

Status: newpending

comment:4 by digitall, 6 years ago

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?

comment:5 by eriktorbjorn, 6 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 eriktorbjorn, 6 years ago

Status: pendingnew

comment:7 by digitall, 6 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 criezy, 6 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:9 by digitall, 6 years ago

criezy: That sounds like a good idea...

comment:10 by eriktorbjorn, 6 years ago

I can confirm that calling setlocale(LC_ALL, "C") in OSystem_SDL::getSystemLanguage() fixes my Zork Nemesis problem.

comment:11 by digitall, 6 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 eriktorbjorn, 6 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 digitall, 6 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 digitall, 6 years ago

Owner: set to criezy
Resolution: fixed
Status: newclosed

comment:15 by digitall, 6 years ago

Fixed in commit de8da01b0e8a309b9ed3f5b0f152ebbcf8f4af37 to master. Closing as fixed...

comment:16 by digitall, 13 months ago

Component: --Unset----Other--
Game: Zork Nemesis
Note: See TracTickets for help on using tickets.