Closed Bug 1700870 Opened 4 years ago Closed 4 years ago

MultiInstanceLock handles character encodings incorrectly

Categories

(Toolkit :: Application Update, defect, P1)

defect

Tracking

()

RESOLVED FIXED
89 Branch
Tracking Status
firefox89 --- fixed

People

(Reporter: emk, Assigned: agashlin)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

GetLockFileName converts Unicode strings to nsCString using nsPrintfCString:
https://searchfox.org/mozilla-central/rev/6c9ff2820d3aae683ec87c53c79e5108d54f3f76/toolkit/xre/MultiInstanceLock.cpp#43
nsPrintfCString uses the ANSI code page:
https://searchfox.org/mozilla-central/rev/6c9ff2820d3aae683ec87c53c79e5108d54f3f76/mozglue/misc/Printf.cpp#1042,1052
Then OpenMultiInstanceLock convert the string back to UTF-16 using NS_ConvertUTF8toUTF16, which is wrong:
https://searchfox.org/mozilla-central/rev/6c9ff2820d3aae683ec87c53c79e5108d54f3f76/toolkit/xre/MultiInstanceLock.cpp#65

If the ProgramData folder path contains a non-ASCII character, this code does not work (although it usually does not).

Thanks for the detailed report, :emk -- we'll try to address this quickly.

I'll get to this shortly, along with bug 1697955. Fortunately it's probably quite rare for a system to have a ProgramData like this, :emk can you think of how it might occur, or did you encounter this in a sweep of nsPrintfCString misuses?

Assignee: nobody → agashlin
Severity: -- → S3
Status: NEW → ASSIGNED
Priority: -- → P1

I happened to find a bad code smell when I look at the patch of bug 1696772.
In general, CreateFileW(PromiseFlatString(NS_ConvertUTF8toUTF16(filePath is a bad signal unless you are sure filePath is encoded in UTF-8. (C++20 char8_t, please!)

nsPrintfCString %S converts from UTF-16 to the ANSI code page, but we rely on the output string being UTF-8.

Pushed by agashlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/32a9e6e145d6 Explicitly convert path to UTF-8. r=application-update-reviewers,bytesized
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: