Get poco-w95.exe running #39

LinusU · 2024-09-11T21:08:52Z

Hello! 👋

This is my first contribution so I'm sure that I got a lot of things wrong! 🙈

First of all, I just want to say that I think that this is a really cool project, and as someone who has worked on a gameboy emulator in Rust before, I like that this takes a similar approach. I'm currently trying to get the classic game Pocoman working.

Opening this for some early feedback since I've already made some changes to existing functions in kernel32.dll and I think that it would be great to get some feedback on that, and maybe even get those changes merged as I continue on with implementing more functions.

The changes to GetStartupInfoA was necessary since some garbage pointers was returned in cbReserved2 and lpReserved2 otherwise, which made the game crash with an oob error otherwise.

This is the binary I'm trying to get running: https://www.sleepless.com/pocoman/poco-w95.exe

Relevant documentation:

evmar · 2024-09-11T22:16:21Z

(On phone sorry for brevity)

This looks great so far! I will give a more thorough review when I get to a computer.

The changes look pretty right to me, but I expected shell32.rs to be empty, not contain generated code (?)

Re the startup info, I think the way the api is supposed to work is the caller provides the size of the struct, because the struct changed size in different windows versions and you want to be sure to only fill in the part they actually allocated space for. However as you observed I am not sure what was supposed to happen when the size param is zero, hm. Maybe there is some default minimum size to use or something like that? I will investigate when I get the chance.

Really cool you’re getting this working!

LinusU · 2024-09-12T10:56:29Z

The changes look pretty right to me, but I expected shell32.rs to be empty, not contain generated code (?)

This is indeed interesting. I had a hard time figuring out how to add a new dll at all, and I thought that I created the shell32.rs file as just an empty file in the beginning. Removing the content and running the make files again doesn't seem to add it back so I'm starting to wonder if I just accidentally pasted it from builtin.rs 😅

I'll remove it 👍

Re the startup info, I think the way the api is supposed to work is the caller provides the size of the struct, because the struct changed size in different windows versions and you want to be sure to only fill in the part they actually allocated space for. However as you observed I am not sure what was supposed to happen when the size param is zero, hm. Maybe there is some default minimum size to use or something like that? I will investigate when I get the chance.

I'll try to find some more info on this as well!

How would you prefer that I structure my work forward? I'm thinking now that it might have been better if this would have been two or three pull requests (one for GetStartupInfoA, one for SizeofResource, and possibly one for the stub shell32 but might be better to have that commit together with my first function implemented in it instead).

I think that maybe I would prefer having an issue open where I track my progress with links to PRs and list of my next steps. And then I'll submit PRs for each isolated change and rebase my working branch on your main branch after each PR merged. To put it more concretely: I would open a PR for GetStartupInfoA and one for SizeofResource. My next todo is advapi32.dll:RegCreateKeyA, so that would be the next PR which should also be free standing. If I do anything else that builds on e.g. the changes in the SizeofResource PR that would live in my branch until that has been merged, and then I'll open a PR for that.

I see a few benefits with this:

I can get things merged upstream quickly which hopefully helps you and other users
I can get feedback on the work I do quickly, so cascading changes are minimized
I would keep the PRs reasonably small, which should help your reviewing work, and make me responsible for updating work in progress code that I haven't yet submitted

Does this sounds good to you?

evmar · 2024-09-12T17:22:20Z

I'm happy to merge as you go, with whatever works best for you. Your proposal is sensible. My only caveat is that I have been really short on time lately (kid stuff), I am super embarrassingly behind on another PR that I am close to finally merging. I'd be happy to merge this as you go.

evmar · 2024-09-12T17:10:46Z

win32/src/winapi/kernel32/resource.rs

@@ -77,18 +81,18 @@ pub fn find_resource<'a>(
    hInstance: HINSTANCE,
    typ: ResourceKey<&Str16>,
    name: ResourceKey<&Str16>,
-) -> Option<&'a [u8]> {
+) -> Option<Range<usize>> {


This range is a span within the 32-bit memory right? For consistency I've been using u32s for these, which helps a bit with keeping straight which kind of things we're talking about. On wasm usize=u32 anyway but on other platforms it's 32-bit vs 64-bit.

evmar · 2024-09-12T17:11:39Z

win32/src/winapi/kernel32/resource.rs

+    let image = mem.slice(hInstance..);
+
+    let resource = &image[handle.1.clone()];
+
+    machine.mem().offset_of(resource.as_ptr())


If I followed this math, this could be just hInstance + handle.1, right?

Just to get all the pointers straight:

pe::find_resource returns a range that is within the image

maybe kernel32::find_resource should return a range that is within the process memory (so with hInstance already added)?

so then this would only need to return handle.1?

evmar · 2024-09-12T17:16:34Z

win32/src/winapi/user32/resource.rs

@@ -75,6 +75,8 @@ fn load_bitmap(
        ResourceKey::Id(pe::RT::BITMAP as u32),
        name,
    )?;
+    let image = machine.mem().slice(hInstance..);
+    let buf = &image[buf];


... and if find_resource returns Range, this can be something like mem().slice(range) or so?

evmar · 2024-09-12T17:17:55Z

win32/src/winapi/kernel32/resource.rs

    let image = mem.slice(hInstance..);
    if hInstance == kernel32.image_base {
        let section = kernel32.resources.as_slice(image)?;
-        Some(&image[pe::find_resource(section, typ.into_pe(), name.into_pe())?])
+        pe::find_resource(section, typ.into_pe(), name.into_pe())


Now that I'm looking, I wonder if pe::find_resource ought to return a Range as well, hm. If you look at its impl you can see it has u32s and it only casts to usize on the way out.

evmar · 2024-09-12T17:22:06Z

win32/src/winapi/kernel32/dll.rs

@@ -322,8 +322,11 @@ unsafe impl ::memory::Pod for STARTUPINFOA {}
 pub fn GetStartupInfoA(_machine: &mut Machine, lpStartupInfo: Option<&mut STARTUPINFOA>) -> u32 {
    // MSVC runtime library passes in uninitialized memory for lpStartupInfo, so don't trust info.cb.
    let info = lpStartupInfo.unwrap();
-    let len = std::cmp::min(info.cb, std::mem::size_of::<STARTUPINFOA>() as u32);
+    let len = std::mem::size_of::<STARTUPINFOA>() as u32;


By the way, I checked and I now believe that STARTUPINFOA has never changed in size, so my worries about callers passing in an undersized buffer or something like that are unfounded. And further I checked and at least win2k's kernel32.dll does exactly what you've done here, so I think this is correct.

LinusU · 2024-09-13T15:12:21Z

I have now managed to run enough of the program to realize that I'm running an installer, and not the actual game 😂

As I've understand GetStartupInfoA, the caller always passes a pointer to uninitialized memory, and the function will then fill in the values. Before this change, GetStartupInfoA read the value of cb as an input parameter to get the length of the struct. In my program that part of the memory was uninitialized and happened to be 0, so the rest of the values were never zeroed out, leading to garbage data being returned in the other field. This change make sure to zero out the entire struct, and then sets cb to the actual length of the struct.

LinusU · 2024-09-13T16:20:40Z

Thanks for the comments! I've adressed them and also implemented my plan resulting in the following things that replaces this PR:

LinusU changed the title ~~Get pocoman-w95.exe running~~ Get poco-w95.exe running Sep 11, 2024

LinusU force-pushed the pocoman branch from 8225a59 to 1fe6e44 Compare September 12, 2024 14:28

evmar reviewed Sep 12, 2024

View reviewed changes

LinusU force-pushed the pocoman branch from 36bfe07 to 5ca61fa Compare September 13, 2024 15:14

This was referenced Sep 13, 2024

Zero out entire struct in GetStartupInfoA #40

Merged

RegCreateKeyA and RegQueryValueExA stubs #41

Merged

Implement SizeofResource #42

Merged

Getting PocoMan.exe running #43

Open

LinusU added 9 commits September 13, 2024 18:18

add RegCreateKeyA stub

fb7f00a

add RegQueryValueExA stub

da06ed4

add handles for resources

9fbb275

implement SizeofResource

f728296

add stub shell32 dll

019de54

handle non-ascii in str16 to_string

a042859

implement SetCurrentDirectoryA

7e7ff88

wip: temporary give back ProgramFilesDir

82379de

add stub SetEnvironmentVariableA

8512734

LinusU force-pushed the pocoman branch from 5ca61fa to 8512734 Compare September 13, 2024 16:19

LinusU closed this Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get poco-w95.exe running #39

Get poco-w95.exe running #39

LinusU commented Sep 11, 2024 •

edited

Loading

evmar commented Sep 11, 2024

LinusU commented Sep 12, 2024

evmar commented Sep 12, 2024

evmar Sep 12, 2024

evmar Sep 12, 2024

evmar Sep 12, 2024

evmar Sep 12, 2024

evmar Sep 12, 2024

evmar Sep 12, 2024

LinusU commented Sep 13, 2024

LinusU commented Sep 13, 2024

Get poco-w95.exe running #39

Get poco-w95.exe running #39

Conversation

LinusU commented Sep 11, 2024 • edited Loading

evmar commented Sep 11, 2024

LinusU commented Sep 12, 2024

evmar commented Sep 12, 2024

evmar Sep 12, 2024

Choose a reason for hiding this comment

evmar Sep 12, 2024

Choose a reason for hiding this comment

evmar Sep 12, 2024

Choose a reason for hiding this comment

evmar Sep 12, 2024

Choose a reason for hiding this comment

evmar Sep 12, 2024

Choose a reason for hiding this comment

evmar Sep 12, 2024

Choose a reason for hiding this comment

LinusU commented Sep 13, 2024

LinusU commented Sep 13, 2024

LinusU commented Sep 11, 2024 •

edited

Loading