wl-copy does not close stderr when forking

[sfan5]

https://github.com/bugaevc/wl-clipboard/blob/master/src/wl-copy.c#L65

This trips up applications that try to capture output from child processes (because stderr remains open)

[sfan5]

Seeing as this has been brought up before in #110 and #154 let me elaborate:

A reasonably common case for mpv Lua scripts is to copy something to the clipboard. This is achieved using something like this:

local function copy_to_clipboard(text)
    if os.getenv("WAYLAND_DISPLAY") then
        local r = mp.command_native({
            name = "subprocess",
            args = {"wl-copy", text},
            playback_only = false,
        })
        if r.status == 0 then
            return true
        end
    else
        -- omitted for brevity
    end
    msg.error("Error writing clipboard")
end

The only problem with this is that is hangs, until something else is selected because at that point wl-copy exits. Now mpv does not try to observe the child process specifically, but it internally captures the stdout and stderr output so it blocks until the child process has exited regardless.

For me what speaks against keeping stderr open:

• too late to properly signal an error anyway (exit code)
• xclip doesn't cause the same issue (and I'm pretty sure it has to fork too)
• likely to trigger exactly these edge cases where parent applications don't expect a child to fork and keep one of its fds

Now I understand you don't want to swallow the error so how about logging to syslog instead?

[sfan5]

@bugaevc thoughts?

[bugaevc]

Hi,

The only problem with this is that is hangs, until something else is selected because at that point wl-copy exits. Now mpv does not try to observe the child process specifically, but it internally captures the stdout and stderr output so it blocks until the child process has exited regardless.

well, this sounds like something to be fixed in mpv, no?

xclip doesn't cause the same issue (and I'm pretty sure it has to fork too)

It does fork, but are you sure mpv doesn't have the same issue with xclip? It certainly doesn't look like xclip closes its stderr (or stdout), neither from skimming their source code, nor in practice:

$ echo hi | strace -f --trace-fds=0,1,2 xclip -i &
[1] 216777
newfstatat(0, "", {st_mode=S_IFIFO|0600, st_size=0, ...}, AT_EMPTY_PATH) = 0
read(0, "hi\n", 4096)                   = 3
read(0, "", 4096)                       = 0
strace: Process 216785 attached
[pid 216784] +++ exited with 0 +++
$ echo "This is sent to xclip's stderr" > /proc/216785/fd/2
This is sent to xclip's stderr
$ readlink /proc/216785/fd/2 /proc/self/fd/2
/dev/pts/3
/dev/pts/3

Now I understand you don't want to swallow the error so how about logging to syslog instead?

That would make sense, yes. Do you know if there's a way to do this nicely? It's possible to send messages to the log using explicit syslog(3) calls, but that won't handle text that just gets printed to stderr by most of wl-clipboard's code, or by library functions. There's logger(1) and systemd-cat(1), but that'd be an overkill.

[sfan5]

I re-did my testing and you're right. xclip behaves exactly the same and results in the same issue with mpv. I did not realize that my original code using xclip looked totally different (popen+write+close), which of course behaved totally differently.

well, this sounds like something to be fixed in mpv, no?

True. This unfortunately requires some annoying workarounds since you can't poll for the death of a process (not portably at least).

That would make sense, yes. Do you know if there's a way to do this nicely?

wl-copy's code could be refactored to use a logging mechanism that can be redirected. I believe external libraries printing directly to stderr is considered bad practice so hopefully that's not a thing that actually happens. Failing that a more lightweight way would be to replace stderr with a pipe and have a thread feed the output to syslog(3).

If you don't want to pursue this request I can understand, however looking at the issues other people have opened relating to this behavior I think it would be worthwhile.

[bugaevc]

This unfortunately requires some annoying workarounds since you can't poll for the death of a process (not portably at least).

FWIW, you can combine poll() with signals portably like this:

volatile sig_atomic_t got_sigchld= 0;

static void sigchld_handler(int signum) {
    assert(signum == SIGCHLD);
    got_sigchld = 1;
}

// Block SIGCHLD, remembering the previous mask.
sigset_t sigmask;
sigprocmask(SIG_SETMASK, NULL, &sigmask);
sigaddset(&sigmask, SIGCHLD);
sigprocmask(SIG_SETMASK, &sigmask, &sigmask);

while (!got_sigchld) {
    int rc = ppoll(pollfds, nfds, &timeout, &sigmask);
    if (rc == -1 && errno == EINTR) continue;
    // handle pollfds...
}
// The child has exited, wait for it (or something).
wait(...);