From a8e92639af37ff1e289eb0c3db9589740a630f4a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 22 Feb 2013 16:22:56 -0800 Subject: [PATCH] Cleanup and simplify null_terminated_array_t and its clients --- common.cpp | 75 ++++++++++++++++++++++++++++++-- common.h | 120 +++++++++++---------------------------------------- env.cpp | 9 +--- env.h | 3 +- exec.cpp | 21 ++++----- postfork.cpp | 4 +- postfork.h | 2 +- 7 files changed, 113 insertions(+), 121 deletions(-) diff --git a/common.cpp b/common.cpp index 91646b8da..c2068b5c6 100644 --- a/common.cpp +++ b/common.cpp @@ -1978,18 +1978,21 @@ void exit_without_destructors(int code) } /* Helper function to convert from a null_terminated_array_t to a null_terminated_array_t */ -null_terminated_array_t convert_wide_array_to_narrow(const null_terminated_array_t &wide_arr) +void convert_wide_array_to_narrow(const null_terminated_array_t &wide_arr, null_terminated_array_t *output) { const wchar_t *const *arr = wide_arr.get(); if (! arr) - return null_terminated_array_t(); + { + output->clear(); + return; + } std::vector list; for (size_t i=0; arr[i]; i++) { list.push_back(wcs2string(arr[i])); } - return null_terminated_array_t(list); + output->set(list); } void append_path_component(wcstring &path, const wcstring &component) @@ -2167,3 +2170,69 @@ wcstokenizer::~wcstokenizer() { free(buffer); } + + +template +static CharType_t **make_null_terminated_array_helper(const std::vector > &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(malloc(pointers_allocation_len + strings_allocation_len)); + if (! base) return NULL; + + /* Divvy it up into the pointers and strings */ + CharType_t **pointers = reinterpret_cast(base); + CharType_t *strings = reinterpret_cast(base + pointers_allocation_len); + + /* Start copying */ + for (size_t i=0; i < count; i++) + { + const std::basic_string &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(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 &lst) +{ + return make_null_terminated_array_helper(lst); +} + diff --git a/common.h b/common.h index 080ff4294..f018dedf4 100644 --- a/common.h +++ b/common.h @@ -330,140 +330,70 @@ inline wcstring to_string(const int &x) return to_string(static_cast(x)); } +wchar_t **make_null_terminated_array(const wcstring_list_t &lst); +char **make_null_terminated_array(const std::vector &lst); /* Helper class for managing a null-terminated array of null-terminated strings (of some char type) */ template 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 string_t; - typedef std::vector string_list_t; - - void swap(null_terminated_array_t &him) - { - std::swap(array, him.array); - } - - /* Silly function to get the length of a null terminated array of...something */ - template - static size_t count_not_null(const T *arr) - { - size_t len; - for (len=0; arr[len] != T(0); len++) - ; - return len; - } + typedef std::vector > string_list_t; 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) - { - for (size_t i = 0; array[i] != NULL; i++) - { - delete [] array[i]; - } - delete [] array; - array = NULL; - } + ::free((void *)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(); - - /* 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; + this->array = make_null_terminated_array(argv); } - 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; - if (array != NULL) - { - size_t count = this->size(); - lst.reserve(count); - lst.insert(lst.end(), array, array + count); - } - return lst; + this->free(); } }; /* Helper function to convert from a null_terminated_array_t to a null_terminated_array_t */ -null_terminated_array_t convert_wide_array_to_narrow(const null_terminated_array_t &arr); +void convert_wide_array_to_narrow(const null_terminated_array_t &arr, null_terminated_array_t *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 diff --git a/env.cpp b/env.cpp index 3bb1e8dc6..5539218e0 100644 --- a/env.cpp +++ b/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 &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(); diff --git a/env.h b/env.h index b5d8023f4..d594ef6a6 100644 --- a/env.h +++ b/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); -void env_export_arr(bool recalc, null_terminated_array_t &result); +const char * const * env_export_arr(bool recalc); /** Returns all variable names. diff --git a/exec.cpp b/exec.cpp index e9135dba8..77c890bf0 100644 --- a/exec.cpp +++ b/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(cenvv); + char * const * argv = const_cast(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 argv_array = convert_wide_array_to_narrow(p->get_argv_array()); + null_terminated_array_t argv_array; + convert_wide_array_to_narrow(p->get_argv_array(), &argv_array); - null_terminated_array_t envv_array; - env_export_arr(false, envv_array); - - char **envv = envv_array.get(); - char **argv = argv_array.get(); + const char * const *argv = argv_array.get(); + const char * const *envv = env_export_arr(false); 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(argv), const_cast(envv)); /* This usleep can be used to test for various race conditions (https://github.com/fish-shell/fish-shell/issues/360) */ //usleep(10000); diff --git a/postfork.cpp b/postfork.cpp index a1d1a2527..6553104b3 100644 --- a/postfork.cpp +++ b/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; diff --git a/postfork.h b/postfork.h index d03e3f226..ba82918a4 100644 --- a/postfork.h +++ b/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 */