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

Race condition in wait(::Process) with process end? #54146

Open
NHDaly opened this issue Apr 19, 2024 · 1 comment
Open

Race condition in wait(::Process) with process end? #54146

NHDaly opened this issue Apr 19, 2024 · 1 comment
Labels
I/O Involving the I/O subsystem: libuv, read, write, etc.

Comments

@NHDaly
Copy link
Member

NHDaly commented Apr 19, 2024

I'm not very familiar with the details of process spawning, but this part looks like it could potentially be a race to me? Inside wait(x::Process), here, we double-check process_exited after grabbing the io_lock:

julia/base/process.jl

Lines 655 to 668 in 329f92c

iolock_begin()
if !process_exited(x)
preserve_handle(x)
lock(x.exitnotify)
iolock_end()
try
wait(x.exitnotify)
finally
unlock(x.exitnotify)
unpreserve_handle(x)
end
else
iolock_end()
end

But then, when the process is killed, we don't lock the io_lock before we notify the .exitnotify condition...:

julia/base/process.jl

Lines 61 to 67 in 329f92c

proc.handle = C_NULL
lock(proc.exitnotify)
try
notify(proc.exitnotify)
finally
unlock(proc.exitnotify)
end

Isn't it therefore possible that we could set the process's handle to NULL and signal the CV after the process_exited check, but before the lock(x.exitnotify), and thus the waiter would miss it?

That is, isn't there a possible race here?:

    iolock_begin()
    if !process_exited(x)  # x.handle != C_NULL
        # ------------
        # Right here, since `uv_return_spawn` hasn't locked the iolock, it could set x.handle = C_NULL,
        # and then notify exitnotify, before this task is waiting on it, so no one gets notified?
        # ------------
        preserve_handle(x)  # <-- this just preserves C_NULL :)
        lock(x.exitnotify)
        iolock_end()
        try
            wait(x.exitnotify)      # <-- then this waits forever?
        finally
            unlock(x.exitnotify)
            unpreserve_handle(x)
        end
    else
        iolock_end()
    end

Originally posted by @NHDaly in #54145 (comment)

@NHDaly NHDaly added the I/O Involving the I/O subsystem: libuv, read, write, etc. label Apr 19, 2024
@NHDaly
Copy link
Member Author

NHDaly commented Apr 19, 2024

Actually, even simpler, why is the iolock involved at all? Why doesn't the waiter lock the exitnotify before checking process_exited, and why doesn't uv_return_spawn lock the exitnotify before nulling the handle?

Isn't the monitor hygiene supposed to be: you should always be holding the monitor's lock before reading or writing the condition that the monitor is checking, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I/O Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Development

No branches or pull requests

1 participant