Bug in MFC ISAPI Support (VC++ 4.1 and 5.0)

There is a bug in the MFC code supporting ISAPI server extensions in Visual C++ 4.1 and 5.0 that can lead to instability in the Web server on which the compiled ISAPI extension is loaded. It affects server extensions accessed via the POST method.

Visual C++ 4.1

There are two member functions in CHttpServer, which is implemented in isapi.cpp, which have bad code. CHttpServer::GetQuery has the following problems

It uses _tcscpy to copy data from the ECB lpbData member, but lpbData is not necessarily nul-terminated. It is a sequence of bytes whose length is given by the cbAvailable member. Since the buffer being copied into is cbAvailable + 1 bytes allocated on the heap, it is possible that the copy overrun in the _tcscpy call will corrupt pointers in the heap.
It assumes the CHttpServerContext::ReadClient member function will return as many bytes as requested. ReadClient behaves like any blocking socket read: it will wait until some data is available and return it, even if there are fewer bytes available than requested.
It fails to nul-terminate lpszQuery after the ReadClient call. The function's caller expects a nul-terminated string.

CHttpServer::HttpExtensionProc has the following problem

It allocates a cbAvailable-sized buffer for the GetQuery call, but it should be cbTotalBytes.

Visual C++ 5.0

The bugs in 4.x were clearly noticed and mostly cleaned up. Unfortunately, CHttpServer::GetQuery still fails to nul-terminate lpszQuery after it calls ReadClient.

Further Note on ReadClient

CHttpServerContext::ReadClient is implemented as an inline function which simply delegates to the ECB ReadClient member function pointer. According to ISAPI documentation, this should only need to be called when cbTotalBytes is greater than 48k, i.e. lpbData will point to the first 48k of data received. Thus, the bug that remains in VC++ 5.0 should only appear when very large strings are associated with a POST.

Workarounds

Unfortunately, because of the way the ISAPI code is implemented, there is no clearly 100% clean way in which to fix the problems. The HttpExtensionProc member function is virtual, but it cannot be replaced wholesale because it depends on variables declared static in isapi.cpp. It is tempting to override it and ensure that all data has been read into the ECB's lpbData buffer before calling up to CHttpServer::HttpExtensionProc. This may work, but it is not clear that it is safe to free() the current buffer and malloc() a new one in its place. The GetQuery member function is not virtual, so even if you override it in a descendant of CHttpServer, the original HttpExtensionProc member function will not call it.