Rework how screen size is tracked

The screen size is fetched after a SIGWINCH is delivered. The current
implementation has two issues:

* It calls ioctl() from the SIGWINCH signal handler, despite ioctl() not
  being a function that is known to be safe to call.
* It's not thread-safe.

Signals can be delivered on arbitrary threads, so we don't know if it's
actually safe to be modifying the cached winsize in response to a
signal. It's also plausible that the winsize may be requested from a
background thread.

To solve the first issue, we twiddle a volatile boolean flag in the
signal handler and defer the ioctl() call until we actually request the
screen size.

To solve the second issue, we introduce a pthread rwlock around the
cached winsize. A rwlock is used because it can be expected that there
are likely to be far more window size reads than window size writes. If
we were using C++11 we could probably get away with atomics, but since
we don't have that (or boost), a rwlock should suffice.

Fixes #1613.

1 files changed, 111 insertions, 12 deletions

diff --git a/common.cpp b/common.cpp
index b8a2661d..ad35470e 100644
--- a/common.cpp
+++ b/common.cpp
@@ -90,9 +90,12 @@ const wchar_t *program_name;
 int debug_level=1;
 
 /**
-   This struct should be continually updated by signals as the term resizes, and as such always contain the correct current size.
+   This struct maintains the current state of the terminal size. It is updated on demand after receiving a SIGWINCH.
+   Do not touch this struct directly, it's managed with a rwlock. Use common_get_width()/common_get_height().
 */
 static struct winsize termsize;
+static volatile bool termsize_valid;
+static rwlock_t termsize_rwlock;
 
 static char *wcs2str_internal(const wchar_t *in, char *out);
 
@@ -1686,26 +1689,44 @@ bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t e
 
 void common_handle_winch(int signal)
 {
-#ifdef HAVE_WINSIZE
-    if (ioctl(1,TIOCGWINSZ,&termsize)!=0)
+    /* don't run ioctl() here, it's not safe to use in signals */
+    termsize_valid = false;
+}
+
+/* updates termsize as needed, and returns a copy of the winsize. */
+static struct winsize get_current_winsize()
+{
+#ifndef HAVE_WINSIZE
+    struct winsize retval = {0};
+    retval.ws_col = 80;
+    retval.ws_row = 24;
+    return retval;
+#endif
+    scoped_rwlock guard(termsize_rwlock, true);
+    struct winsize retval = termsize;
+    if (!termsize_valid)
     {
-        return;
+        struct winsize size;
+        if (ioctl(1,TIOCGWINSZ,&size) == 0)
+        {
+            retval = size;
+            guard.upgrade();
+            termsize = retval;
+        }
+        termsize_valid = true;
     }
-#else
-    termsize.ws_col = 80;
-    termsize.ws_row = 24;
-#endif
+    return retval;
 }
 
 int common_get_width()
 {
-    return termsize.ws_col;
+    return get_current_winsize().ws_col;
 }
 
 
 int common_get_height()
 {
-    return termsize.ws_row;
+    return get_current_winsize().ws_row;
 }
 
 void tokenize_variable_array(const wcstring &val, std::vector<wcstring> &out)
@@ -2223,7 +2244,7 @@ void scoped_lock::lock(void)
 {
     assert(! locked);
     assert(! is_forked_child());
-    VOMIT_ON_FAILURE(pthread_mutex_lock(lock_obj));
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_lock(lock_obj));
     locked = true;
 }
 
@@ -2231,7 +2252,7 @@ void scoped_lock::unlock(void)
 {
     assert(locked);
     assert(! is_forked_child());
-    VOMIT_ON_FAILURE(pthread_mutex_unlock(lock_obj));
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_unlock(lock_obj));
     locked = false;
 }
 
@@ -2250,6 +2271,84 @@ scoped_lock::~scoped_lock()
     if (locked) this->unlock();
 }
 
+void scoped_rwlock::lock(void)
+{
+    assert(! (locked || locked_shared));
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_rdlock(rwlock_obj));
+    locked = true;
+}
+
+void scoped_rwlock::unlock(void)
+{
+    assert(locked);
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj));
+    locked = false;
+}
+
+void scoped_rwlock::lock_shared(void)
+{
+    assert(! (locked || locked_shared));
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_wrlock(rwlock_obj));
+    locked_shared = true;
+}
+
+void scoped_rwlock::unlock_shared(void)
+{
+    assert(locked_shared);
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj));
+    locked_shared = false;
+}
+
+void scoped_rwlock::upgrade(void)
+{
+    assert(locked_shared);
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj));
+    locked = false;
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_wrlock(rwlock_obj));
+    locked_shared = true;
+}
+
+scoped_rwlock::scoped_rwlock(pthread_rwlock_t &rwlock, bool shared) : rwlock_obj(&rwlock), locked(false), locked_shared(false)
+{
+    if (shared)
+    {
+        this->lock_shared();
+    }
+    else
+    {
+        this->lock();
+    }
+}
+
+scoped_rwlock::scoped_rwlock(rwlock_t &rwlock, bool shared) : rwlock_obj(&rwlock.rwlock), locked(false), locked_shared(false)
+{
+    if (shared)
+    {
+        this->lock_shared();
+    }
+    else
+    {
+        this->lock();
+    }
+}
+
+scoped_rwlock::~scoped_rwlock()
+{
+    if (locked)
+    {
+        this->unlock();
+    }
+    else if (locked_shared)
+    {
+        this->unlock_shared();
+    }
+}
+
 wcstokenizer::wcstokenizer(const wcstring &s, const wcstring &separator) :
     buffer(),
     str(),