Fix to failing to disassemble at the end of a memory region.

[BritishPiper]

Check #184 to see my comment on what this is attempting to fix.

[stevemk14ebr]

Hmm this is interesting, you can see from the return code I attempted to handle this case by checking for the partial copy error already.

To confirm what is occurring you are saying that when attempting to read a larger size of memory that is partially inacessible, rpm fails to copy the data even partially but does return the partial copy error (rather than another error)?

[BritishPiper]

Rpm fails to copy the first bytes of memory and returns ERROR_PARTIAL_COPY regardless. You can see by the variable "read" that is 0 after calling rpm indicating 0 bytes were read.

[stevemk14ebr]

Interesting, I did not realize it behaved this way. In that case, could you please update this pr to instead extend the normal safe_mem_read to detect when this happens by checking the read size field and error status, and then doing a second read with adjusted size like you do with virtual query. (So two calls to RPM would occur in this error case)

The intent of safe_mem_read is to always read as much as possible so there is no need for a separate ex variant

[BritishPiper]

Yeah, I can fix the PR. Also give me some minutes to make an example program to test RPM's behavior on your system as well.

[stevemk14ebr]

You might consider adding that reproduction as a unit test for the mem read utilities

[BritishPiper]

#include <Windows.h>
#include <iostream>
#include <iomanip> // std::hex and std::setw

int main(void)
{
    HANDLE hProcess = GetCurrentProcess();
    SIZE_T bufferSize = 15; // Size of the allocated buffer, you can adjust this as needed
    LPVOID lpBaseAddress = VirtualAllocEx(hProcess, NULL, bufferSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
    if (!lpBaseAddress)
    {
        std::cout << "Failed to allocate memory in the target process." << std::endl;
        return 1;
    }

    MEMORY_BASIC_INFORMATION info;
    VirtualQueryEx(hProcess, lpBaseAddress, &info, sizeof(info));

    // Write some data to the allocated memory (for demonstration purposes)
    BYTE dataToWrite[] = { 0xFF, 0x25, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
    SIZE_T bytesWritten = 0;
    LPVOID lpEndAddress = (char*)info.BaseAddress + info.RegionSize - sizeof(dataToWrite);
    if (!WriteProcessMemory(hProcess, lpEndAddress, dataToWrite, sizeof(dataToWrite), &bytesWritten))
    {
        std::cout << "Failed to write to the allocated memory." << std::endl;
        VirtualFreeEx(hProcess, lpBaseAddress, 0, MEM_RELEASE); // Cleanup the allocated memory
        return 1;
    }

    // Now let's read from the end of the allocated memory region
    SIZE_T bytesRead = 0;
    SIZE_T readSize = 100;

    BYTE* buffer = new BYTE[readSize];
    if (!ReadProcessMemory(hProcess, lpEndAddress, buffer, readSize, &bytesRead))
    {
        std::cout << "Failed to read memory with error " << GetLastError() << " (ERROR_PARTIAL_COPY is " << ERROR_PARTIAL_COPY << ")" << std::endl;
        std::cout << "Number of bytes read: " << bytesRead << std::endl;
    }
    
    std::cout << "Buffer Data:" << std::endl;
    for (SIZE_T i = 0; i < sizeof(dataToWrite); ++i)
    {
        std::cout << "0x" << std::hex << std::setw(2) << std::setfill('0') << std::uppercase << static_cast<int>(buffer[i]) << " ";
    }
    std::cout << std::endl;

    if (!ReadProcessMemory(hProcess, lpEndAddress, buffer, 14, &bytesRead))
    {
        std::cout << "Failed to read memory with error " << GetLastError() << " (ERROR_PARTIAL_COPY is " << ERROR_PARTIAL_COPY << ")" << std::endl;
        std::cout << "Number of bytes read: " << bytesRead << std::endl;
    }

    std::cout << "Buffer Data After Proper Read:" << std::endl;
    for (SIZE_T i = 0; i < sizeof(dataToWrite); ++i)
    {
        std::cout << "0x" << std::hex << std::setw(2) << std::setfill('0') << std::uppercase << static_cast<int>(buffer[i]) << " ";
    }
    std::cout << std::endl;

    // Cleanup: Free the allocated memory
    VirtualFreeEx(hProcess, lpBaseAddress, 0, MEM_RELEASE);
    delete[] buffer;

    return 0;
}

And my console output is:

Failed to read memory with error 299 (ERROR_PARTIAL_COPY is 299)
Number of bytes read: 0
Buffer Data:
0xCD 0xCD 0xCD 0xCD 0xCD 0xCD 0xCD 0xCD 0xCD 0xCD 0xCD 0xCD 0xCD 0xCD
Buffer Data After Proper Read:
0xFF 0x25 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00

[stevemk14ebr]

That's too bad they designed the API like that, it would be nice if it actually did the copying of the data it was able to read

[BritishPiper]

I've fixed one bug (ChatGPT trolled me) in the example provided and also made a proper read size after to see it's exactly the size parameter the problem. I'll see about also coding a unit test, I haven't messed with those yet.

[BritishPiper]

I've tested this new commit on my end and it seems to work. See if it's something like this that you'd want as final.
Microsoft's documentation says "If the function succeeds, the return value is nonzero.".
Therefore I didn't see a need to check for GetLastError(). I don't know if you'd want to check for "read == size" instead of "read > 0", but I guess since partial reads should be considered fine (as indicated by your previous code), then "read > 0" would suffice.

Not trying to be annoying, but I personally didn't like the current Unit Test structure, so I might end up not messing with it.

[stevemk14ebr]

I'm mobile so I can't do a proper GitHub style review, so I will just comment here the parts to change.

Make || read > 0 an and please. We want both the return value to be true and the read size to be non zero. And do please add the error value check so the retry only happens if it's a partial copy, we know partial copy is this particular scenario, it may filter out some other error cases where reads fail but we do not want to try again because it wouldn't work.

Don't worry about the unit test then, I can add alter in a few weeks. Thanks for working on this.

[BritishPiper]

Retested with the provided fixes, seems ok to me.