Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix win_write to check if fully sent #28432

Merged
merged 4 commits into from Nov 19, 2022
Merged

Fix win_write to check if fully sent #28432

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
05b794b
Fix win_write to check if fully sent
qbx2 Dec 26, 2021
f1df9cc
Update tcp_windows.cc
qbx2 Dec 27, 2021
75a2eda
Merge branch 'master' into fix-win-write
qbx2 Oct 12, 2022
7cd6d8d
Update tcp_windows.cc
qbx2 Nov 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 37 additions & 15 deletions src/core/lib/iomgr/tcp_windows.cc
View file
Open in desktop
Expand Up @@ -330,7 +330,7 @@ static void on_write(void* tcpp, grpc_error_handle error) {
if (info->wsa_error != 0) {
error = GRPC_WSA_ERROR(info->wsa_error, "WSASend");
} else {
GPR_ASSERT(info->bytes_transferred == tcp->write_slices->length);
GPR_ASSERT(info->bytes_transferred <= tcp->write_slices->length);
}
}

Expand All @@ -350,7 +350,7 @@ static void win_write(grpc_endpoint* ep, grpc_slice_buffer* slices,
WSABUF local_buffers[MAX_WSABUF_COUNT];
WSABUF* allocated = NULL;
WSABUF* buffers = local_buffers;
size_t len;
size_t len, async_buffers_offset = 0;

if (grpc_tcp_trace.enabled()) {
size_t i;
Expand Down Expand Up @@ -391,27 +391,49 @@ static void win_write(grpc_endpoint* ep, grpc_slice_buffer* slices,
/* First, let's try a synchronous, non-blocking write. */
status = WSASend(socket->socket, buffers, (DWORD)tcp->write_slices->count,
&bytes_sent, 0, NULL, NULL);
info->wsa_error = status == 0 ? 0 : WSAGetLastError();

/* We would kind of expect to get a WSAEWOULDBLOCK here, especially on a busy
connection that has its send queue filled up. But if we don't, then we can
avoid doing an async write operation at all. */
if (info->wsa_error != WSAEWOULDBLOCK) {
grpc_error_handle error = status == 0
? absl::OkStatus()
: GRPC_WSA_ERROR(info->wsa_error, "WSASend");
grpc_core::ExecCtx::Run(DEBUG_LOCATION, cb, error);
if (allocated) gpr_free(allocated);
return;
if (status == 0) {
if (bytes_sent == tcp->write_slices->length) {
info->wsa_error = 0;
grpc_error_handle error = absl::OkStatus();
grpc_core::ExecCtx::Run(DEBUG_LOCATION, cb, error);
if (allocated) gpr_free(allocated);
return;
}

/* The data was not completely delivered, we should send the rest of
them by doing an async write operation. */
for (i = 0; i < tcp->write_slices->count; i++) {
if (buffers[i].len > bytes_sent) {
buffers[i].buf += bytes_sent;
buffers[i].len -= bytes_sent;
break;
}
bytes_sent -= buffers[i].len;
async_buffers_offset++;
}
} else {
info->wsa_error = WSAGetLastError();

/* We would kind of expect to get a WSAEWOULDBLOCK here, especially on a
busy connection that has its send queue filled up. But if we don't, then
we can avoid doing an async write operation at all. */
if (info->wsa_error != WSAEWOULDBLOCK) {
grpc_error_handle error = GRPC_WSA_ERROR(info->wsa_error, "WSASend");
grpc_core::ExecCtx::Run(DEBUG_LOCATION, cb, error);
if (allocated) gpr_free(allocated);
return;
}
}

TCP_REF(tcp, "write");

/* If we got a WSAEWOULDBLOCK earlier, then we need to re-do the same
operation, this time asynchronously. */
memset(&socket->write_info.overlapped, 0, sizeof(OVERLAPPED));
status = WSASend(socket->socket, buffers, (DWORD)tcp->write_slices->count,
&bytes_sent, 0, &socket->write_info.overlapped, NULL);
status = WSASend(socket->socket, buffers + async_buffers_offset,
(DWORD)(tcp->write_slices->count - async_buffers_offset),
NULL, 0, &socket->write_info.overlapped, NULL);
if (allocated) gpr_free(allocated);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that the blocking behaviour of the first WSASend function on Windows hides the use-after free of the variable "allocated" (or the stack allocated version "local_buffers" that gets out of scope) of the non blocking second WSASend. In Windows this code is never reached.

This is however another issue that is out of scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that's right. Per the WSASend docs:

If this function is completed in an overlapped manner, it is the Winsock service provider's responsibility to capture the WSABUF structures before returning from this call. This enables applications to build stack-based WSABUF arrays pointed to by the lpBuffers parameter.

Deallocating the heap (and local) variables before returning shouldn't be an issue for the subsequent overlapped WSASend call. Do I maybe misunderstand your concern here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, the copying of the buffer in wsasend is new to me. I more or less assumed it was up to the completion handler.

My comments on use after free can be ignored.


if (status != 0) {
Expand Down