From: Eli Zaretskii <eliz@gnu.org>
To: Elias Martenson <elias.martenson@murex.com>
Cc: 25178@debbugs.gnu.org
Subject: bug#25178: 26.0.50; Crash when pressing C-g in TTY mode
Date: Sat, 17 Dec 2016 15:58:58 +0200 [thread overview]
Message-ID: <83eg167j31.fsf@gnu.org> (raw)
In-Reply-To: <yxd8oa0fmckn.fsf@murex.com> (message from Elias Martenson on Wed, 14 Dec 2016 11:09:12 +0800)
> From: Elias Martenson <elias.martenson@murex.com>
> 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 */
\f
/* 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 <http://www.gnu.org/licenses/>. */
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 <http://www.gnu.org/licenses/>. */
#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;
+ }
+}
+
\f
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);
next prev parent reply other threads:[~2016-12-17 13:58 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-12 4:33 bug#25178: 26.0.50; Crash when pressing C-g in TTY mode Elias Martenson
2016-12-12 16:56 ` Eli Zaretskii
2016-12-13 2:52 ` Elias Martenson
2016-12-13 3:07 ` Elias Martenson
2016-12-13 18:45 ` Eli Zaretskii
2016-12-13 19:26 ` Andreas Schwab
2016-12-13 19:37 ` Eli Zaretskii
2016-12-13 20:12 ` Andreas Schwab
2016-12-14 3:13 ` Elias Martenson
2016-12-14 3:09 ` Elias Martenson
2016-12-14 3:39 ` Eli Zaretskii
2016-12-14 5:41 ` Elias Martenson
2016-12-17 13:58 ` Eli Zaretskii [this message]
2016-12-19 2:48 ` Elias Martenson
[not found] ` <E1cJ1Z7-0000yc-Dd@eggs.gnu.org>
2017-01-05 23:39 ` npostavs
2017-01-06 7:47 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83eg167j31.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=25178@debbugs.gnu.org \
--cc=elias.martenson@murex.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.