Pass WSA_FLAG_NO_HANDLE_INHERIT flag to the WSASocketA() to avoid handle leaking on the Windows in case of using CreateProcess() on the server

[frazenshtein]

https://docs.microsoft.com/en-us/windows/desktop/api/winsock2/nf-winsock2-wsasocketa

[thelinuxfoundation]

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

• User @frazenshtein isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
CLA GitHub bot

[yang-g]

Please sign cla.

[jtattermusch]

@frazenshtein is this PR an attempt to resolve #17704?
Have you been able to confirm that it fixes the problem?

[jtattermusch]

@nicolasnoble opinions?

[apolcyn]

Seeing this error in the linux artifact build:

[CXX]     Compiling src/core/lib/gpr/wrap_memcpy.cc
[CXX]     Compiling src/core/lib/gprpp/fork.cc
[CXX]     Compiling src/core/lib/gprpp/thd_posix.cc
[CXX]     Compiling src/core/lib/gprpp/thd_windows.cc
[CXX]     Compiling src/core/lib/profiling/basic_timers.cc
[AR]      Creating /tmp/libs/opt/libaddress_sorting.a
[CXX]     Compiling src/core/lib/profiling/stap_timers.cc
[AR]      Creating /tmp/libs/opt/libz.a
[AR]      Creating /tmp/libs/opt/libares.a
src/core/lib/iomgr/socket_windows.cc: In function 'void grpc_wsa_socket_flags_init()':
src/core/lib/iomgr/socket_windows.cc:193:77: error: passing NULL to non-pointer argument 5 of 'SOCKET WSASocketW(int, int, int, LPWSAPROTOCOL_INFOW, GROUP, DWORD)' [-Werror=conversion-null]
                           grpc_wsa_socket_flags | WSA_FLAG_NO_HANDLE_INHERIT);

[apolcyn]

is the header that contains the new socket option also available to mingw? That build failure is a mingw issue

[apolcyn]

I'm also wondering has this been verified to fix the issue. A test would be best but barring that it would be good to know that this has been manually verified

[jtattermusch]

@frazenshtein, are you planning to address review feedback?

[frazenshtein]

@frazenshtein, are you planning to address review feedback?

Yes, I will try to take into account the comments in the nearest future. I have some difficulties with the allocation of free time.

[frazenshtein]

Seeing this error in the linux artifact build:

Yes, I really mixed up NULL and 0. Fixed.

is the header that contains the new socket option also available to mingw? That build failure is a mingw issue

Haven't found this one. Looks strange, because we have added #ifndef to the src/core/lib/iomgr/socket_windows.h Can you provide an error message or a link and keywords to grep?

let's turn this into a function rather than have everyone reference the global directly

Fixed. However, I'm happy to rename it to your liking. I didn't find a good example in the code, so it turned out a bit long.

I'm also wondering has this been verified to fix the issue. A test would be best but barring that it would be good to know that this has been manually verified

I was unable to reproduce the problem from the #17704 issue. If the author provides a minimal example it will be easier.
UPD: I have used cpp instead of c#, btw

[jtattermusch]

I triggered an adhoc package build (the nugets) to be able to verify if this patch fixes #17704
https://sponge.corp.google.com/target?id=1c772a91-2b04-42ce-8d55-31a2a2990574&target=grpc/core/experimental/grpc_build_artifacts_multiplatform
I'll later post the link to the pre-built nugets.

[jtattermusch]

The adhoc build of packages is here:
https://storage.googleapis.com/grpc-testing-kokoro-prod/test_result_public/prod/grpc/core/master/windows/grpc_build_packages/5341/20190314-085942/github/grpc/artifacts/csharp_nugets_windows_dotnetcli.zip

[jtattermusch]

According to #17704 (comment) this would fix #17704 if merged.

[jtattermusch]

Do we want to use the default flags for resolver_component_test.cc too?

grpc/test/cpp/naming/resolver_component_test.cc

Line 278 in 934e31d

SOCKET s = WSASocket(AF_INET6, SOCK_STREAM, IPPROTO_TCP, nullptr, 0,

[jtattermusch]

LGTM for general sanity and we also have a confirmation that this fixes #17704.

@apolcyn besides what I mentioned in #18060 (comment), I'm mostly happy with the code in this PR, do you want to give it a quick pass and merge if it looks good?

[frazenshtein]

Could you please make clear for me is something expected from my side?

[jtattermusch]

I believe we need need to use grpc_get_default_wsa_socket_flags() here:

grpc/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_windows.cc

Line 448 in 69b2d57

SOCKET s = WSASocket(af, type, protocol, nullptr, 0, WSA_FLAG_OVERLAPPED);

and here

grpc/test/cpp/naming/resolver_component_test.cc

Line 279 in ad4f805

WSA_FLAG_OVERLAPPED);

Update: ignore my comment.

[jtattermusch]

@apolcyn I believe this is good to do, can you give owners approval so we can merge?

[apolcyn]

LGTM, sorry for the delay