bug#58960: 29.0.50; Assert fails when browsing an URL

bug#58960: 29.0.50; Assert fails when browsing an URL

[Tino Calancha]

emacs -Q
M-x browse-url RET
https://www.example.com RET

[The URL is opened by my default browser and Emacs crashes with the
following backtrace]

process.c:7386: Emacs fatal error: assertion failed: 0 <= fd
Fatal error 6: Aborted
Backtrace:
emacs[0x610be3]
emacs[0x5d806c]
emacs[0x68a1a0]
emacs[0x73b497]
emacs[0x73b900]
emacs[0x6103ae]
emacs[0x73b92b]
/lib64/libpthread.so.0(+0x12ce0)[0x7f937e22ace0]
emacs[0x669b85]
emacs[0x669bf0]
emacs[0x675dd7]
emacs[0x673dfc]
emacs[0x6736f3]
emacs[0x661f1f]
emacs[0x63996b]
emacs[0x63ac19]
emacs[0x6c5492]
emacs[0x7236f7]
emacs[0x6c578c]
emacs[0x6c5be4]
emacs[0x6c4f15]
emacs[0x6c520c]
emacs[0x6c406f]
emacs[0x6c5629]
emacs[0x7236f7]
emacs[0x6c578c]
emacs[0x6c5be4]
emacs[0x6c4f15]
emacs[0x6c520c]
emacs[0x5dd79a]
emacs[0x5ea9e8]
emacs[0x5eab38]
emacs[0x5e7b76]
emacs[0x5f1d97]
emacs[0x5fb14e]
emacs[0x5e486a]
emacs[0x5f9229]
emacs[0x5e090f]
emacs[0x6c0a85]
emacs[0x5e0110]
emacs[0x6bfd21]
emacs[0x5e00a8]
emacs[0x5df4a5]
emacs[0x5df6ad]
emacs[0x5db103]
/lib64/libc.so.6(__libc_start_main+0xf3)[0x7f937d057cf3]
emacs[0x4180ce]




In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
  version 1.15.12, Xaw scroll bars) of 2022-11-02 built on
  tcalanch.remote.csb
Repository revision: 8a5678906fa1b899c4d111e5ee4334b278f50d48
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Red Hat Enterprise Linux 8.6 (Ootpa)

Configured using:
  'configure --enable-checking=yes,glyphs --enable-check-lisp-object-type
  --with-x-toolkit=lucid 'CFLAGS=-O0 -g3' --with-gif=ifavailable'

Configured features:
ACL CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG
LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND
THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XINPUT2 XPM LUCID ZLIB

Important settings:
   value of $LANG: en_US.UTF-8
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   line-number-mode: t
   indent-tabs-mode: t
   transient-mark-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 36693 5402)
  (symbols 48 5093 0)
  (strings 32 13962 1228)
  (string-bytes 1 383102)
  (vectors 16 9295)
  (vector-slots 8 147845 11034)
  (floats 8 34 25)
  (intervals 56 239 0)
  (buffers 984 10))

bug#58960: 29.0.50; Assert fails when browsing an URL

[Gerd Möllmann]

FWIW, this is not reproducible on macOS.

bug#58960: 29.0.50; Assert fails when browsing an URL, bug#58960: 29.0.50; Assert fails when browsing an URL

[Robert Pluim]

>>>>> On Wed, 2 Nov 2022 05:48:30 +0100 (CET), Tino Calancha <tino.calancha@gmail.com> said:

    Tino> emacs -Q
    Tino> M-x browse-url RET
    Tino> https://www.example.com RET

    Tino> [The URL is opened by my default browser and Emacs crashes with the
    Tino> following backtrace]

    Tino> process.c:7386: Emacs fatal error: assertion failed: 0 <= fd

This goes away if you do something like 'M-x shell' first, right?

Looks like `call-process' needs to ensure the child signal fdʼs are
set up before calling `emacs_spawn'. Iʼm suprised nobody has run into
this before. Does the (dirty) patch below fix things for you?

Robert
-- 

diff --git i/src/process.c w/src/process.c
index 358899cded..4690addcf1 100644
--- i/src/process.c
+++ w/src/process.c
@@ -292,7 +292,7 @@ network_lookup_address_info_1 (Lisp_Object host, const char *service,
    descriptor to notify `wait_reading_process_output' of process
    status changes.  */
 static int child_signal_write_fd = -1;
-static void child_signal_init (void);
+void child_signal_init (void);
 #ifndef WINDOWSNT
 static void child_signal_read (int, void *);
 #endif
@@ -7323,7 +7323,7 @@ DEFUN ("process-send-eof", Fprocess_send_eof, Sprocess_send_eof, 0, 1, 0,
 
 /* Set up `child_signal_read_fd' and `child_signal_write_fd'.  */
 
-static void
+void
 child_signal_init (void)
 {
   /* Either both are initialized, or both are uninitialized.  */
diff --git i/src/callproc.c w/src/callproc.c
index 4d4b86629c..10ec643861 100644
--- i/src/callproc.c
+++ w/src/callproc.c
@@ -328,7 +328,7 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
    this case NARGS must be at least 2 and ARGS[1] is the file's name.
 
    At entry, the specpdl stack top entry must be close_file_unwind (FILEFD).  */
-
+extern void child_signal_init (void);
 static Lisp_Object
 call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 	      specpdl_ref tempfile_index)
@@ -650,7 +650,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 
   block_input ();
   block_child_signal (&oldset);
-
+  child_signal_init ();
   child_errno
     = emacs_spawn (&pid, filefd, fd_output, fd_error, new_argv, env,
                    SSDATA (current_dir), NULL, false, false, &oldset);

bug#58960: 29.0.50; Assert fails when browsing an URL, bug#58960: 29.0.50; Assert fails when browsing an URL

[Eli Zaretskii]

> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>,
>  58960@debbugs.gnu.org
> From: Robert Pluim <rpluim@gmail.com>
> Date: Wed, 02 Nov 2022 11:20:31 +0100
> 
> Looks like `call-process' needs to ensure the child signal fdʼs are
> set up before calling `emacs_spawn'.

Why do we need this?  IOW, do you understand how did SIGCHLD cause
this?

bug#58960: 29.0.50; Assert fails when browsing an URL, bug#58960: 29.0.50; Assert fails when browsing an URL

[Robert Pluim]

>>>>> On Wed, 02 Nov 2022 15:10:07 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>,
    >> 58960@debbugs.gnu.org
    >> From: Robert Pluim <rpluim@gmail.com>
    >> Date: Wed, 02 Nov 2022 11:20:31 +0100
    >> 
    >> Looks like `call-process' needs to ensure the child signal fdʼs are
    >> set up before calling `emacs_spawn'.

    Eli> Why do we need this?  IOW, do you understand how did SIGCHLD cause
    Eli> this?

`browse-url' does `call-process' for `xdg-open' by default. `xdg-open'
exits almost immediately, we get SIGCHLD:

(gdb) bt
#0  terminate_due_to_signal (sig=6, backtrace_limit=2147483647) at emacs.c:421
#1  0x00005555555b489e in die
    (msg=msg@entry=0x5555558d938f "0 <= fd", file=file@entry=0x5555558d9354 "process.c", line=line@entry=7386) at alloc.c:7692
#2  0x00005555555bfec9 in child_signal_notify () at process.c:7386
#3  handle_child_signal (sig=<optimized out>) at process.c:7493
#4  0x000055555574e992 in deliver_process_signal
    (sig=17, handler=0x555555831b40 <handle_child_signal>) at sysdep.c:1741
#5  0x00007ffff5752140 in <signal handler called> ()

`child_signal_notify' does this:

  int fd = child_signal_write_fd;
  eassert (0 <= fd);

and if `child_signal_init' hasnʼt been called, then this is still
true:

/* The write end thereof.  The SIGCHLD handler writes to this file
   descriptor to notify `wait_reading_process_output' of process
   status changes.  */
static int child_signal_write_fd = -1;

so the assert fails.

Why canʼt we just call `child_signal_init' from `init_process_emacs'
instead of `create_process'?

Robert
-- 

bug#58960: 29.0.50; Assert fails when browsing an URL, bug#58960: 29.0.50; Assert fails when browsing an URL

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  tino.calancha@gmail.com,
>   gerd.moellmann@gmail.com,  58960@debbugs.gnu.org
> Date: Wed, 02 Nov 2022 14:58:23 +0100
> 
> >>>>> On Wed, 02 Nov 2022 15:10:07 +0200, Eli Zaretskii <eliz@gnu.org> said:
> 
>     >> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>,
>     >> 58960@debbugs.gnu.org
>     >> From: Robert Pluim <rpluim@gmail.com>
>     >> Date: Wed, 02 Nov 2022 11:20:31 +0100
>     >> 
>     >> Looks like `call-process' needs to ensure the child signal fdʼs are
>     >> set up before calling `emacs_spawn'.
> 
>     Eli> Why do we need this?  IOW, do you understand how did SIGCHLD cause
>     Eli> this?
> 
> `browse-url' does `call-process' for `xdg-open' by default. `xdg-open'
> exits almost immediately, we get SIGCHLD:

Ugh, xdg-open again...

> (gdb) bt
> #0  terminate_due_to_signal (sig=6, backtrace_limit=2147483647) at emacs.c:421
> #1  0x00005555555b489e in die
>     (msg=msg@entry=0x5555558d938f "0 <= fd", file=file@entry=0x5555558d9354 "process.c", line=line@entry=7386) at alloc.c:7692
> #2  0x00005555555bfec9 in child_signal_notify () at process.c:7386
> #3  handle_child_signal (sig=<optimized out>) at process.c:7493
> #4  0x000055555574e992 in deliver_process_signal
>     (sig=17, handler=0x555555831b40 <handle_child_signal>) at sysdep.c:1741
> #5  0x00007ffff5752140 in <signal handler called> ()
> 
> `child_signal_notify' does this:
> 
>   int fd = child_signal_write_fd;
>   eassert (0 <= fd);
> 
> and if `child_signal_init' hasnʼt been called, then this is still
> true:
> 
> /* The write end thereof.  The SIGCHLD handler writes to this file
>    descriptor to notify `wait_reading_process_output' of process
>    status changes.  */
> static int child_signal_write_fd = -1;
> 
> so the assert fails.
> 
> Why canʼt we just call `child_signal_init' from `init_process_emacs'
> instead of `create_process'?

Maybe we could.  Assuming the signal stuff is already set so early, I
don't know exactly how posix_spawn works.

Paul, WDYT about this?

bug#58960: 29.0.50; Assert fails when browsing an URL, bug#58960: 29.0.50; Assert fails when browsing an URL

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On 11/2/22 08:09, Eli Zaretskii wrote:
>> Why canʼt we just call `child_signal_init' from `init_process_emacs'
>> instead of `create_process'?
> Maybe we could.  Assuming the signal stuff is already set so early, I
> don't know exactly how posix_spawn works.

child_signal_init is reasonably heavyweight in that it keeps a couple of 
file descriptors open, so the idea is to be lazy and not call it unless 
Emacs plans to have children.

I installed the attached, which is like Robert's patch except it keeps 
the critical section smaller and checks the declaration of the 
now-extern function. Please give it a try. I'll boldly close the bug 
report; we can reopen it if I'm wrong.

[-- Attachment #2: 0001-Initialize-child-signal-handling-before-posix_spawn-.patch --]
[-- Type: text/x-patch, Size: 2157 bytes --]

From 05f5d978ae70c4849a5c47865d62301d27317a8a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 2 Nov 2022 13:24:26 -0700
Subject: [PATCH] Initialize child signal handling before posix_spawn too.

Problem reported by Tino Calancha (Bug#58960).
* src/callproc.c (call_process): Initialize SIGCHLD handling
before possibly creating a child with emacs_span.  This need not
be in the critical section that calls emacs_spawn, so do it
outside the critical section.
* src/process.c (child_signal_init): Now extern.
---
 src/callproc.c | 1 +
 src/lisp.h     | 1 +
 src/process.c  | 3 +--
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index 4d4b86629c..f9f840e544 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -648,6 +648,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 
 #ifndef MSDOS
 
+  child_signal_init ();
   block_input ();
   block_child_signal (&oldset);
 
diff --git a/src/lisp.h b/src/lisp.h
index d87f954938..eafa241adf 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4915,6 +4915,7 @@ #define DAEMON_RUNNING (w32_daemon_event != INVALID_HANDLE_VALUE)
 
 /* Defined in process.c.  */
 struct Lisp_Process;
+extern void child_signal_init (void);
 extern void kill_buffer_processes (Lisp_Object);
 extern int wait_reading_process_output (intmax_t, int, int, bool, Lisp_Object,
 					struct Lisp_Process *, int);
diff --git a/src/process.c b/src/process.c
index 358899cded..5144c5d6c9 100644
--- a/src/process.c
+++ b/src/process.c
@@ -292,7 +292,6 @@ network_lookup_address_info_1 (Lisp_Object host, const char *service,
    descriptor to notify `wait_reading_process_output' of process
    status changes.  */
 static int child_signal_write_fd = -1;
-static void child_signal_init (void);
 #ifndef WINDOWSNT
 static void child_signal_read (int, void *);
 #endif
@@ -7323,7 +7322,7 @@ DEFUN ("process-send-eof", Fprocess_send_eof, Sprocess_send_eof, 0, 1, 0,
 
 /* Set up `child_signal_read_fd' and `child_signal_write_fd'.  */
 
-static void
+void
 child_signal_init (void)
 {
   /* Either both are initialized, or both are uninitialized.  */
-- 
2.38.1

bug#58960: 29.0.50; Assert fails when browsing an URL, bug#58960: 29.0.50; Assert fails when browsing an URL

[Tino Calancha]

On Wed, 2 Nov 2022, Paul Eggert wrote:


> child_signal_init is reasonably heavyweight in that it keeps a couple of file 
> descriptors open, so the idea is to be lazy and not call it unless Emacs 
> plans to have children.
>
> I installed the attached, which is like Robert's patch except it keeps the 
> critical section smaller and checks the declaration of the now-extern 
> function. Please give it a try. I'll boldly close the bug report; we can 
> reopen it if I'm wrong.

I have applied your patch; it fixes the issue.
Thank you.

bug#58960: 29.0.50; Assert fails when browsing an URL, bug#58960: 29.0.50; Assert fails when browsing an URL

[Robert Pluim]

>>>>> On Thu, 3 Nov 2022 04:00:26 +0100 (CET), Tino Calancha <tino.calancha@gmail.com> said:

    Tino> On Wed, 2 Nov 2022, Paul Eggert wrote:


    >> child_signal_init is reasonably heavyweight in that it keeps a
    >> couple of file descriptors open, so the idea is to be lazy and not
    >> call it unless Emacs plans to have children.
    >>

Sure. Calling it from `init_process_emacs' works as well, but then
thereʼs some scary looking "if (!will_dump_with_unexec_p ())", so
letʼs not go there :-)

    >> I installed the attached, which is like Robert's patch except it
    >> keeps the critical section smaller and checks the declaration of the
    >> now-extern function. Please give it a try. I'll boldly close the bug
    >> report; we can reopen it if I'm wrong.

    Tino> I have applied your patch; it fixes the issue.
    Tino> Thank you.

Works for me as well, thanks Paul

Robert
--