handling when stty reports zero for termsize

If the kernel reports a size of zero for the rows or columns (i.e., what
`stty -a` reports) fall back to the `COLUMNS` and `LINES` variables. If
the resulting values are not reasonable fallback to using 80x24.

Fixes #3740

doc_src/index.hdr.in

@@ -849,12 +849,10 @@ The user can change the settings of `fish` by changing the values of certain var
 
 - `FISH_VERSION`, the version of the currently running fish
 
-- `COLUMNS`, the current width of the terminal
-
-- `LINES`, the current height of the terminal
-
 - `SHLVL`, the level of nesting of shells
 
+- `COLUMNS` and `LINES`, the current size of the terminal in height and width. These values are only used by fish if the operating system does not report the size of the terminal. Both variables must be set in that case otherwise a default of 80x24 will be used. They are updated when the window size changes.
+
 The names of these variables are mostly derived from the csh family of shells and differ from the ones used by Bourne style shells such as bash.
 
 Variables whose name are in uppercase are exported to the commands started by fish, while those in lowercase are not exported. This rule is not enforced by fish, but it is good coding practice to use casing to distinguish between exported and unexported variables. `fish` also uses several variables internally. Such variables are prefixed with the string `__FISH` or `__fish.` These should never be used by the user. Changing their value may break fish.

src/common.cpp

@@ -7,7 +7,6 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <limits.h>
-#include <signal.h>
 #include <stdarg.h>
 #include <stddef.h>
 #include <stdint.h>
@@ -33,8 +32,10 @@
 #include <memory>  // IWYU pragma: keep
 
 #include "common.h"
+#include "env.h"
 #include "expand.h"
 #include "fallback.h"  // IWYU pragma: keep
+#include "proc.h"
 #include "wildcard.h"
 #include "wutil.h"  // IWYU pragma: keep
 
@@ -62,9 +63,9 @@ static pid_t initial_fg_process_group = -1;
 /// 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 pthread_mutex_t termsize_lock = PTHREAD_MUTEX_INITIALIZER;
-static volatile bool termsize_valid;
+static struct winsize termsize = {USHRT_MAX, USHRT_MAX, USHRT_MAX, USHRT_MAX};
-static rwlock_t termsize_rwlock;
+static volatile bool termsize_valid = false;
 
 static char *wcs2str_internal(const wchar_t *in, char *out);
 static void debug_shared(const wchar_t msg_level, const wcstring &msg);
@@ -1356,32 +1357,97 @@ bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t e
     return success;
 }
 
-void common_handle_winch(int signal) {
+/// Used to invalidate our idea of having a valid window size. This can occur when either the
-    // Don't run ioctl() here, it's not safe to use in signals.
+/// COLUMNS or LINES variables are changed. This is also invoked when the shell regains control of
-    UNUSED(signal);
+/// the tty since it is possible the terminal size changed while an external command was running.
+void invalidate_termsize(bool invalidate_vars) {
     termsize_valid = false;
+    if (invalidate_vars) {
+        termsize.ws_col = termsize.ws_row = USHRT_MAX;
+    }
+}
+
+/// Handle SIGWINCH. This is also invoked when the shell regains control of the tty since it is
+/// possible the terminal size changed while an external command was running.
+void common_handle_winch(int signal) {
+    // Don't run ioctl() here. Technically it's not safe to use in signals although in practice it
+    // is safe on every platform I've used. But we want to be conservative on such matters.
+    UNUSED(signal);
+    invalidate_termsize(false);
+}
+
+/// Validate the new terminal size. Fallback to the env vars if necessary. Ensure the values are
+/// sane and if not fallback to a default of 80x24.
+static void validate_new_termsize(struct winsize *new_termsize) {
+    if (new_termsize->ws_col == 0 || new_termsize->ws_row == 0) {
+#ifdef HAVE_WINSIZE
+        if (shell_is_interactive()) {
+            debug(1, _(L"Current terminal parameters have rows and/or columns set to zero."));
+            debug(1, _(L"The stty command can be used to correct this "
+                       L"(e.g., stty rows 80 columns 24)."));
+        }
+#endif
+        // Fallback to the environment vars.
+        env_var_t col_var = env_get_string(L"COLUMNS");
+        env_var_t row_var = env_get_string(L"LINES");
+        if (!col_var.missing_or_empty() && !row_var.missing_or_empty()) {
+            // Both vars have to have valid values.
+            int col = fish_wcstoi(col_var.c_str());
+            bool col_ok = errno == 0 && col > 0 && col <= USHRT_MAX;
+            int row = fish_wcstoi(row_var.c_str());
+            bool row_ok = errno == 0 && row > 0 && row <= USHRT_MAX;
+            if (col_ok && row_ok) {
+                new_termsize->ws_col = col;
+                new_termsize->ws_row = row;
+            }
+        }
+    }
+
+    if (new_termsize->ws_col < MIN_TERM_COL || new_termsize->ws_row < MIN_TERM_ROW) {
+        if (shell_is_interactive()) {
+            debug(1, _(L"Current terminal parameters set terminal size to unreasonable value."));
+            debug(1, _(L"Defaulting terminal size to 80x24."));
+        }
+        new_termsize->ws_col = DFLT_TERM_COL;
+        new_termsize->ws_row = DFLT_TERM_ROW;
+    }
+}
+
+/// Export the new terminal size as env vars and to the kernel if possible.
+static void export_new_termsize(struct winsize *new_termsize) {
+    wchar_t buf[64];
+    swprintf(buf, 64, L"%d", (int)new_termsize->ws_col);
+    env_set(L"COLUMNS", buf, ENV_EXPORT | ENV_GLOBAL);
+    swprintf(buf, 64, L"%d", (int)new_termsize->ws_row);
+    env_set(L"LINES", buf, ENV_EXPORT | ENV_GLOBAL);
+
+#ifdef HAVE_WINSIZE
+    ioctl(STDOUT_FILENO, TIOCSWINSZ, new_termsize);
+#endif
 }
 
 /// Updates termsize as needed, and returns a copy of the winsize.
-static struct winsize get_current_winsize() {
+struct winsize get_current_winsize() {
-#ifndef HAVE_WINSIZE
+    scoped_lock guard(termsize_lock);
-    struct winsize retval = {0};
+
-    retval.ws_col = 80;
+    if (termsize_valid) return termsize;
-    retval.ws_row = 24;
+
-    return retval;
+    struct winsize new_termsize = {0, 0, 0, 0};
-#endif
+#ifdef HAVE_WINSIZE
-    scoped_rwlock guard(termsize_rwlock, true);
+    errno = 0;
-    struct winsize retval = termsize;
+    if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &new_termsize) != -1 &&
-    if (!termsize_valid) {
+        new_termsize.ws_col == termsize.ws_col && new_termsize.ws_row == termsize.ws_row) {
-        struct winsize size;
-        if (ioctl(1, TIOCGWINSZ, &size) == 0) {
-            retval = size;
-            guard.upgrade();
-            termsize = retval;
-        }
         termsize_valid = true;
+        return termsize;
     }
-    return retval;
+#endif
+
+    validate_new_termsize(&new_termsize);
+    export_new_termsize(&new_termsize);
+    termsize.ws_col = new_termsize.ws_col;
+    termsize.ws_row = new_termsize.ws_row;
+    termsize_valid = true;
+    return termsize;
 }
 
 int common_get_width() { return get_current_winsize().ws_col; }

src/common.h

@@ -845,6 +845,15 @@ using std::wcscasecmp;
 using std::wcsncasecmp;
 #endif
 
-#endif
+void redirect_tty_output();
 
-void redirect_tty_output(void);
+// Minimum allowed terminal size and default size if the detected size is not reasonable.
+#define MIN_TERM_COL 20
+#define MIN_TERM_ROW 2
+#define DFLT_TERM_COL 80
+#define DFLT_TERM_ROW 24
+#define DFLT_TERM_COL_STR L"80"
+#define DFLT_TERM_ROW_STR L"24"
+void invalidate_termsize(bool invalidate_vars = false);
+struct winsize get_current_winsize();
+#endif

src/env.cpp

@@ -315,6 +315,8 @@ static void react_to_variable_change(const wcstring &key) {
         reader_react_to_color_change();
     } else if (key == L"fish_escape_delay_ms") {
         update_wait_on_escape_ms();
+    } else if (key == L"LINES" || key == L"COLUMNS") {
+        invalidate_termsize(true);  // force fish to update its idea of the terminal size plus vars
     }
 }
 
@@ -357,15 +359,25 @@ static void setup_path() {
     }
 }
 
-int env_set_pwd() {
+/// Initialize the `COLUMNS` and `LINES` env vars if they don't already exist to reasonable
+/// defaults. They will be updated later by the `get_current_winsize()` function if they need to be
+/// adjusted.
+static void env_set_termsize() {
+    env_var_t cols = env_get_string(L"COLUMNS");
+    if (cols.missing_or_empty()) env_set(L"COLUMNS", DFLT_TERM_COL_STR, ENV_EXPORT | ENV_GLOBAL);
+    env_var_t rows = env_get_string(L"LINES");
+    if (rows.missing_or_empty()) env_set(L"LINES", DFLT_TERM_ROW_STR, ENV_EXPORT | ENV_GLOBAL);
+}
+
+bool env_set_pwd() {
     wcstring res = wgetcwd();
     if (res.empty()) {
         debug(0,
               _(L"Could not determine current working directory. Is your locale set correctly?"));
-        return 0;
+        return false;
     }
     env_set(L"PWD", res.c_str(), ENV_EXPORT | ENV_GLOBAL);
-    return 1;
+    return true;
 }
 
 wcstring env_get_pwd_slash(void) {
@@ -400,7 +412,7 @@ static void setup_user(bool force) {
 void env_init(const struct config_paths_t *paths /* or NULL */) {
     // These variables can not be altered directly by the user.
     const wchar_t *const ro_keys[] = {
-        L"status", L"history", L"_", L"LINES", L"COLUMNS", L"PWD", L"FISH_VERSION",
+        L"status", L"history", L"_", L"PWD", L"FISH_VERSION",
         // L"SHLVL" is readonly but will be inserted below after we increment it.
     };
     for (size_t i = 0; i < sizeof ro_keys / sizeof *ro_keys; i++) {
@@ -411,8 +423,6 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
     env_electric.insert(L"history");
     env_electric.insert(L"status");
     env_electric.insert(L"umask");
-    env_electric.insert(L"COLUMNS");
-    env_electric.insert(L"LINES");
 
     top = new env_node_t;
     global_env = top;
@@ -496,8 +506,8 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
         free(unam_narrow);
     }
 
-    // Set PWD.
+    env_set_pwd();       // initialize the PWD variable
-    env_set_pwd();
+    env_set_termsize();  // initialize the terminal size variables
 
     // Set up universal variables. The empty string means to use the deafult path.
     assert(s_universal_variables == NULL);
@@ -815,10 +825,6 @@ env_var_t env_get_string(const wcstring &key, env_mode_flags_t mode) {
             }
             if (history) history->get_string_representation(&result, ARRAY_SEP_STR);
             return result;
-        } else if (key == L"COLUMNS") {
-            return to_string(common_get_width());
-        } else if (key == L"LINES") {
-            return to_string(common_get_height());
         } else if (key == L"status") {
             return to_string(proc_get_last_status());
         } else if (key == L"umask") {

src/env.h

@@ -144,7 +144,7 @@ void env_set_argv(const wchar_t *const *argv);
 wcstring_list_t env_get_names(int flags);
 
 /// Update the PWD variable directory.
-int env_set_pwd();
+bool env_set_pwd();
 
 /// Returns the PWD with a terminating slash.
 wcstring env_get_pwd_slash();

src/reader.cpp

@@ -357,7 +357,7 @@ static void term_steal() {
             break;
     }
 
-    common_handle_winch(0);
+    invalidate_termsize();
 }
 
 int reader_exit_forced() { return exit_forced; }
@@ -1644,7 +1644,7 @@ static void reader_interactive_init() {
         exit_without_destructors(1);
     }
 
-    common_handle_winch(0);
+    invalidate_termsize();
 
     // Set the new modes.
     if (tcsetattr(0, TCSANOW, &shell_modes) == -1) {