Code review on Windows API usage

code review
1

Project linked here. I'm mostly concerned about proper error handling. In the code, I'm acquiring a process handle in order to override some bytes in its memory. You will also notice that I have little to no error handling. If it were me I would do an if statement after every Windows API call but that seems unnecessary for some reason. My question is - how should my thought process be, should I handle every potential pitfall or skip over some functions like GetWindowThreadProcessId that I know will, for most of the time, work just fine?

Also, is there a more optimal way to use the unsafe guard?

 unsafe {
            window = FindWindowA(PSTR::NULL, window_name);
            if IsWindow(window) == true {
                error!("{:?}", window);
                error!("Found window");
                let mut proc_id: u32 = 0;

                GetWindowThreadProcessId(window, &mut proc_id);
                error!("ProcID: {:?}", proc_id);

                let handle = OpenProcess(PROCESS_ALL_ACCESS, false, proc_id);
                error!("Handle: {:?}", handle);

                let base_addr = get_process_base_address(handle);
                let arr: [u8; 6] = [0x90, 0x90, 0x90, 0x90, 0x90, 0x90];
                WriteProcessMemory(
                    handle,
                    (base_addr + 0x7BF10D) as *mut std::ffi::c_void,
                    arr.as_ptr() as *const std::ffi::c_void,
                    arr.len(),
                    std::ptr::null_mut(),
                ); // FF 15 ? ? ? ? FF 15 ? ? ? ? 85 C0

                error!("Base address: {:#04x}", base_addr);

                CloseHandle(handle);
                break;
            }
        }

Would this kind of enclosing be innefficient?

I'm new to this language so please be as pedantic as possible! Thanks!!

2

It's not unnecessary. If a function is fallible, you should handle the errors, especially when dealing with unsafe. If handling a single error in itself is too verbose, you should abstract it away e.g. in a function that performs the checking of the error code, extraction to an error message, etc., and then call that (Result-returning) function in the usual style, with the ? operator.

In general you should minimize the scope of unsafe blocks and precede them with a comment justifying why that particular usage of an unsafe construct is sound.

Inefficient in what sense?

Why the explicit comparison to true? It's unnecessary.

3

Something I've done in the past is to create an extension trait that lets you convert from a return code to a Result<(), SomeError>.

trait ErrorCodeExt {
  fn to_result(self) -> Result<(), WindowsError>;
}

impl ErrorCodeExt for u32 {
    fn to_result(self) -> Result<(), WindowsError> {
     if self == 0 {
      Ok(())
    } else {
      Err(WindowsError::from_code(self)
    }  
   }
}

And you would use it like this:

fn main() -> Result<(), WindowsError> {
  unsafe {
    GetWindowThreadProcessId(window, &mut proc_id).to_result()?;

    ...
  }
}

Pull the code out into an unsafe function. Everything you are doing is unsafe, so the extra level of indentation isn't giving you much.

The return value from these sorts of functions are especially important to check. There's a good chance the error won't occur while you are hacking away locally, only for it to happen spuriously on an end user's machine.

Those sorts of bugs are incredibly annoying to track down.

You can lower the barrier to checking errors by using the ? operator and calling the method from ErrorCodeExt something short like check().

2 Likes
4

I agree but note that you have to be very careful with this. Windows APIs can sometimes have different ways to indicate an error even though they appear to return the same type. So you always need to double check the documentation first (and even then some areas of the documentations aren't always clear).

1 Like
5

Makes sense. I figured that because I'm messing around with such low-level functions the extra level of indentation wasn't going to matter.

I'll look into extensions, I think they would solve my issue surrounding error handling. It seems like a light way to wrap some of these Windows API calls without sacrifices.

EDIT: I'll mark this as solved since now I have a topic to look into, thanks for the answers.

1 Like
6

Yeah, in this case there isn't much you can do about it with the type system. You'd probably need to make different to_result() methods for the different circumstances and just "know" which one is right to use in a particular situation.

7

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.