Crash at NktHook.Hook if LoadCustomDLL cannot find DLL

[bo3b]

This has killed me at least twice now, and I really am expecting LoadCustomDLL to return an error. I check and handle all errors, but LoadCustomDLL will report success, even if the dll path name is invalid.

In particular, if the pathname is invalid, LoadCustomDLL will return the int/hresult=0. And I'll crash later at NktHook.Hook.

If my pathname is valid, then LoadCustomDLL will return int/hresult=1, which is a little weird, but still matches the SUCCESS style macros. Still, it really ought to be 0, this is not a 'test for functionality' type call. The documentation does not indicate it would ever return '1'.

This is a fairly serious problem, because debugging this took me a couple of days. This crash because of a bad path is quite obscure, and would be dramatically better if LoadCustomDLL would return an error.

Here is the stack crawl when it crashes.

    >   00000001()  Unknown
    [Frames below may be incorrect and/or missing]  
    DvAgent.dll!TNktArrayList<CNktDvParam *,128,TNktArrayListItemRemove_Release<CNktDvParam *> >::RemoveAllElements() Line 330  C++
    DvAgent.dll!CNktDvHookEngine::Hook(CNktDvHookEngine::tagHOOKINFO * aHookInfo=0x8007007e, unsigned long nCount=1, int bIsInternal=0) Line 494    C++
    DvAgent.dll!CDvAgentMgr::OnEngMsg_AddHook(tagNKT_DV_TMSG_ADDHOOK * lpMsg=0xffe16054, CNktDvTransportBigData * lpConnBigData=0xffe300e0) Line 2522   C++
    DvAgent.dll!CDvAgentMgr::TAC_OnEngineMessage(CNktDvTransportAgent * lpTransport=0x77ad4060, tagNKT_DV_TMSG_COMMON * lpMsg=0xffe16054, unsigned long nMsgSize=1084, CNktDvTransportBigData * lpConnBigData=0xffe300e0) Line 699  C++
    DvAgent.dll!CNktDvTransportAgent::WorkerThreadProc(unsigned long nIndex=5) Line 564 C++
    DvAgent.dll!TNktClassWorkerThread<CNktDvTransportAgent>::ThreadProc() Line 169  C++
    DvAgent.dll!thread_start<unsigned int (__stdcall*)(void *)>(void * const parameter=0xffe0418c) Line 115 C++
    kernel32.dll!@BaseThreadInitThunk@12() Unknown
    ntdll.dll!___RtlUserThreadStart@8()    Unknown
    ntdll.dll!__RtlUserThreadStart@8() Unknown

Here is my simplified C# code that demonstrates the problem:

        _spyMgr = new NktSpyMgr();
        hresult = _spyMgr.Initialize();
        if (hresult != 0)
            throw new Exception("Deviare initialization error.");
#if DEBUG
        _spyMgr.SettingOverride("SpyMgrDebugLevelMask", 0xCF8);
#endif

        {
            // Launch the game, but suspended, so we can hook our first call and be certain to catch it.

            _gameProcess = _spyMgr.CreateProcess(game, true, out continueevent);
            if (_gameProcess == null)
                throw new Exception("Game launch failed.");

            // Load the NativePlugin for the C++ side.  The NativePlugin must be in this app folder.
            // The Agent supports the use of Deviare in the CustomDLL, but does not respond to hooks.

            _spyMgr.LoadAgent(_gameProcess);
            int result = _spyMgr.LoadCustomDll(_gameProcess, _nativeDLLName, true, true);  // *** trouble
            if (result < 0)
                throw new Exception("Could not load NativePlugin DLL.");

            // Hook the primary DX9 creation call of Direct3DCreate9, which is a direct export of 
            // the d3d9 DLL.  All DX9 games must call this interface, or the Direct3DCreate9Ex.
            // We set this to flgOnlyPreCall, because we want to always create the IDirect3D9Ex object.

            NktHook d3dHook = _spyMgr.CreateHook("D3D9.DLL!Direct3DCreate9", (int)eNktHookFlags.flgOnlyPreCall);
            if (d3dHook == null)
                throw new Exception("Failed to hook D3D9.DLL!Direct3DCreate9");

            // Make sure the CustomHandler in the NativePlugin at OnFunctionCall gets called when this 
            // object is created. At that point, the native code will take over.

            d3dHook.AddCustomHandler(_nativeDLLName, 0, "");

            // Finally attach and activate the hook in the still suspended game process.

            d3dHook.Attach(_gameProcess, true);
            d3dHook.Hook(true);  // *** Will crash here.

            // Ready to go.  Let the game startup.  When it calls Direct3DCreate9, we'll be
            // called in the NativePlugin::OnFunctionCall

            _spyMgr.ResumeProcess(_gameProcess, continueevent);
        }

[mxmauro]

Will check. Thanks for the report.

[bo3b]

If it helps to have an easy test case, the sample I wrote a couple of months ago demonstrates this same problem. You can alter the DLL path and it will generate the same crash.

Hooking-DX9-Present

[mxmauro]

Hi @bo3b, after checking the code remembered that errors are not raised by COM calls because many programmers don't use try/catch blocks. The exception are fatal errors like E_OUTOFMEMORY

DeviareCOM exports an api named GetLastErrorCode that returns an HRESULT. Use it after the call to LoadCustomDll get the real error code.

Sorry for the confusion.

[bo3b]

Hi @mxmauro, thanks for taking a look.

It still seems wrong to me that it returns a 0=noerr if the file is missing. If the functionality is such that these are inverted or something, then it definitely should not return an HRESULT, because that has very clear fail/nofail. Changing the way HRESULT is interpreted would be very bad and confusing.

It seems like this is internally returning a bool, but is defined with an HRESULT. Those are incompatible.

In other words if 0=fail, then the API is misleading. And it is not presently documented that 0=fail, so it would not occur to anyone to check the LastError.

BTW, I'm not suggesting for it to throw exceptions. It just seems like that call is not upholding it's API contract as it presently defined.

[mxmauro]

The problem is any HRESULT failure code is converted to an exception in .NET. In the design of the wrapper, we decided to only return fatal errors as such, else S_OK and use GetLastErrorCode to ensure the call really worked.

I know it is anti-intuitive for COM but for the beginning of .NET programming, when people was no accustomed to exceptions, was a good workaround.

[bo3b]

The problem is any HRESULT failure code is converted to an exception in .NET. In the design of the wrapper, we decided to only return fatal errors as such, else S_OK and use GetLastErrorCode to ensure the call really worked.

I know it is anti-intuitive for COM but for the beginning of .NET programming, when people was no accustomed to exceptions, was a good workaround.

Ummm... OK, I can change to use that model, but GetLastErrorCode is not in the v2 documentation, and none of your samples use it. I've been through every sample and all of your web pages, and this is the first I've heard of this approach. Always possible I've missed something. Would love to see what the old forums said. < wink>< wink >

In any case, using your COM exception model, wouldn't it still make more sense for the LoadCustomDLL call to call GetLastErrorCode and return what it returns?

I don't think any user of a function that returns an HRESULT would expect to need to do this for any scenario except for an error return. Changing a fundamental API contract like this is bound to cause confusion. In every other code I use that returns HRESULT, S_OK means OK, it worked, not "check if something terrible happened." The standard macros SUCCESS and FAILED do not work using your model.

If you really believe that it is better to put the burden on the API caller, than I would recommend changing the API result from HRESULT because it is misleading.

[mxmauro]

Although COM can be used in many languages, Deviare is used primary in C++/C#. For C++, COM is not needed because the underlaying engine is already in that lang.

For .NET, if LoadCustomDll returns an error, will raise the exception. Any non S_OK raises an exception.

We have to update samples;

[DllImport("DeviareCOM.dll")]
static extern int GetLastErrorCode();

[bo3b]

For .NET, if LoadCustomDll returns an error, will raise the exception. Any non S_OK raises an exception.

Always possible that I am confused, but in my GitHub sample program, I am calling LoadCustomDLL from C#, which if I'm not mistaken would be .NET. When the DLL specified is missing, it does not raise an exception, it simply returns S_OK, as 0.

[mxmauro]

Yes because the "not found" error is not considered fatal but if you call GetLastErrorCode api it is very likely you will get 0x80070002 or 0x80070003 because of the wrong path/filename.

[bo3b]

Sorry for the delay in getting back to this.

When I add the GetLastErrorCode, I do get a 0x8007007e error code. "The specified module could not be found. "

This is functional, but I would still encourage you to improve the API. I'm providing you feedback as a client of your API, and for me it is unnecessarily confusing. Please reconsider this approach.

If LoadCustomDll does not legitimately return an HRESULT, then it should be defined as returning void, with the expectation that callers will call GetLastErrorCode in every case.

Having it return 0 or 1 makes no sense.

Also, does this mean I should be calling GetLastErrorCode for every Deviare function? Are they all setup this way?

It seems to me that the function SetupErrorInfoAndReturn is being called wrong from the LoadCustomDLL, or perhaps the defaults are wrong.

Right now it's set with bForceFatal=False, and having it set True would make it work the way I expect, and the way the API is defined- by returning an HRESULT.

The hErrorCodeRes is being unnecessarily masked.