I have a real old code from a service that uses named named pipes in message mode (PIPE_TYPE_MESSAGE) with overlapped i/o (FILE_FLAG_OVERLAPPED).

The code does the following:

1. ReadFile for 4 bytes with overlapped i/o (header message length). The Client wrote this command with one call to WriteFile.
2. After the 4 bytes are read, a ReadFile call is made to read the rest of the message (with the known length) without specifying an OVERLAPPED structure.
3. After the command is executed the routine continues at stage 1. and awaits the next command.

When I read the documentation

Overlapped operations require a file, named pipe, or communications device that was created with the FILE_FLAG_OVERLAPPED flag. When a thread calls a function (such as the ReadFile function) to perform an overlapped operation, the calling thread must specify a pointer to an OVERLAPPED structure. (If this pointer is NULL, the function return value may incorrectly indicate that the operation completed.)

I have to assume that this code will not work or is at least be called incorrect...

In fact this code is 15 years old and runs on hundreds of machines and it works without problems.

So do I have to tell my boss and colleagues that this code is buggy and it is just luck that it works and this code need to be corrected?

CodePudding user response：

After reading more docs, I have to conclude that yes, the code can be said to be incorrect. Particularly these docs on ReadFile:

A pointer to an OVERLAPPED structure is required if the hFile parameter was opened with FILE_FLAG_OVERLAPPED, otherwise it can be NULL.

If hFile is opened with FILE_FLAG_OVERLAPPED, the lpOverlapped parameter must point to a valid and unique OVERLAPPED structure, otherwise the function can incorrectly report that the read operation is complete.

Bear in mind that the MS docs are updated over time, and the docs may have been less clear or incomplete at the time the code was written.

It probably happens to work because the pipe is in message mode. It's strange to have a length prefix header with message mode pipes in the first place, because the message mode handles message boundaries for you. In this specific scenario, the entire message would already be in the local OS so the incorrect synchronous read would always succeed.

Indeed, the protocol (with a separate header) sounds like it was designed to work with a byte stream abstraction such as a byte mode pipe or TCP/IP connection. It is possible that the protocol was originally designed for byte mode pipes and they switched to message mode pipes when it wasn't working (the synchronous ReadFile may behave unexpectedly on a byte mode pipe since the message may not be completely present yet).

CodePudding user response：

yes, this code is incorrect but can work without errors. ReadFile call ZwReadFile. it 5th parameter - IoStatusBlock - Pointer to an IO_STATUS_BLOCK structure - is mandatory and always must be not 0. for any file. for any I/O type. so ReadFile must pass pointer to IO_STATUS_BLOCK when it call ZwReadFile. if you pass not 0 pointer to OVERLAPPED - it pass this pointer as IO_STATUS_BLOCK to ZwReadFile. the first 2 members of OVERLAPPED corresponded to IO_STATUS_BLOCK. but if you pass 0 in place pointer of OVERLAPPED - ReadFile allocate local IO_STATUS_BLOCK iosb variable and pass it to ZwReadFile. the IO_STATUS_BLOCK memory must be valid until I/O not completed. because at the end of I/O kernel write final status to this memory. from another side - if you local variable for IO_STATUS_BLOCK - it became not valid (point to arbitrary memory in stack) after ReadFile return. in case synchronous I/O - no problem here, because ReadFile not return until I/O not completed. but in case asynchronous I/O - this was UB - ReadFile can return while I/O still in progress. so IO_STATUS_BLOCK became not valid before I/O end. and when I/O actually ended - will be overwritten memory at arbitrary place in your thread stack. this can have no any effect or can corrupt your stack. undefinded. depend from where you be (which stack pointer value) at this time