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

Handle waitpid race condition when SIGCHLD is set to SIG_IGN #57241

Merged
merged 2 commits into from
Feb 4, 2025
Merged

Handle waitpid race condition when SIGCHLD is set to SIG_IGN #57241

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
253bc4f
Handle `waitpid` race condition when `SIGCHLD` is set to `SIG_IGN`
topolarity Feb 3, 2025
0ea3f84
Add basic `libdl` embedding test
topolarity Feb 3, 2025
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
12 changes: 9 additions & 3 deletions cli/loader_lib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,16 @@ static char *libstdcxxprobe(void)
pid_t npid = waitpid(pid, &wstatus, 0);
if (npid == -1) {
if (errno == EINTR) continue;
if (errno != EINTR) {
perror("Error during libstdcxxprobe in parent process:\nwaitpid");
exit(1);
if (errno == ECHILD) {
// SIGCHLD is set to SIG_IGN or has flag SA_NOCLDWAIT, so the child
Copy link
Member

Choose a reason for hiding this comment

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

This can be worked around by e.g. using clone on linux and the raw process APIs on osx (and possibly freebsd) if we want to.

Copy link
Member Author

@topolarity topolarity Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I find this fork() (or clone, etc.) fairly suspect to begin with TBH

As we try to be embeddable / loadable in larger applications it becomes more likely that (1) libstdc++ has already been loaded and we're too late to do anything, and (2) the address space of the application is large and mutating, so that fork() can become potentially expensive

I'd prefer to try to remove our dependency on libstdc++ for small, simple Julia-exported libraries TBH and get rid of this altogether

Copy link
Member Author

Choose a reason for hiding this comment

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

Even better, we should probably just mmap the .so and check its symbol table, rather than trying to do this sandboxed dlopen.

The hard part is emulating the linker search path though..

// did not become a zombie and wait for `waitpid` - it just exited.
//
// Assume that it exited successfully and use whatever libpath we
// got out of the pipe, if any.
break;
}
perror("Error during libstdcxxprobe in parent process:\nwaitpid");
exit(1);
}
else if (!WIFEXITED(wstatus)) {
const char *err_str = "Error during libstdcxxprobe in parent process:\n"
Expand Down
14 changes: 11 additions & 3 deletions test/embedding/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ EXE := $(suffix $(abspath $(JULIA)))

# get compiler and linker flags. (see: `contrib/julia-config.jl`)
JULIA_CONFIG := $(JULIA) -e 'include(joinpath(Sys.BINDIR, Base.DATAROOTDIR, "julia", "julia-config.jl"))' --
JULIA_LIBDIR := $(shell $(JULIA) -e 'println(joinpath(Sys.BINDIR, "..", "lib"))' --)
CPPFLAGS_ADD :=
CFLAGS_ADD = $(shell $(JULIA_CONFIG) --cflags)
LDFLAGS_ADD = -lm $(shell $(JULIA_CONFIG) --ldflags --ldlibs)
Expand All @@ -29,23 +30,30 @@ DEBUGFLAGS += -g

#=============================================================================

release: $(BIN)/embedding$(EXE)
debug: $(BIN)/embedding-debug$(EXE)
release: $(BIN)/embedding$(EXE) $(BIN)/libdl-embedding$(EXE)
debug: $(BIN)/embedding-debug$(EXE) $(BIN)/libdl-embedding$(EXE)

$(BIN)/embedding$(EXE): $(SRCDIR)/embedding.c
$(CC) $^ -o $@ $(CPPFLAGS_ADD) $(CPPFLAGS) $(CFLAGS_ADD) $(CFLAGS) $(LDFLAGS_ADD) $(LDFLAGS)

$(BIN)/embedding-debug$(EXE): $(SRCDIR)/embedding.c
$(CC) $^ -o $@ $(CPPFLAGS_ADD) $(CPPFLAGS) $(CFLAGS_ADD) $(CFLAGS) $(LDFLAGS_ADD) $(LDFLAGS) $(DEBUGFLAGS)

$(BIN)/libdl-embedding$(EXE): $(SRCDIR)/libdl_embedding.c
$(CC) $^ -o $@ $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) -ldl -DLIBJULIA_PATH=\"$(JULIA_LIBDIR)/libjulia.so\"

$(BIN)/libdl-embedding-debug$(EXE): $(SRCDIR)/libdl_embedding.c
$(CC) $^ -o $@ $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(DEBUGFLAGS) -ldl -DLIBJULIA_PATH=\"$(JULIA_LIBDIR)/libjulia.so\"

ifneq ($(abspath $(BIN)),$(abspath $(SRCDIR)))
# for demonstration purposes, our demo code is also installed
# in $BIN, although this would likely not be typical
$(BIN)/LocalModule.jl: $(SRCDIR)/LocalModule.jl
cp $< $@
endif

check: $(BIN)/embedding$(EXE) $(BIN)/LocalModule.jl
check: $(BIN)/embedding$(EXE) $(BIN)/libdl-embedding$(EXE) $(BIN)/LocalModule.jl
$(BIN)/libdl-embedding$(EXE) # run w/o error
$(JULIA) --depwarn=error $(SRCDIR)/embedding-test.jl $<
@echo SUCCESS

Expand Down
12 changes: 12 additions & 0 deletions test/embedding/libdl_embedding.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#include <dlfcn.h>
#include <stdio.h>
#include <signal.h>

int main(int argc, char *argv[])
{
// This test doesn't do much yet, except check
// https://github.com/JuliaLang/julia/issues/57240
signal(SIGCHLD, SIG_IGN);
void *handle = dlopen(LIBJULIA_PATH, RTLD_LAZY);
return 0;
}