From b2dd12daac73b00ca93107f33c9a7492807c57ea Mon Sep 17 00:00:00 2001 From: Robin Burchell Date: Thu, 16 May 2019 13:04:47 +0200 Subject: [PATCH] LibC: Change putenv (and getenv) to not copy, but directly return the environ values. This is in keeping with how putenv should function. It does mean that the shell's export command now leaks, but that's not a difficult fix. Contributes to #29. --- LibC/crt0.cpp | 4 +-- LibC/stdio.cpp | 2 ++ LibC/stdlib.cpp | 88 +++++++++++++++++++++++++------------------------ Shell/main.cpp | 12 +++++-- 4 files changed, 59 insertions(+), 47 deletions(-) diff --git a/LibC/crt0.cpp b/LibC/crt0.cpp index 7d43c391a9..c7bf35b2be 100644 --- a/LibC/crt0.cpp +++ b/LibC/crt0.cpp @@ -8,7 +8,7 @@ int main(int, char**); int errno; char** environ; -//bool __environ_is_malloced; +bool __environ_is_malloced; void __libc_init() { @@ -22,7 +22,7 @@ void __libc_init() int _start(int argc, char** argv, char** env) { environ = env; - //__environ_is_malloced = false; + __environ_is_malloced = false; __libc_init(); diff --git a/LibC/stdio.cpp b/LibC/stdio.cpp index 39692691ae..dbd52e4e05 100644 --- a/LibC/stdio.cpp +++ b/LibC/stdio.cpp @@ -293,6 +293,8 @@ void rewind(FILE* stream) int dbgprintf(const char* fmt, ...) { + // if this fails, you're printing too early. + ASSERT(stddbg); va_list ap; va_start(ap, fmt); int ret = vfprintf(stddbg, fmt, ap); diff --git a/LibC/stdlib.cpp b/LibC/stdlib.cpp index d85483ce94..2a7fe67e30 100644 --- a/LibC/stdlib.cpp +++ b/LibC/stdlib.cpp @@ -46,18 +46,17 @@ void abort() char* getenv(const char* name) { + size_t vl = strlen(name); for (size_t i = 0; environ[i]; ++i) { const char* decl = environ[i]; char* eq = strchr(decl, '='); if (!eq) continue; size_t varLength = eq - decl; - char* var = (char*)alloca(varLength + 1); - memcpy(var, decl, varLength); - var[varLength] = '\0'; - if (!strcmp(var, name)) { - char* value = eq + 1; - return value; + if (vl != varLength) + continue; + if (strncmp(decl, name, varLength) == 0) { + return eq + 1; } } return nullptr; @@ -65,48 +64,51 @@ char* getenv(const char* name) int putenv(char* new_var) { - HashMap environment; + char* new_eq = strchr(new_var, '='); - auto handle_environment_entry = [&environment] (const char* decl) { - char* eq = strchr(decl, '='); - if (!eq) - return; - size_t var_length = eq - decl; - char* var = (char*)alloca(var_length + 1); - memcpy(var, decl, var_length); - var[var_length] = '\0'; - const char* value = eq + 1; - environment.set(var, value); - }; - for (size_t i = 0; environ[i]; ++i) - handle_environment_entry(environ[i]); - handle_environment_entry(new_var); + // FIXME: should remove the var from the environment. + if (!new_eq) + return 0; - //extern bool __environ_is_malloced; - //if (__environ_is_malloced) - // free(environ); - //__environ_is_malloced = true; + auto new_var_len = new_eq - new_var; + size_t environ_size = 0; + for (; environ[environ_size]; ++environ_size) { + char* old_var = environ[environ_size]; + char* old_eq = strchr(old_var, '='); + ASSERT(old_eq); + auto old_var_len = old_eq - old_var; - int environment_size = sizeof(char*); // For the null sentinel. - for (auto& it : environment) - environment_size += (int)sizeof(char*) + it.key.length() + 1 + it.value.length() + 1; + if (new_var_len != old_var_len) + continue; // can't match - char* buffer = (char*)malloc(environment_size); - environ = (char**)buffer; - char* bufptr = buffer + sizeof(char*) * (environment.size() + 1); - - int i = 0; - for (auto& it : environment) { - environ[i] = bufptr; - memcpy(bufptr, it.key.characters(), it.key.length()); - bufptr += it.key.length(); - *(bufptr++) = '='; - memcpy(bufptr, it.value.characters(), it.value.length()); - bufptr += it.value.length(); - *(bufptr++) = '\0'; - ++i; + if (strncmp(new_var, old_var, new_var_len) == 0) { + environ[environ_size] = new_var; + return 0; + } } - environ[environment.size()] = nullptr; + + // At this point, we need to append the new var. + // 2 here: one for the new var, one for the sentinel value. + char **new_environ = (char**)malloc((environ_size + 2) * sizeof(char*)); + if (new_environ == nullptr) { + errno = ENOMEM; + return -1; + } + + for (size_t i = 0; environ[i]; ++i) { + new_environ[i] = environ[i]; + } + + new_environ[environ_size] = new_var; + new_environ[environ_size + 1] = nullptr; + + // swap new and old + // note that the initial environ is not heap allocated! + extern bool __environ_is_malloced; + if (__environ_is_malloced) + free(environ); + __environ_is_malloced = true; + environ = new_environ; return 0; } diff --git a/Shell/main.cpp b/Shell/main.cpp index a048a3c203..96f1eab7dc 100644 --- a/Shell/main.cpp +++ b/Shell/main.cpp @@ -70,7 +70,13 @@ static int sh_export(int argc, char** argv) fprintf(stderr, "usage: export variable=value\n"); return 1; } - putenv(const_cast(String::format("%s=%s", parts[0].characters(), parts[1].characters()).characters())); + + // FIXME: Yes, this leaks. + // Maybe LibCore should grow a CEnvironment which is secretly a map to char*, + // so it can keep track of the environment pointers as needed? + const auto& s = String::format("%s=%s", parts[0].characters(), parts[1].characters()); + char *ev = strndup(s.characters(), s.length()); + putenv(ev); return 0; } @@ -408,7 +414,9 @@ int main(int argc, char** argv) if (pw) { g.username = pw->pw_name; g.home = pw->pw_dir; - putenv(const_cast(String::format("HOME=%s", pw->pw_dir).characters())); + const auto& s = String::format("HOME=%s", pw->pw_dir); + char *ev = strndup(s.characters(), s.length()); + putenv(ev); } endpwent(); }