2

The following code is copied from MS Async Filter. It is supposed that the following code is calling either CancelIo or CancelIoEx. I don't see where CancelIoEx is called in anyway. It is supposed that the typedef is representing the CancelIoEx but is never called. What exactly the line bResult = (pfnCancelIoEx)(m_hFile, NULL); is doing?

// Cancel: Cancels pending I/O requests.
HRESULT CFileStream::Cancel()
{
    CAutoLock lock(&m_CritSec);

    // Use CancelIoEx if available, otherwise use CancelIo.

    typedef BOOL (*CANCELIOEXPROC)(HANDLE hFile, LPOVERLAPPED lpOverlapped);

    BOOL bResult = 0;
    CANCELIOEXPROC pfnCancelIoEx = NULL;


    HMODULE hKernel32 = LoadLibrary(L"Kernel32.dll");

    if (hKernel32){

        //propably bad code !!! Take Care.
        bResult = (pfnCancelIoEx)(m_hFile, NULL);

        FreeLibrary(hKernel32);
    }
    else {

        bResult = CancelIo(m_hFile);
    }

    if (!bResult) {

        return HRESULT_FROM_WIN32(GetLastError());
    }
    return S_OK;
}
tshepang
  • 12,111
  • 21
  • 91
  • 136
Maverick
  • 1,105
  • 12
  • 41

1 Answers1

1

Assuming this is all the code, it has a serious bug in it. This:

CANCELIOEXPROC pfnCancelIoEx = NULL;

defines pfnCancelIoEx as a pointer to function whose signature matches that of CancelIoEx. The pointer is initialised to a null value and the obvious intention is to point it at CancelIoEx later.

That function is defined in Kernel32.dll, so loading this is the logical next step. If this succeeds, the code should proceed by doing this:

pfnCancelIoEx = GetProcAddress(hKernel32, "CancelIoEx");

And then it should check the result. However, it does not do either of that.

Next, on this line:

bResult = (pfnCancelIoEx)(m_hFile, NULL);

it attempts to call the function pointed to by pfnCancelIoEx. However, this pointer is never changed from its initial null value, so this will attempt to dereference a null pointer, resulting in undefined behaviour and likely a crash.

Angew is no longer proud of SO
  • 167,307
  • 17
  • 350
  • 455
  • Yes, I absolutely agree with you... but I think it would be easier to call directly CancelIoEx instead of doing a programming circle to accomplish the same thing :-) I dont see the point !!! you ? – Maverick Apr 19 '13 at 17:57
  • @Maverick You'd normally use such a trick if the function could be unavailable in certain situations (e.g. if it was only available on certain versions of Windows). Hoever, based on its documentaiton, `CancelIoEx` is available since Windows 95, so this doesn't seem to be the case here... No other reason I can think of. – Angew is no longer proud of SO Apr 19 '13 at 18:10
  • 1
    The function CancelIoEx is available since Vista and later Os (you can check at http://msdn.microsoft.com/en-us/library/windows/desktop/aa363792(v=vs.85).aspx), but in this case I don't think that just checking if it can load the Kernel32.dll is a sign that is a Vista OS or that specific function is implemented. Am i correct ? – Maverick Apr 19 '13 at 18:36
  • @Maverick Whoops, I had the wrong MSDN page open. Thanks for correcting the function availability. And the code is not actually calling `CancelIoEx` (even if it happens to be in the loaded `Kernel32`), it's dereferencing a null pointer. – Angew is no longer proud of SO Apr 19 '13 at 18:44
  • Yes I agree with you about pointer dereferecing and of course its wrong. The original code itself, is supposed to check the availability of the new CancelIoEx with the code "if(hKernel32)" and if it was true, it was an evident that CancelIoEx exists and is called (someway that is also wrong). Now, in the case that hKernel32 check, was failed, then is called the old CancelIo as you can see in the code. As I said, this code is wrong from the begging since you can not determine the availability or the existance of CancelIoEx just cheking if Kernel32.dll can load without problems. – Maverick Apr 20 '13 at 04:49