Boost C++ Libraries: Ticket #8722: 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

attachment set

Mon, 24 Jun 2013 12:30:23 GMT

attachment → BOOSTCA.7z

Windows named-pipe message mode example illustrating the described problem

attachment set

Mon, 24 Jun 2013 12:42:11 GMT

attachment → proposedFix.patch

Proposed fix patch

Mon, 24 Jun 2013 12:48:42 GMT

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;
  }
}

status changed; resolution set

chris_kohlhoff — Tue, 01 Oct 2013 08:26:22 GMT

status new → closed
resolution → fixed

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

status changed; resolution deleted

aoney@… — Fri, 09 May 2014 22:31:48 GMT

status closed → reopened
resolution fixed

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;

}

status changed; resolution set

chris_kohlhoff — Sat, 10 May 2014 09:14:39 GMT

status reopened → closed
resolution → fixed

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 — Sat, 10 May 2014 09:18:16 GMT

Opened #10034.

aoney@… — Fri, 16 May 2014 00:13:32 GMT

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.

Tue, 01 Aug 2017 15:44:04 GMT

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.