thcrap_update: handle games started through wrapper patches

[32th-System]

This fixes issue #69. This code might have errors in it that I didn't catch, so I would like to have these changes reviewed before merging them into master

[32th-System]

I have fixed a race condition issue, but there's still the issue that the ability for game files to be updated through wrapper patches is still tied to the loader window being open (which it isn't if background_updates is disabled). Should background_updates being disabled keep the window open but simply disable the 5 minute periodic update checking, or should I do something completely different?

[brliron]

I didn't read the source code in the commits yet, but to answer the question about changing background_updates: for thcrap, as a developer, I'm biased and I like the current default behavior, so I wouldn't mind too much. But if I look at my mindset for other launchers for other games, I see them as a tool to update the game before launching it. When I want to start the game, they do their thing, then I start the game, and at this point, they did their job, I don't want to see them anymore. And I definitely don't want them to keep a window open after I close the game, because way too often, I'll want to open another game, and because there's that launcher window already open, I'll reopen the previous game accidentally.

So, if I was a normal user for thcrap, I would disable background updates, and I would want thcrap_loader to close itself just after starting the game.

[32th-System]

It seems like update_at_exit was always broken with wrapper patches. To be honest, I'm not sure why this was added in the first place. Perhaps I could add one of the newer options, like console to the loader UI

[brliron]

IIRC this was a feature suggestion from Pook, he doesn't like waiting for update before his game starts, so that's an alternative where the game starts instantly, but we still update the game at some point: when the user is done playing the game and don't care about waiting anymore.

[32th-System]

I have fixed the buffer overflow and null termination issues by actually checking the size of the data I'm sending and also using a JSON format instead. I have also done what was suggested regarding the race condition of the launcher terminating between the CreateFileW and WaitForSingleObject call.

My idea for update_at_exit isn't the best. Leaving a process open with anything that uses the console subsystem will always leave the console window open, and vpatch's loader uses the console subsystem. I think the best thing to do is to either have some way to be able to wait for a process as well as any of it's subprocess to all exit ( so we'd catch the vpatch process launching a game and obtain a process handle to that game and also wait on that) or to have the ExitProcess detour do some IPC to tell the loader to perform an update

In any case, I'm ready for a second round of reviews

[brliron]

Nothing to say about the new batch of code changes, looks good!

For the design choice around update_at_exit, I'm not a huge fan either, but I can live with it. As an improvement, we could hide the console window. Msdn is providing contradicting informations saying that a function to get the console handle both exists (https://learn.microsoft.com/en-us/windows/console/getconsolewindow) and doesn't (https://learn.microsoft.com/en-us/troubleshoot/windows-server/performance/obtain-console-window-handle), I don't have a Windows PC to test right now so I'll assume that it exists, and then we can just call ShowWindow(hwnd, SW_HIDE) on it.

An alternative (which is probably a better solution overall) would be to send the new PID to the mailslot from inject_CreateProcess_helper. That way, the mailbox server in the loader knows about all the child processes and can wait on all of them.

[32th-System]

Alright, in terms of behaviour, everything now works the same way it worked in previous versions of thcrap (with the exception that wrapper patches are properly supported now). update_at_exit now also doesn't require that all wrappers be left running until all child processes terminate. I'm ready for the last round of reviews before this can be merged

[32th-System]

A few general things: -GetCurrentProcessId() would be preferable to the GetProcessId(GetCurrentProcess()) combo used in multiple places. -Since mailslots still depend on a separate event to signal reads/writes and the length being written is currently limited to a fixed size, would it make sense to use the same memory mapped file as the PID array instead of a mailslot? The data in the file could also be treated as a struct type to provide better type checking:

struct SharedMemory {
  DWORD pid_list[128];
  char json_message[1024];
};

I use the pid_list and JSON message for 2 related but still completely different things so I think it makes sense to separate them. But also, pid_list isn't always needed so it's not always allocated

[32th-System]

so I returned to the code after over a month and I don't see anything wrong and I tested it and it seems to work just like the current version except with proper wrapper patch support. I'm ready for the final round of reviews and then this could be merged