Skip to content

Conversation

@oconnor663
Copy link
Contributor

@oconnor663 oconnor663 commented Jun 11, 2025

I've been investigating astral-sh/uv#13919, and I believe I've traced it to some questionable behavior in libedit, but I genuinely don't know where to report that. We might want to patch it as in this PR, but even if not I wanted some place to write all this down so we can reference it.

Our standalone build substitutes libedit for libreadline on Linux. It turns out that libedit has some funny signal handling behavior. Here's a minimized repro. First, a toy C program that we can link against either libreadline or libedit, call it test.c:

#include <stdio.h>
#include <stdlib.h>

#include <readline/readline.h>

int main(int argc, char **argv) {
  char *line = readline("Reading a line: ");
  if (line) {
    printf("You entered: %s\n", line);
    free(line);
  } else {
    printf("EOF received\n");
  }
  return 0;
}

Now, a wrapper Rust program that starts the C program (or anything else you give it on the command line), signals it (currently SIGTERM, but SIGINT behaves the same in these examples if you edit it in), and waits for it to exit, listening for signals the whole time:

#!/usr/bin/env -S cargo +nightly -Zscript -q

---
[package]
edition = "2024"

[dependencies]
signal-hook = "0.3.18"
libc = "0.2.172"
---

use signal_hook::consts::{SIGINT, SIGTERM};
use signal_hook::iterator::Signals;
use std::thread;
use std::time::Duration;

fn listen_for_signals() {
    let mut signals = Signals::new(&[SIGINT, SIGTERM]).unwrap();
    for signal in signals.forever() {
        println!("wrapper received signal: {}", signal);
    }
}

fn main() -> std::io::Result<()> {
    // Spawn a bg thread to listen for signals.
    thread::spawn(listen_for_signals);
    // Spawn the child process.
    let argv: Vec<_> = std::env::args().skip(1).collect();
    let mut child = std::process::Command::new(&argv[0])
        .args(&argv[1..])
        .spawn()?;
    // Give the child time to start.
    thread::sleep(Duration::from_millis(100));
    // Signal the child.
    unsafe {
        libc::kill(child.id() as _, libc::SIGTERM);
    }
    // Give the child time to signal us.
    thread::sleep(Duration::from_millis(100));
    // Wait on the child in case it's still running.
    child.wait()?;
    Ok(())
}

If I link the C code against libreadline and run it, nothing is amiss (libreadline also clears its prompt when signaled, which is neat). Here's me doing it on Arch Linux, with both readline and libedit installed:

$ gcc test.c -lreadline
$ ./signal_wrapper.rs ./a.out
$

However, if I link the C code against libedit, the wrapper process catches a signal (libedit also doesn't clear its prompt, but that doesn't matter):

$ gcc test.c -ledit
$ ./signal_wrapper.rs ./a.out
Reading a line: wrapper received signal: 15
$

Similarly, if I comment out the libc::kill call in the wrapper and use ^C myself in the terminal, the wrapper usually catches SIGINT once as expected, but occasionally it catches SIGINT twice:

$ gcc test.c -ledit
$ ./signal_wrapper.rs ./a.out
Reading a line: wrapper received signal: 2
$ ./signal_wrapper.rs ./a.out
Reading a line: wrapper received signal: 2
$ ./signal_wrapper.rs ./a.out
Reading a line: wrapper received signal: 2
$ ./signal_wrapper.rs ./a.out
Reading a line: wrapper received signal: 2
$ ./signal_wrapper.rs ./a.out
Reading a line: wrapper received signal: 2
$ ./signal_wrapper.rs ./a.out
Reading a line: wrapper received signal: 2
wrapper received signal: 2                     <--- a second signal from one ^C
$

The origin of this behavior is the sig_handler function in libedit's sig.c, which re-broadcasts all(?) signals to the entire pgroup. Even though we're working around the original uv ^C issue in a different way at the same time (astral-sh/uv#13925), this is pretty weird behavior, and we might want to consider patching it. I have some comments and speculation about why the behavior is there, inline above the patch.

You can use the same wrapper program with our existing standalone builds, putting the libc::kill back in (though note that Python 3.13 no longer uses readline/libedit unless you set PYTHON_BASIC_REPL=1):

$ ./signal_wrapper.rs ~/.local/share/uv/python/cpython-3.11.13-linux-x86_64-gnu/bin/python
Python 3.11.13 (main, Jun  4 2025, 17:37:17) [Clang 20.1.4 ] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> wrapper received signal: 15
$

After running ./build-linux.sh with this patched version of cpython-unix/build-libedit.sh and untar'ing the result, I've confirmed that the misguided signal is gone:

$ ./signal_wrapper.rs ~/astral/python-build-standalone/dist/python/install/bin/python
Python 3.11.13 (main, Jun 11 2025, 02:37:45) [Clang 20.1.4 ] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>>
$

@geofft I'm curious to get your thoughts about this. Have you seen it before? It's obscure enough that we might decide that fixing it isn't worth the trouble of maintaining the patch, but I at least wanted to write all this down. It also seems like something I should report upstream, but again I'm not sure where...FreeBSD?

@oconnor663
Copy link
Contributor Author

Opened a ticket upstream, will see if we get any comments: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=287452

@zanieb zanieb added platform:darwin Specific to the macOS platform platform:linux Specific to the Linux platform labels Jun 11, 2025
@oconnor663 oconnor663 added the bug Something isn't working label Jun 11, 2025
@geofft
Copy link
Collaborator

geofft commented Jun 14, 2025

@zoulasc I would love your take on this, this looks like a bug dating from the original 4.4BSD version of libedit :)

Briefly, our understanding is that libedit never puts the terminal into ~ISIG mode, and so Ctrl-C during el_gets() is handled through the signal handler sig.c, which gets out of line-reading mode, restores the original signal handler, and does kill(0, signo). I think the intention there is to call the original signal handler in the current process, but if there other things in the process group, it gets to them too which gets weird. At the very least, Ctrl-C would have sent SIGINT to all the processes in the foreground pgrp anyway (although signal coalescing means they usually won't notice). And in particular, if the process running libedit is a child in the same process group as some watcher process that forwards signals to the child (which is what uv does), you can get in a weird infinite loop. See @oconnor663's excellent writeup above and in the linked issue for more details.

I suspect that, at least for an interactive shell, it's uncommon for it to be sharing its pgrp with anyone, and so kill(0, signo) is in practice raise(signo), which is why nobody's really complained in 30+ years. But for non-shell REPLs like Python this is more of a problem. I also just found that the Postgres folks seem to have run into the same problem, and they note that GNU readline only signals the current process.

The patch in this PR changes to raise(signo). Does that seem correct to you, and should we try to get that into upstream libedit in NetBSD and all the other places that carry it?

@geofft
Copy link
Collaborator

geofft commented Jun 14, 2025

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=159226 also looks like someone noticing this issue, judging from the patch, though they are running into a seemingly different problem.

FreeBSD seems to now use snapshots of libedit from some other upstream, presumably NetBSD.

@zoulasc
Copy link

zoulasc commented Jun 14, 2025

Changed to use raise(signo). Thanks!

Copy link
Collaborator

@geofft geofft left a comment

Choose a reason for hiding this comment

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

Thanks, Christos! That means the Thrysoee libedit distribution will pick this up soon enough so I feel very comfortable merging this.

@geofft geofft merged commit 69f53e2 into main Jun 14, 2025
435 checks passed
@geofft geofft deleted the jack/libedit_patch branch June 14, 2025 16:13
@oconnor663
Copy link
Contributor Author

NetBSD has taken this change upstream 🎉 https://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/sig.c?rev=1.29

@geofft geofft mentioned this pull request Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working platform:darwin Specific to the macOS platform platform:linux Specific to the Linux platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants