win32_hook: preserve ordinal forwarding

[wusikijeronii]

Description

Fix incorrect forwarding of ordinal-based GetProcAddress calls in the Win32 hook layer.

On some systems (e.g. WinAppSDK / CoreMessagingXP), GetProcAddress can be invoked using an ordinal (e.g. USER32 ordinal 0xA16). The current hook attempts to resolve such calls via the OrdinalNames mapping and forward them as name-based lookups. If the mapping entry is missing or invalid, this results in forwarding with an incorrect name and subsequent lookup failure (ERROR_PROC_NOT_FOUND), which can lead to application termination.

This change preserves the original request for ordinal-based lookups:

• Detect whether the request is ordinal or name-based.
• For ordinal requests, forward using the original ordinal value.
• If ordinal-to-name mapping is unavailable or empty, bypass translation and pass the original request directly.
• Name-based requests continue to behave unchanged.

The fix is limited to renderdoc/os/win32/win32_hook.cpp and only affects ordinal handling in Hooked_GetProcAddress.

Fixes: #3822

[baldurk]

Thanks for finding this, it's a little concerning that user32.dll intermittently has missing ordinal names but it is what it is.

I think in terms of the change it would be better to flip this on its head - leave func alone throughout the function (it could be marked as a const parameter that won't affect the ABI) and instead copy func to a local before the ordinal lookup, update that local inside the if, and use it for the initialisation of search.

If the name ends up as empty or otherwise invalid then the search will fail and we'll fall through and use the original func. From looking at the change it seems like the underlying problem was that func got modified expecting to get a real name, and then when it fell through to the final fallback that was wrong?

[wusikijeronii]

If the name ends up as empty or otherwise invalid then the search will fail and we'll fall through and use the original func. From looking at the change it seems like the underlying problem was that func got modified expecting to get a real name, and then when it fell through to the final fallback that was wrong?

Yes, that's exactly what was happening.
func was being overwritten during ordinal-to-name translation. When the ordinal name was missing or empty, we ended up propagating that invalid value instead of the original func, so the final fallback was using the wrong input.

Updated the implementation as suggested so that:

• func remains unchanged,
• a local searchFunc is used for translation and lookup,
• If translation fails, we fall back using the original func.

[baldurk]

Thanks for making those changes. What I was hoping for was to reduce the set of changes though - originalRequest / requestIsOrdinal I don't think is needed and setting searchFunc to NULL then handling that seems unneeded when an empty string would fail the search naturally.

Unless I've misunderstood then all that is needed is to avoid the func trample and everything else would work as-is. Could you do have a new local for func and add no extra processing or changes?

[baldurk]

nitpick: can you make this const LPCSTR func.

[wusikijeronii]

Done. Updated Hooked_GetProcAddress to keep func unchanged and introduced a local searchFunc, without adding any extra processing or changing the existing fallback behaviour.