4
\$\begingroup\$

I'm asking myself if there's any better/shorter way of getting a module entry by its name from an external process.

This is the code I have so far:

MODULEENTRY32 GetModule(const char* ModuleName) {

    HANDLE Module = CreateToolhelp32Snapshot(TH32CS_SNAPMODULE, pid);
    MODULEENTRY32 Entry;
    Entry.dwSize = sizeof(Entry);

    WCHAR *ModuleNameChar;
    int Chars = MultiByteToWideChar(CP_ACP, 0, ModuleName, -1, NULL, 0);
    ModuleNameChar = new WCHAR[Chars];
    MultiByteToWideChar(CP_ACP, 0, ModuleName, -1, (LPWSTR)ModuleNameChar, Chars);

    while (Module32Next(Module, &Entry)) {
        if (!wcscmp((wchar_t*)Entry.szModule, ModuleNameChar)) {
            CloseHandle(Module);
            return Entry;
        }
    }

    /*
        Return the module base 0x0 if we don't find any module
    */
    CloseHandle(Module);
    Entry.modBaseAddr = 0x0;
    return Entry;
}

It should work but it's quite complicated. Is there any useful function or method I can make use of to simplify this whole thing?

\$\endgroup\$
  • \$\begingroup\$ I'd say this is pretty concise for WinAPI code. When I wrote a similar function once it turned out longer. \$\endgroup\$ – yuri Aug 1 '18 at 17:55
2
\$\begingroup\$

Unfortunately, I'm not sure of any way of being more concise.

There are a few changes you could make, but they're mostly optional.

  1. Check the return value of CreateToolhelp32Snapshot(). The documentation says the following:

    Includes all modules of the process specified in th32ProcessID in the snapshot. To enumerate the modules, see Module32First. If the function fails with ERROR_BAD_LENGTH, retry the function until it succeeds.

    I use CreateToolhelp32Snapshot() in an application that runs 24/7 and I know for a fact that I get the ERROR_BAD_LENGTH error here and there. In my case, I cannot afford false negatives, so I have to put this function in a loop. If you are okay with having false-negatives occasionally, then I wouldn't worry too much about it.

  2. I don't know if you're showing us the full code for this function, but you have a memory leak here:

    ModuleNameChar = new WCHAR[Chars];

    I don't see where you are deleting this buffer. I personally would use an std::vector<WCHAR> as a buffer, give it the correct size, and pass the address of the first element to MultiByteToWideChar().

  3. If you use HANDLEs a lot, consider wrapping them in an RAII container. That way, you don't have to worry about closing them manually. This will also make your code exception-safe if you ever add any functions that can throw (you actually already have one that can throw, the new call).

  4. I noticed you're not calling ::Module32First(). You probably don't need it, but I always have it anyway.

  5. This is a personal preference, but I think your upper-case variable names look absolutely horrendous. Please use snake-case or camel-case for variable names.

There's really nothing helpful to say unfortunately, the Windows API can be quite cumbersome.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ If the function fails with ERROR_BAD_LENGTH, retry the function until it succeeds. - this is of course documentation bug. you can note that CreateToolhelp32Snapshot not take any buffer size as input parameter. as result we can not increment buffer size before calling this api again. without this - call it again(with same parameters) absolute senseless - it again fail. this is how i say strange documentation bug \$\endgroup\$ – RbMm Aug 6 '18 at 13:06
  • \$\begingroup\$ @RbMm I have gotten this error before on real-life production servers. This isn't something I'm pulling out of my ass. This is definitely not a "pure" function, so you can get different results even with the same input. \$\endgroup\$ – ncalmbeblpaicr0011 Aug 6 '18 at 15:17
  • \$\begingroup\$ you can reproduce this ? i absolute sure that this is impossible even from generic point of view (i already not say that i know how this api internal working). simply because caller not control/pass any buffer size. and callee can not know - are this is first, or say second call. which buffer size used before. try set yourself on place who write this api - how in case ERROR_BAD_LENGTH retry call can help ? if function in some internal place got this error - it must not pass it to caller, but retry some internal call with bigger buffer. this like say - if you got access denied - try again \$\endgroup\$ – RbMm Aug 6 '18 at 16:37
  • \$\begingroup\$ yes, you correct. CreateToolhelp32Snapshot really can some time fail with ERROR_BAD_LENGTH - it read dll list from remote process, allocate memory, and then read this list again. if between this too calls new dlls will be loaded in target process, api can fail with this error (also because not used synchronization during read this double-linked list possible and another errors). this is (and by another efficient reason) because i never use this api yourself and preffer more reliable solution \$\endgroup\$ – RbMm Aug 8 '18 at 10:30
1
\$\begingroup\$

more simply api, way not exist.

1.) wrong return type - MODULEENTRY32. really impossible return structure, which not fit generic register size. when compiler view T fn(..) { * } it silently change this to void fn(T* t, ...) {*} and in your code Entry will be copied, what really not need. you must not try return MODULEENTRY32 but pass pointer to it as function argument.

2.) you lost error value. the GetModule can fail by different reasons - can not exist process with such id (STATUS_INVALID_CID), you can fail open target process (STATUS_ACCESS_DENIED), module can simply not exist in process. without returned error value - you can not know reason why function fail. really the best practice always return win32 error code (or NTSTATUS) from function.

3.) why you use module name as ansi string ? this is always bad and error design. use unicode name format. and you will be not need convert it (i already not say that if module name (this is rarely and never for system modules) use non english chars - it can not have correct ansi name at all(and depend from system locale - you encode it on self comp in one locale, on another comp - can be another locale. but ansi name depend from it).

4.) so signature of function must be ULONG GetModule(DWORD th32ProcessID, MODULEENTRY32* Entry, PCWSTR szModule)

5.) use wcscmp is wrong here. we need case insensitive compare. - _wcsicmp

6.) Return the module base 0x0 if we don't find any module - how i say - we need return appropriate error code in this case, instead set base address at 0 (we can left it undefined). client code must check not base address for 0, but error code for 0 (NOERROR).

7.) not forget about 32-64 bit issues here (are your and target process have some bit-ness)

so code can look like:

ULONG GetModule(DWORD th32ProcessID, MODULEENTRY32* Entry, PCWSTR szModule)
{
    HANDLE hSnapshot = CreateToolhelp32Snapshot(TH32CS_SNAPMODULE, th32ProcessID);

    if (hSnapshot != INVALID_HANDLE_VALUE)
    {
        ULONG dwError = ERROR_NOT_FOUND;

        Entry->dwSize = sizeof(MODULEENTRY32);

        if (Module32FirstW(hSnapshot, Entry))
        {
            do 
            {
                if (!_wcsicmp(Entry->szModule, szModule))
                {
                    dwError = NOERROR;
                    break;
                }
            } while (Module32NextW(hSnapshot, Entry));
        }

        CloseHandle(hSnapshot);

        return dwError;
    }

    // return RtlGetLastNtStatus(); 
    // this is much more better compare GetLastError();
    return GetLastError();
}

and usage:

MODULEENTRY32 Entry;
if (ULONG dwError = GetModule(*, &Entry, L"*"))
{
    // error, dont touch Entry
}
else
{
    //ok, use Entry
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1 You're answer makes some good points, but I disagree with #1. I believe many modern compilers can do copy elision and return value optimization, so passing a pointer or reference as an output is kind of old fashioned. \$\endgroup\$ – ncalmbeblpaicr0011 Aug 6 '18 at 15:24
  • \$\begingroup\$ @ncalmbeblpaicr0011 - may be in some case compiler and can internal optimize this for avoid extra copy. but i think programmer must understand that internally anyway to function will be pass pointer to caller allocated structure. are compiler can direct use this pointer instead local copy in function (MODULEENTRY32 Entry) is question. anyway i personal think that this is very bad style. and good style - always return error code from function (as result we also need return value for error reserve). \$\endgroup\$ – RbMm Aug 6 '18 at 16:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.