Skip to content

Commit

Permalink
process: ensure uvfinalize and _uv_close_cb are synchronized (#44476)
Browse files Browse the repository at this point in the history
There appeared to be a possibility they could race and the data field
might already be destroyed before we reached the close callback,
from looking at the state of the program when reproducing #44460.

This is because the uv_return_spawn set the handle to NULL, which later
can cause the uvfinalize to exit early (if the finalizer gets run on
another thread, since we have disabled finalizers on our thread). Then
the GC can reap the julia Process object prior to uv_close cleaning up
the object. We solve this by calling disassociate_julia_struct before
dropping the reference to the handle. But then we also fully address any
remaining race condition by having uvfinalize acquire a lock also.

The uv_return_spawn callback also needs to be synchronized with the
constructor, since we might have arrived there before we finished
allocating the Process struct here, leading to missed exit events.

Fixes #44460

(cherry picked from commit c591bf2)
  • Loading branch information
vtjnash authored and KristofferC committed Apr 19, 2022
1 parent 6e5b031 commit ea86a62
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 28 deletions.
68 changes: 42 additions & 26 deletions base/process.jl
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,13 @@ pipe_writer(p::ProcessChain) = p.in
# release ownership of the libuv handle
function uvfinalize(proc::Process)
if proc.handle != C_NULL
disassociate_julia_struct(proc.handle)
ccall(:jl_close_uv, Cvoid, (Ptr{Cvoid},), proc.handle)
proc.handle = C_NULL
iolock_begin()
if proc.handle != C_NULL
disassociate_julia_struct(proc.handle)
ccall(:jl_close_uv, Cvoid, (Ptr{Cvoid},), proc.handle)
proc.handle = C_NULL
end
iolock_end()
end
nothing
end
Expand All @@ -52,6 +56,7 @@ function uv_return_spawn(p::Ptr{Cvoid}, exit_status::Int64, termsignal::Int32)
proc = unsafe_pointer_to_objref(data)::Process
proc.exitcode = exit_status
proc.termsignal = termsignal
disassociate_julia_struct(proc) # ensure that data field is set to C_NULL
ccall(:jl_close_uv, Cvoid, (Ptr{Cvoid},), proc.handle)
proc.handle = C_NULL
lock(proc.exitnotify)
Expand All @@ -74,33 +79,44 @@ const SpawnIOs = Vector{Any} # convenience name for readability
# handle marshalling of `Cmd` arguments from Julia to C
@noinline function _spawn_primitive(file, cmd::Cmd, stdio::SpawnIOs)
loop = eventloop()
iohandles = Tuple{Cint, UInt}[ # assuming little-endian layout
let h = rawhandle(io)
h === C_NULL ? (0x00, UInt(0)) :
h isa OS_HANDLE ? (0x02, UInt(cconvert(@static(Sys.iswindows() ? Ptr{Cvoid} : Cint), h))) :
h isa Ptr{Cvoid} ? (0x04, UInt(h)) :
error("invalid spawn handle $h from $io")
cpumask = cmd.cpus
cpumask === nothing || (cpumask = as_cpumask(cmd.cpus))
GC.@preserve stdio begin
iohandles = Tuple{Cint, UInt}[ # assuming little-endian layout
let h = rawhandle(io)
h === C_NULL ? (0x00, UInt(0)) :
h isa OS_HANDLE ? (0x02, UInt(cconvert(@static(Sys.iswindows() ? Ptr{Cvoid} : Cint), h))) :
h isa Ptr{Cvoid} ? (0x04, UInt(h)) :
error("invalid spawn handle $h from $io")
end
for io in stdio]
handle = Libc.malloc(_sizeof_uv_process)
disassociate_julia_struct(handle)
(; exec, flags, env, dir) = cmd
iolock_begin()
err = ccall(:jl_spawn, Int32,
(Cstring, Ptr{Cstring}, Ptr{Cvoid}, Ptr{Cvoid},
Ptr{Tuple{Cint, UInt}}, Int,
UInt32, Ptr{Cstring}, Cstring, Ptr{Bool}, Csize_t, Ptr{Cvoid}),
file, exec, loop, handle,
iohandles, length(iohandles),
flags,
env === nothing ? C_NULL : env,
isempty(dir) ? C_NULL : dir,
cpumask === nothing ? C_NULL : cpumask,
cpumask === nothing ? 0 : length(cpumask),
@cfunction(uv_return_spawn, Cvoid, (Ptr{Cvoid}, Int64, Int32)))
if err == 0
pp = Process(cmd, handle)
associate_julia_struct(handle, pp)
else
ccall(:jl_forceclose_uv, Cvoid, (Ptr{Cvoid},), handle) # will call free on handle eventually
end
for io in stdio]
handle = Libc.malloc(_sizeof_uv_process)
disassociate_julia_struct(handle) # ensure that data field is set to C_NULL
(; exec, flags, env, dir) = cmd
err = ccall(:jl_spawn, Int32,
(Cstring, Ptr{Cstring}, Ptr{Cvoid}, Ptr{Cvoid},
Ptr{Tuple{Cint, UInt}}, Int,
UInt32, Ptr{Cstring}, Cstring, Ptr{Cvoid}),
file, exec, loop, handle,
iohandles, length(iohandles),
flags,
env === nothing ? C_NULL : env,
isempty(dir) ? C_NULL : dir,
@cfunction(uv_return_spawn, Cvoid, (Ptr{Cvoid}, Int64, Int32)))
iolock_end()
end
if err != 0
ccall(:jl_forceclose_uv, Cvoid, (Ptr{Cvoid},), handle) # will call free on handle eventually
throw(_UVError("could not spawn " * repr(cmd), err))
end
pp = Process(cmd, handle)
associate_julia_struct(handle, pp)
return pp
end

Expand Down
4 changes: 2 additions & 2 deletions src/jl_uv.c
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ JL_DLLEXPORT void jl_uv_disassociate_julia_struct(uv_handle_t *handle)
handle->data = NULL;
}

#define UV_CLOSED 0x02 // UV_HANDLE_CLOSED on Windows (same value)
#define UV_HANDLE_CLOSED 0x02

JL_DLLEXPORT int jl_spawn(char *name, char **argv,
uv_loop_t *loop, uv_process_t *proc,
Expand All @@ -310,7 +310,7 @@ JL_DLLEXPORT int jl_spawn(char *name, char **argv,
if (!(flags == UV_INHERIT_FD || flags == UV_INHERIT_STREAM || flags == UV_IGNORE)) {
proc->type = UV_PROCESS;
proc->loop = loop;
proc->flags = UV_CLOSED;
proc->flags = UV_HANDLE_CLOSED;
return UV_EINVAL;
}
}
Expand Down

0 comments on commit ea86a62

Please sign in to comment.