Fix win_write to check if fully sent #28432

qbx2 · 2021-12-26T12:07:47Z

win_write does not check if the data is completely sent in synchronous phase. When that situation happens, the connection hangs up. I added handling code for that.

@donnadionne

linux-foundation-easycla · 2021-12-26T12:07:51Z

The committers are authorized under a signed CLA.

✅ Sunyeop Lee (05b794b)

qbx2 · 2022-01-10T17:51:36Z

@donnadionne

qbx2 · 2022-02-02T13:48:45Z

Please review.

stale · 2022-09-21T03:00:16Z

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 30 days. It will be closed automatically if no further update occurs in 7 day. Thank you for your contributions!

veblush · 2022-10-11T19:38:46Z

We might still want to have this fix. AJ, would you mind reviewing this since you recently worked on Windows?

Schramp · 2022-10-21T20:58:23Z

Fixes issue #28524

Schramp · 2022-11-13T19:57:33Z

I still want this. Any way to upvote this?

drfloob

Thanks for this, sorry it slipped through the cracks (significantly). It looks correct to me, but I'm not sure how our tests never caught it. In particular, we have an end2end test that sends a 10MB payload (//test/core/end2end:h2_full_test@invoke_large_request). Are the windows transport buffers > 10MB by default? Best I could find, the default is O(kilobytes). If we can add a test for this, or modify one to catch it, that would be ideal.

src/core/lib/iomgr/tcp_windows.cc

qbx2 · 2022-11-18T00:25:25Z

Actually, the issue does not cause on windows environment because windows does not perform partial send (I didn’t know this at the time of writing code). It is only caused when we run grpc server on WINE. Even though, I think it’d be best to fix it because the behavior is not documented by microsoft. Therefore, we could use WINE to catch it. Do you think it is ok to add WINE tests to catch it?

Schramp · 2022-11-18T09:14:41Z

It is only caused when we run grpc server on WINE.
Same way I found it.

Schramp · 2022-11-18T09:22:58Z

src/core/lib/iomgr/tcp_windows.cc

-                   &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);


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.

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?

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.

drfloob · 2022-11-18T17:52:41Z

@qbx2, regarding:

Actually, the issue does not cause on windows environment because windows does not perform partial send

Do you have a source for that information? I'm not sure it's true, if the implementations are built to WSASend's current spec:

For non-overlapped sockets, ... If the socket is non-blocking and stream-oriented, and there is not sufficient space in the transport's buffer, WSASend will return with only part of the application's buffers having been consumed.

drfloob · 2022-11-18T23:36:46Z

@Schramp @qbx2 Given that our concern about use-after-free can be considered separately, I'm fine to merge this PR as is. @veblush can you take a second look as well, please?

qbx2 · 2022-11-20T16:07:30Z

@drfloob The source is Jinoh Kang from: https://bugs.winehq.org/show_bug.cgi?id=10648 . Also, he said that windows implementation sometimes does not follow the specification (documentation)

* [EventEngine] WindowsEndpoint Initial sketch, all tests passing * Port fix from #28432 * GPR_WINDOWS guard * use MemoryAllocator::MakeReservation for allocated buffers * better logging (respect slice length) * Automated change: Fix sanity tests * improvements * Automated change: Fix sanity tests * InlinedVector<WSABUF, kMaxWSABUFCount> * initial attempt at socket util reunification * posix fixes + local run of sanitize.sh * posix socket includes * fix * Automated change: Fix sanity tests * remove unused include (breaks windows) * remove stale comment Co-authored-by: drfloob <drfloob@users.noreply.github.com>

* Fix win_write to check if fully sent * Update tcp_windows.cc * Update tcp_windows.cc

* [EventEngine] WindowsEndpoint Initial sketch, all tests passing * Port fix from #28432 * GPR_WINDOWS guard * use MemoryAllocator::MakeReservation for allocated buffers * better logging (respect slice length) * Automated change: Fix sanity tests * improvements * Automated change: Fix sanity tests * InlinedVector<WSABUF, kMaxWSABUFCount> * initial attempt at socket util reunification * posix fixes + local run of sanitize.sh * posix socket includes * fix * Automated change: Fix sanity tests * remove unused include (breaks windows) * remove stale comment Co-authored-by: drfloob <drfloob@users.noreply.github.com>

Fix win_write to check if fully sent

05b794b

github-actions bot added the lang/core label Dec 26, 2021

Update tcp_windows.cc

f1df9cc

yashykt assigned nicolasnoble May 17, 2022

nicolasnoble mentioned this pull request May 17, 2022

Handle partial transmits in WSASend (Compatibility with WINE). #28506

Closed

nicolasnoble removed their assignment Jun 15, 2022

grpc-bot assigned veblush Jun 15, 2022

stale bot added the disposition/stale label Sep 21, 2022

veblush added platform/Windows kind/bug and removed disposition/stale labels Oct 11, 2022

veblush requested a review from drfloob October 11, 2022 19:37

veblush assigned drfloob and unassigned veblush Oct 11, 2022

Merge branch 'master' into fix-win-write

75a2eda

drfloob reviewed Nov 17, 2022

View reviewed changes

src/core/lib/iomgr/tcp_windows.cc Outdated Show resolved Hide resolved

drfloob added the kokoro:force-run label Nov 17, 2022

grpc-kokoro removed the kokoro:force-run label Nov 17, 2022

drfloob added the release notes: no Indicates if PR should not be in release notes label Nov 17, 2022

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels Nov 17, 2022

Update tcp_windows.cc

7cd6d8d

Schramp approved these changes Nov 18, 2022

View reviewed changes

Schramp reviewed Nov 18, 2022

View reviewed changes

Schramp approved these changes Nov 18, 2022

View reviewed changes

drfloob added the kokoro:force-run label Nov 18, 2022

grpc-kokoro removed the kokoro:force-run label Nov 18, 2022

drfloob approved these changes Nov 18, 2022

View reviewed changes

drfloob requested a review from veblush November 18, 2022 23:37

veblush approved these changes Nov 19, 2022

View reviewed changes

drfloob merged commit 756fdde into grpc:master Nov 19, 2022

Schramp mentioned this pull request Nov 21, 2022

Handle partial transmits in WSASend (Compatibility with WINE). #28524

Closed

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Nov 21, 2022

drfloob added a commit to drfloob/grpc that referenced this pull request Nov 22, 2022

Port fix from grpc#28432

c8da1ce

jskeet mentioned this pull request Jan 16, 2023

Google.Cloud.PubSub.V1 does not work on Wine googleapis/google-cloud-dotnet#9561

Closed

wanlin31 pushed a commit that referenced this pull request May 18, 2023

Fix win_write to check if fully sent (#28432)

1355de7

* Fix win_write to check if fully sent * Update tcp_windows.cc * Update tcp_windows.cc

Schramp mentioned this pull request Jun 21, 2023

[BUG] WSAEFAULT #27292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix win_write to check if fully sent #28432

Fix win_write to check if fully sent #28432

qbx2 commented Dec 26, 2021

linux-foundation-easycla bot commented Dec 26, 2021 •

edited

qbx2 commented Jan 10, 2022

qbx2 commented Feb 2, 2022

stale bot commented Sep 21, 2022

veblush commented Oct 11, 2022

Schramp commented Oct 21, 2022

Schramp commented Nov 13, 2022

drfloob left a comment

qbx2 commented Nov 18, 2022

Schramp commented Nov 18, 2022

Schramp Nov 18, 2022

drfloob Nov 18, 2022

Schramp Nov 20, 2022

drfloob commented Nov 18, 2022

drfloob commented Nov 18, 2022

qbx2 commented Nov 20, 2022 •

edited

Fix win_write to check if fully sent #28432

Fix win_write to check if fully sent #28432

Conversation

qbx2 commented Dec 26, 2021

linux-foundation-easycla bot commented Dec 26, 2021 • edited

qbx2 commented Jan 10, 2022

qbx2 commented Feb 2, 2022

stale bot commented Sep 21, 2022

veblush commented Oct 11, 2022

Schramp commented Oct 21, 2022

Schramp commented Nov 13, 2022

drfloob left a comment

Choose a reason for hiding this comment

qbx2 commented Nov 18, 2022

Schramp commented Nov 18, 2022

Schramp Nov 18, 2022

Choose a reason for hiding this comment

drfloob Nov 18, 2022

Choose a reason for hiding this comment

Schramp Nov 20, 2022

Choose a reason for hiding this comment

drfloob commented Nov 18, 2022

drfloob commented Nov 18, 2022

qbx2 commented Nov 20, 2022 • edited

linux-foundation-easycla bot commented Dec 26, 2021 •

edited

qbx2 commented Nov 20, 2022 •

edited