Skip to content

[EventPipe] Add dotnet_ipc_created mapping #123779

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

Open
mdh1418 wants to merge 2 commits into
base: main
Choose a base branch
from
+41 −1
Open

[EventPipe] Add dotnet_ipc_created mapping #123779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
897efbf
[EventPipe] Add dotnet_ipc_created mapping
mdh1418 Jan 29, 2026
d2850f2
Add mapping lifetime comment
mdh1418 Jan 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/native/eventpipe/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ check_include_file(
HAVE_SYS_IOCTL_H
)

check_include_file(
sys/syscall.h
HAVE_SYS_SYSCALL_H
)

check_include_file(
unistd.h
HAVE_UNISTD_H
Expand All @@ -43,6 +48,17 @@ check_include_file(
HAVE_ERRNO_H
)

check_include_file(
sys/mman.h
HAVE_SYS_MMAN_H
)

check_symbol_exists(
__NR_memfd_create
sys/syscall.h
HAVE_MEMFD_CREATE
)

if (NOT DEFINED EP_GENERATED_HEADER_PATH)
message(FATAL_ERROR "Required configuration EP_GENERATED_HEADER_PATH not set.")
endif (NOT DEFINED EP_GENERATED_HEADER_PATH)
Expand Down
24 changes: 23 additions & 1 deletion src/native/eventpipe/ds-ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@
#include "ep.h"
#include "ds-rt.h"

#if HAVE_SYS_MMAN_H && HAVE_MEMFD_CREATE && HAVE_UNISTD_H
#include <sys/mman.h>
#include <sys/syscall.h>
#include <unistd.h>
#ifndef MFD_CLOEXEC
#define MFD_CLOEXEC 0x0001U
#endif
#endif

/*
* Globals and volatile access functions.
*/
Expand Down Expand Up @@ -332,7 +341,20 @@ ds_ipc_stream_factory_configure (ds_ipc_error_callback_func callback)
default_port_builder.suspend_mode = port_suspend > 0 ? DS_PORT_SUSPEND_MODE_SUSPEND : DS_PORT_SUSPEND_MODE_NOSUSPEND;
default_port_builder.type = DS_PORT_TYPE_LISTEN;

result &= ipc_stream_factory_build_and_add_port (&default_port_builder, callback, true);
bool default_listen_port_ready = ipc_stream_factory_build_and_add_port (&default_port_builder, callback, true);
Copy link
Member

@lateralusX lateralusX Jan 30, 2026

Choose a reason for hiding this comment

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

Why only create this when using the default listener port? On some platforms we don't build with the default listener, but you can still setup diagnostic ports using the DOTNET_DiagnosticPorts env variable, so believe this should be done when we have any diagnostic port setup and started, not just the default.

#if HAVE_SYS_MMAN_H && HAVE_MEMFD_CREATE && HAVE_UNISTD_H
// Create a mapping to signal that the diagnostic server IPC is available.
// External tools can use this to detect when the runtime is ready to accept diagnostic commands.
// The mapping's lifetime is tied to the process, so tools that start after the .NET process can still detect it.
if (default_listen_port_ready) {
int fd = (int)syscall (__NR_memfd_create, "dotnet_ipc_created", (unsigned int)MFD_CLOEXEC);
if (fd >= 0) {
mmap (NULL, 1, PROT_EXEC | PROT_READ, MAP_PRIVATE, fd, 0);
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The return value of mmap should be checked for MAP_FAILED. The mmap call can fail for various reasons (e.g., insufficient memory, invalid permissions), and ignoring this failure could lead to resource leaks or unexpected behavior. Other instances of mmap in the codebase consistently check for MAP_FAILED (see src/native/libs/System.Native/pal_io.c:1010, src/native/corehost/hostmisc/pal.unix.cpp:99, and src/native/minipal/memorybarrierprocesswide.c:119).

Suggested change
mmap (NULL, 1, PROT_EXEC | PROT_READ, MAP_PRIVATE, fd, 0);
void *map = mmap (NULL, 1, PROT_EXEC | PROT_READ, MAP_PRIVATE, fd, 0);
if (map == MAP_FAILED) {
// Mapping failed; external tools will not see the readiness signal.
}

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@mdh1418 mdh1418 Jan 29, 2026

Choose a reason for hiding this comment

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

If we fail to create the mapping for some reason, should we even try again and if so, how many times?

Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The PROT_EXEC flag allows pages to be executed, which is a security concern. Since this mapping is only intended as a signal mechanism (the mapped memory is never accessed), PROT_EXEC is unnecessary and violates the principle of least privilege. The mapping should use minimal permissions required for its purpose. Consider using PROT_READ only, or even PROT_NONE if the mere existence of the mapping is sufficient for detection by external tools.

Suggested change
mmap (NULL, 1, PROT_EXEC | PROT_READ, MAP_PRIVATE, fd, 0);
mmap (NULL, 1, PROT_NONE, MAP_PRIVATE, fd, 0);

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@mdh1418 mdh1418 Jan 29, 2026

Choose a reason for hiding this comment

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

I tried just using PROT_READ (also suggested by Copilot CLI), but when running the userevents tests, it was insufficient for record-trace to discover the mapping.

Copy link
Member Author

@mdh1418 mdh1418 Jan 29, 2026

Choose a reason for hiding this comment

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

record-trace skips non executable mmaps.
https://github.com/microsoft/one-collect/blob/ebdb5acfbf5b767d0d648a74bc302e0807e50000/one_collect/src/helpers/dotnet/os/linux.rs#L1291-L1294

Copy link
Member

@hoyosjs hoyosjs Jan 29, 2026

Choose a reason for hiding this comment

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

Can it be at least just PROT_READ?

Copy link
Member

@jkotas jkotas Jan 29, 2026

Choose a reason for hiding this comment

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

record-trace skips non executable mmaps.

Can it be changed? That code will need to be modified to check for this mapping instead anyway.

Copy link
Member Author

@mdh1418 mdh1418 Jan 29, 2026

Choose a reason for hiding this comment

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

Asked in the record-trace PR microsoft/one-collect#229 (comment)

Copy link
Member

@lateralusX lateralusX Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If we have this memory map indicating that we have a diagnostic server up and running, I believe that is a much better indicator than the doublemapper they used in the past, so maybe they should steer away from that and only use this instead to find dotnet processes that support tracing.

close (fd);
}
}
#endif
result &= default_listen_port_ready;

ds_port_builder_fini (&default_port_builder);
} else {
Expand Down
2 changes: 2 additions & 0 deletions src/native/eventpipe/ep-shared-config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
#define EP_SHARED_CONFIG_H_INCLUDED

#cmakedefine01 HAVE_ERRNO_H
#cmakedefine01 HAVE_SYS_MMAN_H
#cmakedefine01 HAVE_MEMFD_CREATE
#cmakedefine01 HAVE_LINUX_USER_EVENTS_H
#cmakedefine01 HAVE_SYS_IOCTL_H
#cmakedefine01 HAVE_SYS_SOCKET_H
Expand Down
Loading