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.

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

[release/6.0][EventPipe] Fix reverse connection socket leaking to child processes. #65768

Merged
merged 2 commits into from
Mar 8, 2022

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Feb 23, 2022

Customer Impact

Clients requesting information through EventPipe could see a connection being active for an indeterminate even though the target runtime has closed it if the process forked in the time window from the session started to the time it gets finished. This was reported by customers through VS 4 mac.

Description

Sockets were getting leaked upon accepting connections. This meant that child processes could cause fully terminated tracing sessions and other IPC connections alive, causing clients to wait for input indefinitely. This sets the CLOEXEC bits on the socket atomically upon accepting if the OS provides the capability, falling back to a best effort fcntl on systems like macOS and x866 Android emulators.

Port of #65365 to release/6.0.

Regression

No

Testing

Validated customer scenario on top of testing.

Risk

Low. The change mitigates risk by preventing sockets getting inherited and ensuring the state is clean. When possible it does it as an atomic kernel operation. Should fall back to prior behavior if close on exec is not available.

…ld processes.

Sockets were getting leaked upon accepting connections. This meant that child processes could cause
fully terminated tracing sessions and other IPC connections alive, causing clients to wait for input
indefinitely. This sets the CLOEXEC bits on the socket atomically upon accepting if the OS provides
the capability, falling back to a best effort fcntl on systems like macOS and x866 Android emulators.

Port of dotnet#65365 to release/6.0.
@hoyosjs hoyosjs requested review from lateralusX, wfurt and a team February 23, 2022 10:13
@ghost ghost assigned hoyosjs Feb 23, 2022
@marek-safar marek-safar added this to the 6.0.x milestone Feb 23, 2022
@marek-safar marek-safar added the Servicing-consider Issue for next servicing release review label Feb 23, 2022
@wfurt
Copy link
Member

wfurt commented Feb 23, 2022

 -- Looking for accept4
  -- Looking for accept4 - not found
  CMake Error: File /eventpipe/ep-shared-config.h.in does not exist.
  CMake Error at /Users/runner/work/1/s/src/native/eventpipe/configure.cmake:12 (configure_file):
    configure_file Problem configuring file
  Call Stack (most recent call first):
    mono/eventpipe/CMakeLists.txt:3 (include)
    mono/component/CMakeLists.txt:56 (include)
    mono/mini/CMakeLists.txt:52 (include)

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

Source change LGTM (assuming that whatever the build issue is gets resolved)

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. We should take for consideration in 6.0.x

@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 24, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.4 Feb 24, 2022
@carlossanlop
Copy link
Member

Branch is now open for Tactics-approved changes. The PR is signed off, has the servicing-approved label applied, and the CI passed. The mono System.Text.Json.Tests failure is known and unrelated.

@carlossanlop carlossanlop merged commit 154cbb0 into dotnet:release/6.0 Mar 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2022
@hoyosjs hoyosjs deleted the juhoyosa/net6-cloexec-ep branch September 19, 2022 21:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tracing-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants