Stop using GetNativePath on Windows #1876

Open
opened 10 months ago by Moonchild · 5 comments
Owner

nsIFile::GetNativePath is a bit of a footgun, because it returns char* -- this is a problem because it depends on the system code page what is a valid path and what is not, which can lead to problems if non-ascii paths are encountered for the installation or profile, since it breaks on unicode paths. This is primarily an issue on Windows since Linux (and assuming Solaris) defaults to UTF-8 which would mostly avoid the pitfalls of char* vs. code pages.

This can lead to problems like encountered in Topic 27829

For extension compatibility, we should not remove it from nsIFile, but at the very least we should stop using it ourselves.

See MoonchildProductions/GRE#3057 for previous work done.

`nsIFile::GetNativePath` is a bit of a footgun, because it returns `char*` -- this is a problem because it depends on the system code page what is a valid path and what is not, which can lead to problems if non-ascii paths are encountered for the installation or profile, since it breaks on unicode paths. This is primarily an issue on Windows since Linux (and assuming Solaris) defaults to UTF-8 which would mostly avoid the pitfalls of `char*` vs. code pages. This can lead to problems like encountered in [Topic 27829](https://forum.palemoon.org/viewtopic.php?f=3&t=27829) For extension compatibility, we should **not** remove it from nsIFile, but at the very least we should stop using it ourselves. See MoonchildProductions/GRE#3057 for previous work done.
Moonchild added this to the 31.1.0 milestone 10 months ago
Moonchild added the
OS: Windows
The whole codebase
labels 10 months ago
Poster
Owner

I've ported the patches from GRE but this doesn't solve the underlying issue with running it from a path with extended UTF-8 characters in it on Windows, which was the whole reason I started on this (it's on a separate branch). So this needs a closer look when there's enough time to do so (or if someone feels like they want to do the research).

I've ported the patches from GRE but this doesn't solve the underlying issue with running it from a path with extended UTF-8 characters in it on Windows, which was the whole reason I started on this (it's on a separate branch). So this needs a closer look when there's enough time to do so (or if someone feels like they want to do the research).
Moonchild removed this from the 31.1.0 milestone 9 months ago
Moonchild added the
Low Priority
Research
labels 9 months ago

There seems to be a way to kinda select UTF-8 for use with all the -A apis.

https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page

Just by adding this to the application manifest.

  <application>
    <windowsSettings>
      <activeCodePage xmlns="http://schemas.microsoft.com/SMI/2019/WindowsSettings">UTF-8</activeCodePage>
    </windowsSettings>
  </application>

With this the native paths could switch to UTF8 as in linux.

CP_ACP equates to CP_UTF8 only if running on Windows Version 1903 (May 2019 Update) or above and the ActiveCodePage property described above is set to UTF-8. Otherwise, it honors the legacy system code page. We recommend using CP_UTF8 explicitly.

This means CP_ACP can still be used and it would be a kinda trivial change.

There seems to be a way to kinda select UTF-8 for use with all the -A apis. https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page Just by adding this to the application manifest. ``` <application> <windowsSettings> <activeCodePage xmlns="http://schemas.microsoft.com/SMI/2019/WindowsSettings">UTF-8</activeCodePage> </windowsSettings> </application> ``` With this the native paths could switch to UTF8 as in linux. > CP_ACP equates to CP_UTF8 only if running on Windows Version 1903 (May 2019 Update) or above and the ActiveCodePage property described above is set to UTF-8. Otherwise, it honors the legacy system code page. We recommend using CP_UTF8 explicitly. This means CP_ACP can still be used and it would be a kinda trivial change.
Poster
Owner

Ah, Nice find!
Could you please check if placing Pale Moon in an extended UTF-8 named path works with that setting (currently crashes)

Ah, Nice find! Could you please check if placing Pale Moon in an extended UTF-8 named path works with that setting (currently crashes)

Seems to work, No crash. Sqliteadmin was able to use a DB-Name using Kyrillic and FireFTP was able to copy some files, got some connection issues but I think thats a problem on server side.
grafik

Seems to work, No crash. Sqliteadmin was able to use a DB-Name using Kyrillic and FireFTP was able to copy some files, got some connection issues but I think thats a problem on server side. ![grafik](/attachments/cc0a9249-b32c-4d58-9416-c47bcfc287ba)
177 KiB

I would say it works.
grafik

I created a PR.

I would say it works. ![grafik](/attachments/4e8b1efe-f677-4d8d-ac5a-9c5ba1cbd368) I created a [PR](https://repo.palemoon.org/MoonchildProductions/Pale-Moon/pulls/1899).
177 KiB
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: MoonchildProductions/UXP#1876
Loading…
There is no content yet.