Cleanup and simplify null_terminated_array_t and its clients

common.cpp

@@ -1978,18 +1978,21 @@ void exit_without_destructors(int code)
 }
 
 /* Helper function to convert from a null_terminated_array_t<wchar_t> to a null_terminated_array_t<char_t> */
-null_terminated_array_t<char> convert_wide_array_to_narrow(const null_terminated_array_t<wchar_t> &wide_arr)
+void convert_wide_array_to_narrow(const null_terminated_array_t<wchar_t> &wide_arr, null_terminated_array_t<char> *output)
 {
     const wchar_t *const *arr = wide_arr.get();
     if (! arr)
-        return null_terminated_array_t<char>();
+    {
+        output->clear();
+        return;
+    }
 
     std::vector<std::string> list;
     for (size_t i=0; arr[i]; i++)
     {
         list.push_back(wcs2string(arr[i]));
     }
-    return null_terminated_array_t<char>(list);
+    output->set(list);
 }
 
 void append_path_component(wcstring &path, const wcstring &component)
@@ -2167,3 +2170,69 @@ wcstokenizer::~wcstokenizer()
 {
     free(buffer);
 }
+
+
+template <typename CharType_t>
+static CharType_t **make_null_terminated_array_helper(const std::vector<std::basic_string<CharType_t> > &argv)
+{
+    size_t count = argv.size();
+    
+    /* We allocate everything in one giant block. First compute how much space we need. */
+
+    /* N + 1 pointers */
+    size_t pointers_allocation_len = (count + 1) * sizeof(CharType_t *);
+    
+    /* In the very unlikely event that CharType_t has stricter alignment requirements than does a pointer, round us up to the size of a CharType_t */
+    pointers_allocation_len += sizeof(CharType_t) - 1;
+    pointers_allocation_len -= pointers_allocation_len % sizeof(CharType_t);
+    
+    /* N null terminated strings */
+    size_t strings_allocation_len = 0;
+    for (size_t i=0; i < count; i++)
+    {
+        /* The size of the string, plus a null terminator */
+        strings_allocation_len += (argv.at(i).size() + 1) * sizeof(CharType_t);
+    }
+    
+    /* Now allocate their sum */
+    unsigned char *base = static_cast<unsigned char *>(malloc(pointers_allocation_len + strings_allocation_len));
+    if (! base) return NULL;
+    
+    /* Divvy it up into the pointers and strings */
+    CharType_t **pointers = reinterpret_cast<CharType_t **>(base);
+    CharType_t *strings = reinterpret_cast<CharType_t *>(base + pointers_allocation_len);
+    
+    /* Start copying */
+    for (size_t i=0; i < count; i++)
+    {
+        const std::basic_string<CharType_t> &str = argv.at(i);
+        // store the current string pointer into self
+        *pointers++ = strings;
+        
+        // copy the string into strings
+        strings = std::copy(str.begin(), str.end(), strings);
+        // each string needs a null terminator
+        *strings++ = (CharType_t)(0);
+    }
+    // array of pointers needs a null terminator
+    *pointers++ = NULL;
+    
+    // Make sure we know what we're doing
+    assert((unsigned char *)pointers - base == (ptrdiff_t)pointers_allocation_len);
+    assert((unsigned char *)strings - (unsigned char *)pointers == (ptrdiff_t)strings_allocation_len);
+    assert((unsigned char *)strings - base == (ptrdiff_t)(pointers_allocation_len + strings_allocation_len));
+    
+    // Return what we did
+    return reinterpret_cast<CharType_t**>(base);
+}
+
+wchar_t **make_null_terminated_array(const wcstring_list_t &lst)
+{
+    return make_null_terminated_array_helper(lst);
+}
+
+char **make_null_terminated_array(const std::vector<std::string> &lst)
+{
+    return make_null_terminated_array_helper(lst);
+}
+

common.h

@@ -330,140 +330,70 @@ inline wcstring to_string(const int &x)
     return to_string(static_cast<long>(x));
 }
 
+wchar_t **make_null_terminated_array(const wcstring_list_t &lst);
+char **make_null_terminated_array(const std::vector<std::string> &lst);
 
 /* Helper class for managing a null-terminated array of null-terminated strings (of some char type) */
 template <typename CharType_t>
 class null_terminated_array_t
 {
     CharType_t **array;
+    
+    /* No assignment or copying */
+    void operator=(null_terminated_array_t rhs);
+    null_terminated_array_t(const null_terminated_array_t &);
 
-    typedef std::basic_string<CharType_t> string_t;
+    typedef std::vector<std::basic_string<CharType_t> > string_list_t;
-    typedef std::vector<string_t> string_list_t;
-
-    void swap(null_terminated_array_t<CharType_t> &him)
-    {
-        std::swap(array, him.array);
-    }
-
-    /* Silly function to get the length of a null terminated array of...something */
-    template <typename T>
-    static size_t count_not_null(const T *arr)
-    {
-        size_t len;
-        for (len=0; arr[len] != T(0); len++)
-            ;
-        return len;
-    }
 
     size_t size() const
     {
-        return count_not_null(array);
+        size_t len = 0;
+        if (array != NULL)
+        {
+            while (array[len] != NULL)
+            {
+                len++;
+            }
+        }
+        return len;
     }
 
     void free(void)
     {
-        if (array != NULL)
+        ::free((void *)array);
-        {
+        array = NULL;
-            for (size_t i = 0; array[i] != NULL; i++)
-            {
-                delete [] array[i];
-            }
-            delete [] array;
-            array = NULL;
-        }
     }
 
 public:
     null_terminated_array_t() : array(NULL) { }
-    null_terminated_array_t(const string_list_t &argv) : array(NULL)
+    null_terminated_array_t(const string_list_t &argv) : array(make_null_terminated_array(argv))
     {
-        this->set(argv);
     }
+    
     ~null_terminated_array_t()
     {
         this->free();
     }
 
-    /** operator=. Notice the pass-by-value parameter. */
-    null_terminated_array_t& operator=(null_terminated_array_t rhs)
-    {
-        if (this != &rhs)
-            this->swap(rhs);
-        return *this;
-    }
-
-    /* Copy constructor. */
-    null_terminated_array_t(const null_terminated_array_t &him) : array(NULL)
-    {
-        this->set(him.array);
-    }
-
     void set(const string_list_t &argv)
     {
-        /* Get rid of the old argv */
         this->free();
-
+        this->array = make_null_terminated_array(argv);
-        /* Allocate our null-terminated array of null-terminated strings */
-        size_t i, count = argv.size();
-        this->array = new CharType_t * [count + 1];
-        for (i=0; i < count; i++)
-        {
-            const string_t &str = argv.at(i);
-            this->array[i] = new CharType_t [1 + str.size()];
-            std::copy(str.begin(), str.end(), this->array[i]);
-            this->array[i][str.size()] = CharType_t(0);
-        }
-        this->array[count] = NULL;
     }
 
-    void set(const CharType_t * const *new_array)
-    {
-        if (new_array == array)
-            return;
-
-        /* Get rid of the old argv */
-        this->free();
-
-        /* Copy the new one */
-        if (new_array)
-        {
-            size_t i, count = count_not_null(new_array);
-            this->array = new CharType_t * [count + 1];
-            for (i=0; i < count; i++)
-            {
-                size_t len = count_not_null(new_array[i]);
-                this->array[i] = new CharType_t [1 + len];
-                std::copy(new_array[i], new_array[i] + len, this->array[i]);
-                this->array[i][len] = CharType_t(0);
-            }
-            this->array[count] = NULL;
-        }
-    }
-
-    CharType_t **get()
-    {
-        return array;
-    }
     const CharType_t * const *get() const
     {
         return array;
     }
-
+    
-    string_list_t to_list() const
+    void clear()
     {
-        string_list_t lst;
+        this->free();
-        if (array != NULL)
-        {
-            size_t count = this->size();
-            lst.reserve(count);
-            lst.insert(lst.end(), array, array + count);
-        }
-        return lst;
     }
 };
 
 /* Helper function to convert from a null_terminated_array_t<wchar_t> to a null_terminated_array_t<char_t> */
-null_terminated_array_t<char> convert_wide_array_to_narrow(const null_terminated_array_t<wchar_t> &arr);
+void convert_wide_array_to_narrow(const null_terminated_array_t<wchar_t> &arr, null_terminated_array_t<char> *output);
 
 /* Helper class to cache a narrow version of a wcstring in a malloc'd buffer, so that we can read it after fork() */
 class narrow_string_rep_t

env.cpp

@@ -1478,20 +1478,13 @@ static void update_export_array_if_necessary(bool recalc)
 
 }
 
-char **env_export_arr(bool recalc)
+const char * const *env_export_arr(bool recalc)
 {
     ASSERT_IS_MAIN_THREAD();
     update_export_array_if_necessary(recalc);
     return export_array.get();
 }
 
-void env_export_arr(bool recalc, null_terminated_array_t<char> &output)
-{
-    ASSERT_IS_MAIN_THREAD();
-    update_export_array_if_necessary(recalc);
-    output = export_array;
-}
-
 env_vars_snapshot_t::env_vars_snapshot_t(const wchar_t * const *keys)
 {
     ASSERT_IS_MAIN_THREAD();

env.h

@@ -206,8 +206,7 @@ void env_push(bool new_scope);
 void env_pop();
 
 /** Returns an array containing all exported variables in a format suitable for execv. */
-char **env_export_arr(bool recalc);
+const char * const * env_export_arr(bool recalc);
-void env_export_arr(bool recalc, null_terminated_array_t<char> &result);
 
 /**
   Returns all variable names.

exec.cpp

@@ -271,13 +271,16 @@ char *get_interpreter(const char *command, char *interpreter, size_t buff_size)
    in \c p. It never returns.
 */
 /* Called in a forked child! Do not allocate memory, etc. */
-static void safe_launch_process(process_t *p, const char *actual_cmd, char **argv, char **envv)
+static void safe_launch_process(process_t *p, const char *actual_cmd, const char *const* cargv, const char *const *cenvv)
 {
     int err;
 
 //  debug( 1, L"exec '%ls'", p->argv[0] );
 
-    // Wow, this wcs2str call totally allocates memory
+    // This function never returns, so we take certain liberties with constness
+    char * const * envv = const_cast<char* const *>(cenvv);
+    char * const * argv = const_cast<char* const *>(cargv);
+
     execve(actual_cmd, argv, envv);
 
     err = errno;
@@ -326,7 +329,7 @@ static void launch_process_nofork(process_t *p)
     ASSERT_IS_NOT_FORKED_CHILD();
 
     char **argv = wcsv2strv(p->get_argv());
-    char **envv = env_export_arr(false);
+    const char *const *envv = env_export_arr(false);
     char *actual_cmd = wcs2str(p->actual_cmd.c_str());
 
     /* Bounce to launch_process. This never returns. */
@@ -1275,13 +1278,11 @@ void exec(parser_t &parser, job_t *j)
             case EXTERNAL:
             {
                 /* Get argv and envv before we fork */
-                null_terminated_array_t<char> argv_array = convert_wide_array_to_narrow(p->get_argv_array());
+                null_terminated_array_t<char> argv_array;
+                convert_wide_array_to_narrow(p->get_argv_array(), &argv_array);
 
-                null_terminated_array_t<char> envv_array;
+                const char * const *argv = argv_array.get();
-                env_export_arr(false, envv_array);
+                const char * const *envv = env_export_arr(false);
-
-                char **envv = envv_array.get();
-                char **argv = argv_array.get();
 
                 std::string actual_cmd_str = wcs2string(p->actual_cmd);
                 const char *actual_cmd = actual_cmd_str.c_str();
@@ -1309,7 +1310,7 @@ void exec(parser_t &parser, job_t *j)
                     if (made_it)
                     {
                         /* We successfully made the attributes and actions; actually call posix_spawn */
-                        int spawn_ret = posix_spawn(&pid, actual_cmd, &actions, &attr, argv, envv);
+                        int spawn_ret = posix_spawn(&pid, actual_cmd, &actions, &attr, const_cast<char * const *>(argv), const_cast<char * const *>(envv));
 
                         /* This usleep can be used to test for various race conditions (https://github.com/fish-shell/fish-shell/issues/360) */
                         //usleep(10000);

postfork.cpp

@@ -528,7 +528,7 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, posix_spawn_fil
 }
 #endif //FISH_USE_POSIX_SPAWN
 
-void safe_report_exec_error(int err, const char *actual_cmd, char **argv, char **envv)
+void safe_report_exec_error(int err, const char *actual_cmd, const char * const *argv, const char *const *envv)
 {
     debug_safe(0, "Failed to execute process '%s'. Reason:", actual_cmd);
 
@@ -542,7 +542,7 @@ void safe_report_exec_error(int err, const char *actual_cmd, char **argv, char *
             long arg_max = -1;
 
             size_t sz = 0;
-            char **p;
+            const char * const *p;
             for (p=argv; *p; p++)
             {
                 sz += strlen(*p)+1;

postfork.h

@@ -69,7 +69,7 @@ pid_t execute_fork(bool wait_for_threads_to_die);
 bool do_builtin_io(const char *out, size_t outlen, const char *err, size_t errlen);
 
 /** Report an error from failing to exec or posix_spawn a command */
-void safe_report_exec_error(int err, const char *actual_cmd, char **argv, char **envv);
+void safe_report_exec_error(int err, const char *actual_cmd, const char * const *argv, const char * const *envv);
 
 #if FISH_USE_POSIX_SPAWN
 /* Initializes and fills in a posix_spawnattr_t; on success, the caller should destroy it via posix_spawnattr_destroy */