Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does not work if my username contains non-ASCII characters (fix PR provided) #83

Closed
FRex opened this issue Aug 26, 2021 · 3 comments
Closed
Labels
bug Something isn't working fixed Fixed the bug

Comments

@FRex
Copy link
Contributor

FRex commented Aug 26, 2021

Hello. The file that stub loads is in the %TMP% directory. That path usually contains the username. In case of non-ASCII usernames there are two lines that prevent the game from starting:

  1. https://github.com/FenPhoenix/AngelLoader/blob/master/AngelLoader_Stub/dllmain.cpp#L139
  2. https://github.com/FenPhoenix/AngelLoader/blob/master/AngelLoader_Stub/dllmain.cpp#L181

Attempting to convert a path to std::string when there are non-ASCII characters causes an exception, terminating the game (but see note at the end).

You can test this yourself:

  1. add two local accounts, one with ASCII username, one with Unicode username.
  2. Find Angel Loader exe, hold shift, right click, select 'run as another user', and try with the new ASCII user, it should work.
  3. If everything worked correctly, try the same with non-ASCII user, it should not work.

Let me know if you cannot reproduce this.

If you can reproduce it, then you can test the fix I have created here: #82

The fix is to:

  1. use args_file.c_str() to get a wchar_t pointer and preserve the Unicode, the std::ifstream constructor has an overload that handles wchar_t *.
  2. user proper C++17 filesystem library remove function instead of std::remove (which is old C char * only function) - https://en.cppreference.com/w/cpp/filesystem/remove

Another possible change would be to not use a temporary file but use an environment variable to pass the data from Angel Loader to stub, but it'd be more invasive than this fix.

I am experienced in C and C++ so if you would like any other changes done to the stub (cleaning up the code, slimming it down, or looking into your problem with fopen) then let me know.

NOTE: I am not sure if language set on the PC matters, e.g. if a French user on French Windows would not have this problem. My Windows is set to English (US) so there is a mismatch between Windows language and the diacritics used in the username.

@FenPhoenix
Copy link
Owner

I made a new user called "Unicodeグ" but can't reproduce the issue. The game starts fine and FMs run fine for me even using that account. But your fix does no harm either, so I guess I'll accept it.

Feel free to clean up the stub. I freely admit I know little to nothing about C++ and I wrote it by googling for documentation, doing what the static analyzer suggested, and just kind of bumbling through until it worked. If you feel you can make some more improvements then by all means!

I think the problem with fopen was probably that originally I was accidentally writing the temp file with UTF8 BOM (so it had the FEFF bytes at the beginning) and it didn't like that. I remember something about garbage characters at the start so maybe that was it. I could be wrong. Honestly I don't care that much as long as what's there now works, but if you wanna try your hand then be my guest.

Regarding passing the data in an environment variable, I stay well away from that due to this sort of thing:
https://devblogs.microsoft.com/oldnewthing/20100203-00/?p=15083
Sure, we probably wouldn't hit any limits, but I just don't like how janky it all is. Using a temp file may not be a refined solution, but it works well in my experience (unless we have a bug like this of course).

@FRex
Copy link
Contributor Author

FRex commented Aug 26, 2021

I knew of the environment variable limit but I've assumed it'd be generous enough for your needs or that you could use more than one and use one variable for each value you need instead of parsing a file yourself. Nevermind it if you are not interested.

Also my PR uses the non-standard wide char std::ifstream constructor, which is Microsoft specific addition (I'm not sure how available it is on other compilers on Windows), but I assume MSVC is (for now?) the only compiler you care about.

I'm not sure what can cause your system to not produce the exception. I'm guessing you have somehow globally set your default Windows codepage to 65001 which is UTF-8 or made other changes in your registry (which I am afraid to test in case it bricks my Windows install). On modern Windows 10 versions there was a lot of work done by Microsoft to support UTF-8 for filenames, char * APIs, etc.

Here's a test program with a string (グąáüñж汉한ẞשָׁї́) containing characters from many non-ASCII languages. For me it prints "1252" and "No mapping for the Unicode character exists in the target multi-byte code page." (this is the exception message).

The 1252 is codepage https://en.wikipedia.org/wiki/Windows-1252 , and if I use only characters from it (accents, ñ, umlauts, etc.) it does convert the string properly.

If you want to verify it then you could try Angel Loader with non-ASCII username or this test program on a freshly created VM of Windows 7, 8 or 10. I did try on a VM of 10 yesterday, but I was doing something wrong with setting up Thief and didn't have time to try again yet. If you want to find codepoints of given characters you can use my website: https://frex.github.io/unicode.html

#include <Windows.h>
#include <filesystem>
#include <stdexcept>
#include <iostream>
#include <string>

int main(void)
{
    const wchar_t w[] = {
        L'a', 0x30B0, 0x105, 0xE1, 0xFC, 0xF1, 0x436, 0x6C49, 0xD55C,
        0x1E9E, 0x5E9, 0x5B8, 0x5C1, 0x457, 0x301, L'b', L'\0'
    };

    std::cout << GetACP() << std::endl;
    std::filesystem::path x(w);
    try {
        std::cout << x.string() << std::endl;
    }
    catch(const std::exception& e) {
        std::cerr << e.what() << std::endl;
    }
}

@FRex FRex closed this as completed Aug 27, 2021
@FRex
Copy link
Contributor Author

FRex commented Aug 27, 2021

I've closed it since the fix PR was merged. If I ever improve the stub I'll make another PR and issue.

@FenPhoenix FenPhoenix added bug Something isn't working fixed Fixed the bug labels Sep 2, 2021
@FenPhoenix FenPhoenix assigned FenPhoenix and unassigned FenPhoenix Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Fixed the bug
Projects
None yet
Development

No branches or pull requests

2 participants