The boost::asio::windows::stream_handle::read_some method may (incorrectly) throw when used with named-pipes in message mode (on Windows) and receives an ERROR_MORE_DATA error

[Dentcho Bankov <dbankov@…>]

Please note that this ticket is similar to Ticket#2936.

When using named-pipes in message mode on Windows it is possible that the read operation on the client side will fail with GetLastError() == ERROR_MORE_DATA. In the overlapped IO case the latter means that GetOverlappedResult(...) fails and reports this error.

Now looking at the win_iocp_handle_service::do_read(...) when in the above scenario if the GetOverlappedResult(...) fails do_read(...) would update the ec parameter with the error value (in this case ERROR_MORE_DATA) and then boost::asio::windows::stream_handle::read_some(...) would throw. The latter may be incorrect as the ReadFile(...) function may have actually read some data but the information about this data (bytes read) would be lost when the exception is thrown leaving the upper layers practically unable to recover from this situation even if they'd catch the exception... (see attached code)

It seems the fix for this should be to avoid throwing the exception by adding an "if" statement similar to the one after the ReadFile(...) call (several lines above in do_read(...)) e.g. making the code around GetOverlappedResult(...) look as follows:

Wait for the operation to complete. DWORD bytes_transferred = 0; ok = ::GetOverlappedResult(impl.handle_,

&overlapped, &bytes_transferred, TRUE);

if (!ok) {

DWORD last_error = ::GetLastError(); if (last_error != ERROR_MORE_DATA) <---- added "if" statement {

if (last_error == ERROR_HANDLE_EOF) {

ec = boost::asio::error::eof;

} else {

ec = boost::system::error_code(last_error,

boost::asio::error::get_system_category());

} return 0;

}

}

Please comment if the proposed fix is correct as I'd like to apply it before it is released in an official BOOST release.

[Dentcho Bankov <dbankov@…>]

Windows named-pipe message mode example illustrating the described problem

[Dentcho Bankov <dbankov@…>]

Proposed fix patch

[Dentcho Bankov <dbankov@…>]

Code formatting in my previous comment is awful. I'm sorry I should have previewed it.

Here is another attempt:

// Wait for the operation to complete.
DWORD bytes_transferred = 0;
ok = ::GetOverlappedResult(impl.handle_,
    &overlapped, &bytes_transferred, TRUE);
if (!ok)
{
  DWORD last_error = ::GetLastError();
  if (last_error != ERROR_MORE_DATA) // <--- the added "if" statement
  {
    if (last_error == ERROR_HANDLE_EOF)
    {
      ec = boost::asio::error::eof;
    }
    else
    {
      ec = boost::system::error_code(last_error,
           boost::asio::error::get_system_category());
    }
    return 0;
  }
}

[chris_kohlhoff]

Fixed on trunk in [85759]. Merged to release in [85838].

[aoney@…]

On the above note 'Fixed on trunk in [85759]. Merged to release in [85838].'

I think this may have been fixed incorrectly.

First note that the fix in the trunk is *not* the fix suggested by Dentcho above. The fix that went into trunk has a "return 0;" at the end of the "if (!ok)" clause (see below), not in the inner clause like above.

Thus, the change does not revert the logic back to how it looked in boost 1.47 (in boost 1.48, the ERROR_MORE_DATA check was removed, only to be added back later).

Now to the behavior - in message mode, ReadFile et. all will return partial data and ERROR_MORE_DATA to indicate additional parts of the message remain in the buffer. The boost method signature isn't rich enough to return this extra information (that the read splits a message rather than terminates it). However, boost *does* need to at least return the number of bytes read so far. And trunk's (and 1.55.0's) logic below, unlike Dentcho's logic above, clearly fails to do that because the return 0 is located differently:

if (!ok) {

DWORD last_error = ::GetLastError(); if (last_error != ERROR_MORE_DATA) {

if (last_error == ERROR_HANDLE_EOF) {

ec = boost::asio::error::eof;

} else {

ec = boost::system::error_code(last_error,

boost::asio::error::get_system_category());

}

} return 0;

}

[chris_kohlhoff]

Can you please clarify what you mean by:

Thus, the change does not revert the logic back to how it looked in boost 1.47 (in boost 1.48, the ERROR_MORE_DATA check was removed, only to be added back later).

as version control shows no diffs in win_iocp_handle_service.ipp between those versions.

In any case, I now think that the premise of this bug report was wrong. The read_some() method should throw if ReadFile fails with ERROR_MORE_DATA. If you don't want it to throw, you use the error_code overload. It just needs to also return the bytes_transferred value when it does.

This should be done as a new ticket as the behaviour is different from the one described in this bug report.

[chris_kohlhoff]

Opened #10034.

[aoney@…]

Replying to chris_kohlhoff:

Can you please clarify what you mean by:

Thus, the change does not revert the logic back to how it looked in boost 1.47 (in boost 1.48, the ERROR_MORE_DATA check was removed, only to be added back later).

as version control shows no diffs in win_iocp_handle_service.ipp between those versions.

Never mind on the 1.47 comment (I was looking at our fork of 1.47, not boost's).

In any case, I now think that the premise of this bug report was wrong. The read_some() method should throw if ReadFile fails with ERROR_MORE_DATA. If you don't want it to throw, you use the error_code overload. It just needs to also return the bytes_transferred value when it does.

This should be done as a new ticket as the behaviour is different from the one described in this bug report.

Exposing the 'torn message' case is indeed better than the alternative. That said, for us being able to treat a message pipe like a byte-stream pipe has proved valuable.

[Dentcho Bankov <dbankov@…>]

We hit this problem again now that we moved to BOOST 1.61. At the time I filed this ticket I suspected that the correct fix could be to provide the bytes_transmitted value in the exception but weren't happy with this because we'd have to handle a standard use case through an exception. Using the error_code overload seems to be a good enough way to avoid throwing an exception in a normal use case so I'd agree this is an acceptable solution. Still #10034 is not fixed so I'll try to provide a fix for it.