Hooks on nested functions are broken

[1ndahous3]

DRAKVUF has the same code concept, which, when copied from plugin to plugin, causes the same bug.

Some plugins, like apimon, use the map as a container to store hooks:

drakvuf/src/plugins/apimon/apimon.h

Line 145 in ec7af12

std::map<uint64_t, std::unique_ptr<libhook::ReturnHook>> ret_hooks;

So the plugin also uses a function to generate a key for this map:

drakvuf/src/plugins/apimon/apimon.cpp

Lines 126 to 131 in ec7af12

	static uint64_t make_hook_id(const drakvuf_trap_info_t* info)
	{
	uint64_t u64_pid = info->attached_proc_data.pid;
	uint64_t u64_tid = info->attached_proc_data.tid;
	return (u64_pid << 32) | u64_tid;
	}

Thus, according to the key, only one hook per thread in a process can be stored at a time.

The scenario when this concept breaks the functionality is when in a plugin (with such a map) we hook 2 functions, one of which calls the other (let it be kernel32!LoadLibraryExW() -> ntdll!LdrLoadDll()).

The following happens:

1. The hook on kernel32!LoadLibraryExW() is triggered, the plugin creates a ret hook, placing it in the ret_hooks map.
2. The hook on ntdll!LdrLoadDll() is triggered, the plugin creates a ret hook, placing it in the ret_hooks map with the same key, so the previous ret hook is destroyed (since all traps in classes based on BaseHook destroyed in dtor), replacing it with a new ret hook.
3. The ret hook from ntdll!LdrLoadDll is triggered, the return value is captured and the entire event is printed.
4. The is no ret hook from kernel32!LoadLibraryExW (it was removed on step 2), so we lose the entire event.

In apimon, the bug appeared in e2e6c3a, but this is just one of the typical bugs caused by copy-paste.

[tklengyel]

That certainly sounds like its bad practice. The key should probably include some other data so they can be distinguished and avoid these collisions.

[1ndahous3]

That certainly sounds like its bad practice. The key should probably include some other data so they can be distinguished and avoid these collisions.

This is the fix I thought of first, but I made a more reliable fix in #1711