From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#25178: 26.0.50; Crash when pressing C-g in TTY mode Date: Sat, 17 Dec 2016 15:58:58 +0200 Message-ID: <83eg167j31.fsf@gnu.org> References: <83bmwhccib.fsf@gnu.org> <838trjbrcq.fsf@gnu.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1481983222 5552 195.159.176.226 (17 Dec 2016 14:00:22 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 17 Dec 2016 14:00:22 +0000 (UTC) Cc: 25178@debbugs.gnu.org To: Elias Martenson Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sat Dec 17 15:00:18 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cIFX7-0000eQ-50 for geb-bug-gnu-emacs@m.gmane.org; Sat, 17 Dec 2016 15:00:17 +0100 Original-Received: from localhost ([::1]:36822 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cIFXA-0001Wr-24 for geb-bug-gnu-emacs@m.gmane.org; Sat, 17 Dec 2016 09:00:20 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:33574) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cIFWx-0001Py-1N for bug-gnu-emacs@gnu.org; Sat, 17 Dec 2016 09:00:08 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cIFWs-0005lt-P7 for bug-gnu-emacs@gnu.org; Sat, 17 Dec 2016 09:00:07 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:57657) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cIFWs-0005lg-MZ for bug-gnu-emacs@gnu.org; Sat, 17 Dec 2016 09:00:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1cIFWs-0005e5-Fx for bug-gnu-emacs@gnu.org; Sat, 17 Dec 2016 09:00:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 17 Dec 2016 14:00:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 25178 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 25178-submit@debbugs.gnu.org id=B25178.148198318821655 (code B ref 25178); Sat, 17 Dec 2016 14:00:02 +0000 Original-Received: (at 25178) by debbugs.gnu.org; 17 Dec 2016 13:59:48 +0000 Original-Received: from localhost ([127.0.0.1]:44823 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cIFWd-0005dC-JL for submit@debbugs.gnu.org; Sat, 17 Dec 2016 08:59:47 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:42566) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cIFWb-0005cx-OG for 25178@debbugs.gnu.org; Sat, 17 Dec 2016 08:59:46 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cIFWT-0005XC-5q for 25178@debbugs.gnu.org; Sat, 17 Dec 2016 08:59:40 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:46424) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cIFWT-0005X4-1a; Sat, 17 Dec 2016 08:59:37 -0500 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:3671 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1cIFWS-0002my-Ax; Sat, 17 Dec 2016 08:59:36 -0500 In-reply-to: (message from Elias Martenson on Wed, 14 Dec 2016 11:09:12 +0800) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:127103 Archived-At: > From: Elias Martenson > CC: <25178@debbugs.gnu.org> > Date: Wed, 14 Dec 2016 11:09:12 +0800 > > Calling pthread_mutex_unlock() twice has to be undefined behaviour. In > fact, it can never work. Imagine what would happen if a different thread > called pthread_mutex_lock() on the mutex between two the two unlock > calls. In that case, you'd be unlocking a mutex help by a different > thread which is obviously very dangerous. Can you try the patch below and see if it stops the crashes? With this patch, I no longer see two calls to pthread_mutex_unlock in a row. Would people who know about signals and threads please eyeball this patch and comment on whether it is correct, safe, etc.? TIA. diff --git a/src/keyboard.c b/src/keyboard.c index 1fb1d49..f2ee313 100644 --- a/src/keyboard.c +++ b/src/keyboard.c @@ -2571,6 +2571,9 @@ read_char (int commandflag, Lisp_Object map, so restore it now. */ restore_getcjmp (save_jump); pthread_sigmask (SIG_SETMASK, &empty_mask, 0); +#if THREADS_ENABLED + maybe_reacquire_global_lock (); +#endif unbind_to (jmpcount, Qnil); XSETINT (c, quit_char); internal_last_event_frame = selected_frame; diff --git a/src/sysdep.c b/src/sysdep.c index 3d2b9bd..5e5a605 100644 --- a/src/sysdep.c +++ b/src/sysdep.c @@ -765,6 +765,23 @@ unblock_child_signal (sigset_t const *oldset) pthread_sigmask (SIG_SETMASK, oldset, 0); } +void +block_interrupt_signal (sigset_t *oldset) +{ + sigset_t blocked; + sigemptyset (&blocked); + sigaddset (&blocked, SIGINT); + pthread_sigmask (SIG_BLOCK, &blocked, oldset); +} + +/* Unblock SIGINT. */ + +void +unblock_interrupt_signal (sigset_t const *oldset) +{ + pthread_sigmask (SIG_SETMASK, oldset, 0); +} + #endif /* !MSDOS */ /* Saving and restoring the process group of Emacs's terminal. */ diff --git a/src/syssignal.h b/src/syssignal.h index 3de83c7..d3b585c 100644 --- a/src/syssignal.h +++ b/src/syssignal.h @@ -25,6 +25,8 @@ along with GNU Emacs. If not, see . */ extern void init_signals (bool); extern void block_child_signal (sigset_t *); extern void unblock_child_signal (sigset_t const *); +extern void block_interrupt_signal (sigset_t *); +extern void unblock_interrupt_signal (sigset_t const *); extern void block_tty_out_signal (sigset_t *); extern void unblock_tty_out_signal (sigset_t const *); diff --git a/src/thread.c b/src/thread.c index e8cb430..e519558 100644 --- a/src/thread.c +++ b/src/thread.c @@ -24,6 +24,7 @@ along with GNU Emacs. If not, see . */ #include "buffer.h" #include "process.h" #include "coding.h" +#include "syssignal.h" static struct thread_state primary_thread; @@ -100,6 +101,23 @@ acquire_global_lock (struct thread_state *self) post_acquire_global_lock (self); } +/* This is called from keyboard.c when it detects that SIGINT + interrupted thread_select before the current thread could acquire + the lock. We must acquire the lock to prevent a thread from + running without holding the global lock, and to avoid repeated + calls to sys_mutex_unlock, which invokes undefined behavior. */ +void +maybe_reacquire_global_lock (void) +{ + if (current_thread->not_holding_lock) + { + struct thread_state *self = current_thread; + + acquire_global_lock (self); + current_thread->not_holding_lock = 0; + } +} + static void @@ -493,11 +511,20 @@ really_call_select (void *arg) { struct select_args *sa = arg; struct thread_state *self = current_thread; + sigset_t oldset; + block_interrupt_signal (&oldset); + self->not_holding_lock = 1; release_global_lock (); + unblock_interrupt_signal (&oldset); + sa->result = (sa->func) (sa->max_fds, sa->rfds, sa->wfds, sa->efds, sa->timeout, sa->sigmask); + + block_interrupt_signal (&oldset); acquire_global_lock (self); + self->not_holding_lock = 0; + unblock_interrupt_signal (&oldset); } int diff --git a/src/thread.h b/src/thread.h index 739069a..b044383 100644 --- a/src/thread.h +++ b/src/thread.h @@ -171,6 +171,13 @@ struct thread_state interrupter should broadcast to this condition. */ sys_cond_t *wait_condvar; + /* This thread might have released the global lock. If so, this is + non-zero. When a thread runs outside thread_select with this + flag non-zero, it means it has been interrupted by SIGINT while + in thread_select, and didn't have a chance of acquiring the lock. + It must do so ASAP. */ + int not_holding_lock; + /* Threads are kept on a linked list. */ struct thread_state *next_thread; }; @@ -224,6 +231,7 @@ extern void unmark_threads (void); extern void finalize_one_thread (struct thread_state *state); extern void finalize_one_mutex (struct Lisp_Mutex *); extern void finalize_one_condvar (struct Lisp_CondVar *); +extern void maybe_reacquire_global_lock (void); extern void init_threads_once (void); extern void init_threads (void);