Modify

Ticket #8722 (closed Bugs: fixed)

Opened 21 months ago

Last modified 10 months ago

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

Reported by: Dentcho Bankov <dbankov@…> Owned by: chris_kohlhoff
Milestone: To Be Determined Component: asio
Version: Boost Development Trunk Severity: Problem
Keywords: Cc: chris_kohlhoff

Description

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.

Attachments

BOOSTCA.7z Download (1.2 KB) - added by Dentcho Bankov <dbankov@…> 21 months ago.
Windows named-pipe message mode example illustrating the described problem
proposedFix.patch Download (1004 bytes) - added by Dentcho Bankov <dbankov@…> 20 months ago.
Proposed fix patch

Change History

Changed 21 months ago by Dentcho Bankov <dbankov@…>

Windows named-pipe message mode example illustrating the described problem

Changed 20 months ago by Dentcho Bankov <dbankov@…>

Proposed fix patch

comment:1 Changed 20 months ago by 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;
  }
}

comment:2 Changed 17 months ago by chris_kohlhoff

  • Status changed from new to closed
  • Resolution set to fixed

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

comment:3 Changed 10 months ago by aoney@…

  • Status changed from closed to reopened
  • Resolution fixed deleted

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;

}

comment:4 follow-up: ↓ 6 Changed 10 months ago by chris_kohlhoff

  • Status changed from reopened to closed
  • Resolution set to 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.

comment:5 Changed 10 months ago by chris_kohlhoff

Opened #10034.

comment:6 in reply to: ↑ 4 Changed 10 months ago by 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.

View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.