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

currently known Windows-related bugs #1

Closed
time-killer-games opened this issue May 23, 2022 · 4 comments
Closed

currently known Windows-related bugs #1

time-killer-games opened this issue May 23, 2022 · 4 comments

Comments

@time-killer-games
Copy link
Owner

Getting a environ, cwd, and cmdline from a process that is a different architecture than the calling process on Windows (for example: x86 <-> x86_64) this is handled going both ways using a very convoluted approach: first I build a command line exe which prints to the terminal the environ, cwd, or cmdline (choosing one if those based on the arguments passed to the exe) and using C++ std::string literal null characters to separate each environment variable or command line argument, while double null terminating the string at the end. The cwd is only one string, so it is single null terminated. This executable is then converted to unsigned char array, using the terminal command xxd, which i have provided to my build script thanks to MSYS2, although I will need something else to replace the xxd command when building with visual studio, which i currently have no solution for, but will think of something. This unsigned char array is written to to the temporary path of the calling process, (as retrieved from GetTempPathW() and narrowed back down as a std::string for UTF-8 support). Doing it this way works for a calling x86 process reading process information from a x86_64 process, as well as an x86_64 process reading from a x86 process.

Going both ways like this is not possible with any known official Microsoft API, whether documented or undocumented, mainly due to the limitation of 64-bit pointers being a larger than 32-bit pointers, you get safety issues like truncation which will crash your program if you try to read a 64-bit pointer in a 64-bit process address space from a 32-bit calling process, at least, in any "normal" way. This is why I take this complicated approach. Technically, you can read the other direction with an undocumented MS API which allows you to read 32-bit process information from a 64-bit process, but for simplicity's sake, i decided to take the same approach going both directions between 32-bit <-> 64-bit communication, also because I want to discourage people relying on mixed architecture process communication on Windows, as it is very slow when doing 32-bit from 64-bit with my approach, its just not a good idea to rely on mixing architecture in general because most people who target Windows expect to port their software to 32-bit, and they will end up wondering why the 32-bit app they wrote is very slow compared to the 64-bit, even more than what is normally the case between those two architectures than it should be under any normal circumstance. The feature is there for those who need it, but it's very discouraged with 32-bit slowly dying as a platform in general. My though is, if you want to support 32-bit for anything, you need a very good reason, and in most cases, there just isn't one outside of backwards compatibility and being able to use closed source software that was never ported to 64-bit. These things will be mentioned in the boost documentation.

Building with MSYS2's g++ compiler works on Windows and has no memory leaks that i could tell, which is very odd because building with MSVC has very noticable memory leaks when reading between two architectures with the convoluted method I previously had mentioned, it is almost always guaranteed to crash when reading a single environ from the process of a different architecture. This problem happens with both 32-bit and 64-bit apps built in the MSVC compiler. Reading the cmdline cwd also shows evidence of memory leaks in my testing with this compiler, however, due to the nature of those strings not typically being as long as the environ, they don't crash immediacy on the first time reading from the other process of different architecture, it usually takes a bit longer for the segmentation fault to manifest with those for obvious reasons.

I used to think it was a stack overflow in MSVC since it is limited to 1MB by default on the stack. However I don't I was using that much, and after increasing the stack and such to be much, much less limited, a lot higher, still, it made no difference and continued to crash exactly the same as it did before in all the same places. Due to the nature of this bug, it might be a bug with the GNU g++ compiler itself that my program even works when compiled with that without segfaulting. However, It very well could be a bug with the MSVC compiler that my program doesn't work, instead. It's hard to say. But my theory is it would be a lot more likely for the g++ compiler to have bugs than for Microsoft's officially supported compiler, that is actually written by Microsoft and not some third party hacking away at Windows being blocked by a Chinese wall, or reverse engineering, when needed. It is much more plausible for g++ to be buggy than for MSVC to be. Also to be considered here, I am using std::string::data() which ensures the string will not have null characters, yet oddly enough, though I was using that for safety purposes (i needed the string to be modifyable as a char pointer and not a const), this came with the cost of removing null character going this particular route to retrieve a char pointer from the std::string, where as there are other ways to go that without removing null characters, the documentation suggests std::string::data() in C++14 and high will return a non-null terminated char pointer, and not a const char pointer like in older C++ versions. I am using and intend to use a more modern version of C++, but to my mistake I ended up using something to get a char pointer that wasn't correct for my needs to directly keep in tact the null characters, as I am using them for a delimiter character, and need them to be reserved for that otherwise I would use a delimiter that could potentially be needed by the user in their environ or cmdline, and would break things for them. Anyway, as I mentioned, despite what the documentation said, to my surprise, the null character were not removed from the string once I converted the std::string to a char pointer via std::string::data(). This happens with both compilers, not just g++ but also in MSVC when I test with the cmdline instead of the environ to so it won't immediately crash. This appears to be a bug with both compilers; very odd.

In short, I am relying on multiple compiler bugs, to get this code working at any level, regardless of the compiler in use. This only applies to environ, cmdline, and, (possibly), the cwd. To be technical I'm not sure if cwd is causing a memory leak in MSVC although I know from testing at least that both cmdline and environ in fact are, because they have that null character issue I mentioned, which makes this issue more plausible using my best educated guess and reasoning. I haven't tested with any debugger yet, only noticed the crashes that happen under very specific circumstances, so I didn't feel the need to debug it any beyond observing the crashes themselves, it makes enough sense to me what the root of the problem is based on the timing of the segfaults when using MSVC to compile. I'm actually not sure if I'm building with MSVC or clang, all I know is I'm building with whateve Visual Studio's IDE uses by default, though I do have both those compilers installed with my Visual Studio installer. I think its a problem regardless of whether I use MSVC or clang as I do remember building with MS's build of clang and it had the crash as well as whatever the default compiler is.

All of these things need to be addressed by some means as a top priority before any of my code may be considered into becoming a part of boost.

@klemens-morgenstern
Copy link
Collaborator

Why are you converting wchar to char? Shouldn't you be handling wchar_t instead?

@time-killer-games
Copy link
Owner Author

time-killer-games commented Jul 9, 2022

Why are you converting wchar to char? Shouldn't you be handling wchar_t instead?

You probably know more about programming than I do, so I will assume you are correct in that it is probably better to use wchar_t. but for the code to be cross-platform, Windows is the only platform, with my limited understanding, where it makes sense to use wchar_t, because all other platforms prefer char for all their API's. Although I'm not really sure what difference it makes. I could convert all platforms to use wchar_t to cater to Windows, or i could just make Windows use char to be like all other platforms.

Here's the part where you inform me my initial decision was not a good idea and why. 👍

@klemens-morgenstern
Copy link
Collaborator

char is usually the right way to go, unless you directly point to windows strings basically. The question is if you always copy - in which case converting to char with utf-8 is imo the right choice - or if you can point directly to the memory. If it's the latter you'd need to support wchar_t. See my env impl in v2 for how I solved that.

@time-killer-games
Copy link
Owner Author

time-killer-games commented Jul 11, 2022

My question regarding what you are saying, since I'm still not 100% clear on what you mean.

For example, are you saying I should use GetModuleFileNameA here and avoid wchar_t completely?

Wouldn't this kill off UTF-8 support?

if (proc_id == proc_id_from_self()) {
wchar_t exe[MAX_PATH];
if (GetModuleFileNameW(nullptr, exe, MAX_PATH) != 0) {
static std::string str; str = narrow(exe);
*buffer = (char *)str.c_str();
}
} else {

Or are you referring more to code like this right here:

wchar_t *res = new wchar_t[len / 2 + 1];
ReadProcessMemory(proc, buf, res, len, &nRead);
if (!nRead) return; res[len / 2] = L'\0'; *buffer = res;

...and you mean to use the the memory as a char * directly, and not copy it as a new wchar_t *?

Edit:

Checking your process v2 lib environment code now.

Edit2:

To be honest, I can't easily read your code, it uses a lot of stuff I'm not familiar with and it will take a while for me to look up docs on and digest a bit. However I gave you write access to this repository. Feel free to edit anything without my consent, but please do let me know what your changes do regardless if that's ok, each time you push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants