Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LoadMemModule will crash in the second time call if donot FreeMemModule the module return by the first time call LoadMemModule #13

Closed
kyle-go opened this issue Mar 15, 2019 · 3 comments

Comments

@kyle-go
Copy link

kyle-go commented Mar 15, 2019

If donot call FreeMemModule, LoadMemModule will crash in the second time call.
In my opinion, this will cause a memory leak or resource leak, but it crash. There must be sth wrong.

CRASH IN LINE 947 of file mmLoader.c

BOOL
CallModuleEntry(PMEM_MODULE pMemModule, DWORD dwReason) {
  if (NULL == pMemModule || NULL == pMemModule->pImageDosHeader)
    return FALSE;

  PIMAGE_NT_HEADERS pImageNtHeader =
      MakePointer(PIMAGE_NT_HEADERS, pMemModule->pImageDosHeader, pMemModule->pImageDosHeader->e_lfanew);

  Type_DllMain pfnModuleEntry = NULL;

  // If there is no entry point return false
  if (0 == pImageNtHeader->OptionalHeader.AddressOfEntryPoint) {
    return FALSE;
  }

  pfnModuleEntry = MakePointer(Type_DllMain, pMemModule->lpBase, pImageNtHeader->OptionalHeader.AddressOfEntryPoint);

  if (NULL == pfnModuleEntry) {
    pMemModule->dwErrorCode = MMEC_INVALID_ENTRY_POINT;
    return FALSE;
  }

 // ⬇⬇⬇ THIS LINE WILL BE CRASH  ⬇⬇⬇
  return pfnModuleEntry(pMemModule->hModule, dwReason, NULL);
}

Thanks! Best regards.

Demo Code:

...
while (true) {
    // Load the module from the buffer
    hMemModule = (HMEMMODULE)MemModuleHelper(MHM_BOOL_LOAD, moduleBuffer, (LPVOID)FALSE, &dwErrorCode);

    if (hMemModule) {
      _tprintf(_T("Module was loaded successfully. Module Base: 0x%p!\r\n"), (LPVOID)hMemModule);

      // will crash in second time call 
      LPVOID lpAddr = (LPVOID)MemModuleHelper(MHM_FARPROC_GETPROC, hMemModule, "demoFunction", 0);
      if (lpAddr) {
        _tprintf(_T("Get address of demoFunction successfully. Address: 0x%p!\r\n"), lpAddr);

        // Function pointer type of demoFunction
      typedef BOOL(_stdcall * Type_TargetFunction)(unsigned char *, unsigned int);

        // Call the demoFunction
        Type_TargetFunction pfnFunction = (Type_TargetFunction)lpAddr;

        unsigned char buf[MAX_PATH] = {0};
      if (pfnFunction(buf, MAX_PATH)) {
          printf("%s\n", buf);
        } else
          _tprintf(_T("Failed to get address of demoFunction from memory module.\r\n"));

        // donot free the module.
        //MemModuleHelper(MHM_VOID_FREE, hMemModule, 0, 0);
      }
    } else
      _tprintf(_T("Failed to load the module!\r\n"));
  }
@kyle-go kyle-go changed the title LoadMemModule will crash in the second time if donot FreeMemModule the module return by the first time call LoadMemModule LoadMemModule will crash in the second time call if donot FreeMemModule the module return by the first time call LoadMemModule Mar 15, 2019
@tishion
Copy link
Owner

tishion commented Mar 23, 2019

@KyleScript I believe this is caused by the DLL you were loading.

Currently, mmLoader doesn't have management on the loaded modules, that means if you load the same module twice, mmLoader will really try to load it twice(two section copies in the memory). Generally, in this case mmLoader should only increase the reference count instead of loading it again. What is worse if the module doesn't support multiple instances loading it will crash the process.

So this is not a small bug to fix, this is a new feature to be implemented. I am adding this feature to the list.

Thanks.

@kyle-go
Copy link
Author

kyle-go commented Mar 26, 2019

@KyleScript I believe this is caused by the DLL you were loading.

Currently, mmLoader doesn't have management on the loaded modules, that means if you load the same module twice, mmLoader will really try to load it twice(two section copies in the memory). Generally, in this case mmLoader should only increase the reference count instead of loading it again. What is worse if the module doesn't support multiple instances loading it will crash the process.

So this is not a small bug to fix, this is a new feature to be implemented. I am adding this feature to the list.

Thanks.

YES, U R right! I tried again with the dll in your demo project. Nothing happened, it's not crash. so, the crash must be caused by the dll. Regrettably,I cannot Reproduce the BUG.

Thanks.

@tishion
Copy link
Owner

tishion commented Mar 29, 2019

After think twice, I realized this feature is not feasible. because all the modules are loaded from buffer, there is no data used as the unique key of the module like path.

Although we could get the hash digest of the PE file header and use it as the unique key, but I think this is meaningless. If you really need this feature you can implement it by yourself.

Thanks. I am closing this.

@tishion tishion closed this as completed Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants