Sent from my iPhone
> On May 12, 2019, at 04:58, Nadav Har'El <[email protected]> wrote: > > >> On Wed, May 1, 2019 at 6:49 AM Waldemar Kozaczuk <[email protected]> >> wrote: >> This patch enhances implementation of getopt() and >> getopt_long() functions to work with PIEs. >> >> Newer GCC compiler optimizes PIEs by emitting machine code >> with so called copy relocations and affects how >> caller (PIEs) and callee (OSv kernel) see same logical >> global variables like optarg. >> >> New implementation of getopt() and getopt_long() >> detects if caller uses its (other) copies of global >> variables like optind and copies its value >> to the kernel copy. Likewise on return from >> the getopt() function it copies values of all "output" global >> variables from kernel to the equivalent caller copies of >> those variables. >> >> Fixes #689 >> >> Signed-off-by: Waldemar Kozaczuk <[email protected]> >> --- >> core/elf.cc | 14 +++ >> include/osv/elf.hh | 4 + >> libc/misc/getopt.cc | 62 +++++++++-- >> libc/misc/getopt_long.cc | 37 ++++++- >> modules/tests/Makefile | 9 +- >> tests/tst-getopt.cc | 232 +++++++++++++++++++++++++++++++++++++++ >> 6 files changed, 345 insertions(+), 13 deletions(-) >> create mode 100644 tests/tst-getopt.cc >> >> diff --git a/core/elf.cc b/core/elf.cc >> index ca9226f6..84e6fca3 100644 >> --- a/core/elf.cc >> +++ b/core/elf.cc >> @@ -147,6 +147,20 @@ void* object::lookup(const char* symbol) >> return sm.relocated_addr(); >> } >> >> +void* object::cached_lookup(const char* name) >> +{ >> + auto name_str = std::string(name); > > Nitpick: this means that on each called to cached_lookup(), even successful > ones, we need to > allocate this copy of name. It's not the end of the world (getopt() is > definitely not a performance > sensitive function) but if, instead, you had > > void* object::cached_lookup(const std::string& name) > > the extra copy would have been avoided. The caller who always calls this with > a constant string, > could do something like > > static constexpr std::string optarg_str("optarg"); > ... > cached_lookup(optarg_str); Good idea. I was also considering guarding the cached_lookup implementation with a thread lock just in case in future we use it for other reason than getopt or maybe what if we have to apps starting and suing getopt. But this we would not support anyway as there is only one global variable copy on the kernel size. Plus I do not think it is a issue given OSv is primarily unikernel and running two or more executables is not really supported because of the limitations like this. > > >> + auto cached_address_it = _cached_symbols.find(name_str); >> + if (cached_address_it != _cached_symbols.end()) { >> + return cached_address_it->second; >> + } >> + else { >> + void *symbol_address = lookup(name); >> + _cached_symbols[name_str] = symbol_address; >> + return symbol_address; >> + } >> +} >> + >> std::vector<Elf64_Shdr> object::sections() >> { >> size_t bytes = size_t(_ehdr.e_shentsize) * _ehdr.e_shnum; >> diff --git a/include/osv/elf.hh b/include/osv/elf.hh >> index 0f1792c7..86ac1cf6 100644 >> --- a/include/osv/elf.hh >> +++ b/include/osv/elf.hh >> @@ -14,6 +14,7 @@ >> #include <stack> >> #include <memory> >> #include <unordered_set> >> +#include <unordered_map> >> #include <osv/types.h> >> #include <atomic> >> >> @@ -349,6 +350,7 @@ public: >> void run_fini_funcs(); >> template <typename T = void> >> T* lookup(const char* name); >> + void* cached_lookup(const char* name); >> dladdr_info lookup_addr(const void* addr); >> bool contains_addr(const void* addr); >> ulong module_index() const; >> @@ -415,6 +417,8 @@ protected: >> bool _is_executable; >> bool is_core(); >> >> + std::unordered_map<std::string,void*> _cached_symbols; >> + >> // Keep list of references to other modules, to prevent them from being >> // unloaded. When this object is unloaded, the reference count of all >> // objects listed here goes down, and they too may be unloaded. >> diff --git a/libc/misc/getopt.cc b/libc/misc/getopt.cc >> index 70a11414..a9a749ec 100644 >> --- a/libc/misc/getopt.cc >> +++ b/libc/misc/getopt.cc >> @@ -4,6 +4,8 @@ >> #include <limits.h> >> #include <stdlib.h> >> #include <libc/libc.hh> >> +#include <osv/sched.hh> >> +#include <osv/app.hh> >> >> extern "C" { >> >> @@ -13,6 +15,30 @@ int optind=1, opterr=1, optopt, __optpos, __optreset=0; >> #define optpos __optpos >> weak_alias(__optreset, optreset); >> >> +#define COPY_GLOBALS_AND_RETURN(retval) { \ >> + if (other_optarg) { \ >> + *other_optarg = optarg; \ >> + } \ >> + if (other_optind) { \ >> + *other_optind = optind; \ >> + }\ >> + if (other_optopt) { \ >> + *other_optopt = optopt; \ >> + }\ >> + return retval; \ >> +} > > This is a reasonable approach, but I saw you need to use this macro in a lot > of places. Which is kind > of sad because first of all you need to not forget it anywhere, and second - > if we ever want to upgrade > musl we'll need to fix this again in all the same places. > > One way C++ can help you avoid the need to remember to do things every time > you need to return > from a function is to use RAII: a local object which automatically does > something when destructed. > > Here is an example of something you can do instead of your macros, with a lot > less changes to the > insides of the existing functions - basically just one line of change (rename > getop() to static _getopt). > Please consider the following idea (but if you prefer yours, feel free to say > so): > > > // This is a simple RAII class for retreiving the caller's copy of the global > opt* variables > // on initialization, and returning them back to the caller on destruction. > class caller_optvars { > std::shared_ptr<osv::application_runtime> _runtime; > caller_optvars() : _runtime(sched::thread::current()->app_runtime()) { > if (_runtime) { > auto obj = _runtime->app.lib(); > // Note that only optind and opterr are input variables. > optind = *reinterpret_cast<int*>(obj->cached_lookup("optind")); > opterr = *reinterpret_cast<int*>(obj->cached_lookup("opterr")); > > } > } > ~caller_optvars() { > if (_runtime) { > auto obj = _runtime->app.lib(); > // Note that only optind, optarg and optopt are output > variables. > // Note: Only "optind" is both input and output. We could save > the result of its lookup in this > // class. But not worth the effort. > *reinterpret_cast<int*>(obj->cached_lookup("optind")) = optind; > *reinterpret_cast<int*>(obj->cached_lookup("optopt")) = optopt; > > *reinterpret_cast<char**>(obj->cached_lookup("optarg")) = > optarg; > } > } > } > > int getopt(int argc, char * const argv[], const char *optstring) { > caller_optvars guard; > return _getopt(argc, argv, optstring); > } > > You could use the same RAII class for getopt_long() although I saw that you > noticed that getopt_long() > uses slightly different variables (doesn't use opterr and optind) so it can > use a different class. > Your approach is way more fancy than mine but I definitely like yours much better :-) it is always nice to learn from the examples like this. I think the assignments in both constructor and destructor need to be more defensive and check if symbol copy of optind or other actually exists. > + >> +// As explained in >> http://www.shrubbery.net/solaris9ab/SUNWdev/LLM/p22.html#CHAPTER4-84604 >> +// newer versions of gcc produce position independent executables with >> copies of >> +// some global variables like those used by getopt() and getopt_long() for >> optimizations reason. >> +// In those circumstances the caller of these functions uses different >> copies of >> +// global variables (like optind) than the getopt() code that is part of >> OSv kernel. >> +// For that reason in the beginning of the function we need to copy values >> of the caller >> +// copies of those variables to the kernel placeholders. Likewise on every >> from the function >> +// we need to copy the values of kernel copies of global variables to the >> caller ones. >> +// >> +// See http://man7.org/linux/man-pages/man3/getopt.3.html >> +// >> int getopt(int argc, char * const argv[], const char *optstring) >> { >> int i; >> @@ -20,6 +46,26 @@ int getopt(int argc, char * const argv[], const char >> *optstring) >> int k, l; >> char *optchar; >> >> + char **other_optarg = nullptr; >> + int* other_optind = nullptr, *other_optopt = nullptr, *other_opterr >> = nullptr; >> + >> + auto __runtime = sched::thread::current()->app_runtime(); >> + if (__runtime) { >> + auto obj = __runtime->app.lib(); >> + other_optarg = >> reinterpret_cast<char**>(obj->cached_lookup("optarg")); >> + other_optind = >> reinterpret_cast<int*>(obj->cached_lookup("optind")); >> + other_optopt = >> reinterpret_cast<int*>(obj->cached_lookup("optopt")); >> + other_opterr = >> reinterpret_cast<int*>(obj->cached_lookup("opterr")); >> + } >> + >> + // Copy values of caller copies >> + if (other_opterr) { >> + opterr = *other_opterr; >> + } >> + if (other_optind) { >> + optind = *other_optind; >> + } >> + >> if (!optind || __optreset) { >> __optreset = 0; >> __optpos = 0; >> @@ -27,9 +73,11 @@ int getopt(int argc, char * const argv[], const char >> *optstring) >> } >> >> if (optind >= argc || !argv[optind] || argv[optind][0] != '-' || >> !argv[optind][1]) >> - return -1; >> - if (argv[optind][1] == '-' && !argv[optind][2]) >> - return optind++, -1; >> + COPY_GLOBALS_AND_RETURN(-1) >> + if (argv[optind][1] == '-' && !argv[optind][2]) { >> + optind++; >> + COPY_GLOBALS_AND_RETURN(-1) >> + } >> >> if (!optpos) optpos++; >> if ((k = mbtowc(&c, argv[optind]+optpos, MB_LEN_MAX)) < 0) { >> @@ -54,23 +102,23 @@ int getopt(int argc, char * const argv[], const char >> *optstring) >> write(2, optchar, k); >> write(2, "\n", 1); >> } >> - return '?'; >> + COPY_GLOBALS_AND_RETURN('?') >> } >> if (optstring[i+1] == ':') { >> if (optind >= argc) { >> - if (optstring[0] == ':') return ':'; >> + if (optstring[0] == ':') COPY_GLOBALS_AND_RETURN(':') >> if (opterr) { >> write(2, argv[0], strlen(argv[0])); >> write(2, ": option requires an argument: ", >> 31); >> write(2, optchar, k); >> write(2, "\n", 1); >> } >> - return '?'; >> + COPY_GLOBALS_AND_RETURN('?') >> } >> optarg = argv[optind++] + optpos; >> optpos = 0; >> } >> - return c; >> + COPY_GLOBALS_AND_RETURN(c) >> } >> >> weak_alias(getopt, __posix_getopt); >> diff --git a/libc/misc/getopt_long.cc b/libc/misc/getopt_long.cc >> index fbde89fc..184d22fe 100644 >> --- a/libc/misc/getopt_long.cc >> +++ b/libc/misc/getopt_long.cc >> @@ -1,6 +1,18 @@ >> #include <stddef.h> >> #include <getopt.h> >> #include <stdio.h> >> +#include <osv/sched.hh> >> +#include <osv/app.hh> >> + >> +#define COPY_GLOBALS_AND_RETURN(retval) { \ >> + if (other_optarg) { \ >> + *other_optarg = optarg; \ >> + } \ >> + if (other_optind) { \ >> + *other_optind = optind; \ >> + } \ >> + return retval; \ >> +} >> >> extern "C" { >> >> @@ -8,12 +20,27 @@ extern int __optpos, __optreset; >> >> static int __getopt_long(int argc, char *const *argv, const char >> *optstring, const struct option *longopts, int *idx, int longonly) >> { >> + char **other_optarg = nullptr; >> + int* other_optind = nullptr; >> + >> + auto __runtime = sched::thread::current()->app_runtime(); >> + if (__runtime) { >> + auto obj = __runtime->app.lib(); >> + other_optarg = >> reinterpret_cast<char**>(obj->cached_lookup("optarg")); >> + other_optind = >> reinterpret_cast<int*>(obj->cached_lookup("optind")); >> + } >> + >> + // Copy value of caller copy >> + if (other_optind) { >> + optind = *other_optind; >> + } >> + >> if (!optind || __optreset) { >> __optreset = 0; >> __optpos = 0; >> optind = 1; >> } >> - if (optind >= argc || !argv[optind] || argv[optind][0] != '-') >> return -1; >> + if (optind >= argc || !argv[optind] || argv[optind][0] != '-') >> COPY_GLOBALS_AND_RETURN(-1) >> if ((longonly && argv[optind][1]) || >> (argv[optind][1] == '-' && argv[optind][2])) >> { >> @@ -30,20 +57,20 @@ static int __getopt_long(int argc, char *const *argv, >> const char *optstring, con >> } else { >> if (longopts[i].has_arg == >> required_argument) { >> if (!(optarg = argv[++optind])) >> - return ':'; >> + COPY_GLOBALS_AND_RETURN(':') >> } else optarg = NULL; >> } >> optind++; >> if (idx) *idx = i; >> if (longopts[i].flag) { >> *longopts[i].flag = longopts[i].val; >> - return 0; >> + COPY_GLOBALS_AND_RETURN(0) >> } >> - return longopts[i].val; >> + COPY_GLOBALS_AND_RETURN(longopts[i].val) >> } >> if (argv[optind][1] == '-') { >> optind++; >> - return '?'; >> + COPY_GLOBALS_AND_RETURN('?') >> } >> } >> return getopt(argc, argv, optstring); >> diff --git a/modules/tests/Makefile b/modules/tests/Makefile >> index 5c94e549..b0bfb031 100644 >> --- a/modules/tests/Makefile >> +++ b/modules/tests/Makefile >> @@ -62,6 +62,13 @@ $(out)/%.so: $(out)/%.o >> >> $(out)/tests/tst-non-fpic.o: CXXFLAGS:=$(subst >> -fPIC,-mcmodel=large,$(CXXFLAGS)) >> >> +$(out)/tests/tst-getopt-pie.o: CXXFLAGS:=$(subst -fPIC,-fpie,$(CXXFLAGS)) >> +$(out)/tests/tst-getopt-pie.o: $(src)/tests/tst-getopt.cc >> + $(makedir) >> + $(call quiet, $(CXX) $(CXXFLAGS) -c -o $@ $<, CXX $*.cc) >> +$(out)/tests/tst-getopt-pie.so: $(out)/tests/tst-getopt-pie.o >> + $(call quiet, $(CXX) $(CXXFLAGS) -pie -o $@ $< $(LIBS), LD $*.so) >> + >> # The rofs test image mounts /tmp as ramfs and 4 tests that exercise file >> system >> # fail due to some unresolved bugs or other shortcomings of the ramfs >> implementation >> # and are temporarily removed from the rofs-only-tests list. The tests >> tst-readdir.so >> @@ -117,7 +124,7 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so >> tst-bsd-evh.so \ >> tst-ttyname.so tst-pthread-barrier.so tst-feexcept.so tst-math.so \ >> tst-sigaltstack.so tst-fread.so tst-tcp-cork.so tst-tcp-v6.so \ >> tst-calloc.so tst-crypt.so tst-non-fpic.so tst-small-malloc.so \ >> - tst-mmx-fpu.so >> + tst-mmx-fpu.so tst-getopt.so tst-getopt-pie.so >> # libstatic-thread-variable.so tst-static-thread-variable.so \ >> >> tests += testrunner.so >> diff --git a/tests/tst-getopt.cc b/tests/tst-getopt.cc >> new file mode 100644 >> index 00000000..7e70f754 >> --- /dev/null >> +++ b/tests/tst-getopt.cc >> @@ -0,0 +1,232 @@ >> +/* >> + * Copyright (C) 2019 Waldek Kozaczuk >> + * >> + * This work is open source software, licensed under the terms of the >> + * BSD license as described in the LICENSE file in the top-level directory. >> + * >> + * This source code is loosely based on the examples from GNU libs manuals >> + * >> https://www.gnu.org/software/libc/manual/html_node/Getopt-Long-Option-Example.html >> + * and >> https://www.gnu.org/software/libc/manual/html_node/Example-of-Getopt.html. >> + */ >> + >> +#include <getopt.h> >> +#include <ctype.h> >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include <unistd.h> >> +#include <cassert> >> +#include <cstring> >> + >> +void >> +test_getopt(int argc, char *const argv[], int expected_aflag, int >> expected_bflag, >> + const char *expected_cvalue, const char >> *expected_non_option_arg) { >> + >> + int aflag = 0; >> + int bflag = 0; >> + char *cvalue = NULL; >> + int c; >> + >> + printf("Running test: %s ...\n", argv[0]); >> + >> + opterr = 0; >> + optind = 0; >> + >> + while ((c = getopt(argc, argv, "abc:")) != -1) >> + switch (c) { >> + case 'a': >> + aflag = 1; >> + break; >> + case 'b': >> + bflag = 1; >> + break; >> + case 'c': >> + cvalue = optarg; >> + break; >> + case '?': >> + if (optopt == 'c') >> + fprintf(stderr, "Option -%c requires an argument.\n", >> optopt); >> + else if (isprint (optopt)) >> + fprintf(stderr, "Unknown option `-%c'.\n", optopt); >> + else >> + fprintf(stderr, >> + "Unknown option character `\\x%x'.\n", >> + optopt); >> + assert(0); >> + default: >> + assert(0); >> + } >> + >> + assert(expected_aflag == aflag); >> + assert(expected_bflag == bflag); >> + if (expected_cvalue && cvalue) { >> + assert(strcmp(expected_cvalue, cvalue) == 0); >> + } else { >> + assert(!cvalue && !expected_cvalue); >> + } >> + >> + if (expected_non_option_arg) { >> + assert(optind + 1 == argc); >> + assert(strcmp(expected_non_option_arg, argv[optind]) == 0); >> + } else { >> + assert(optind == argc); >> + } >> +} >> + >> +static int verbose_flag; >> + >> +void >> +test_getopt_long(int argc, char *const argv[], int expected_aflag, int >> expected_bflag, const char *expected_cvalue, >> + const char *expected_dvalue, const char *expected_fvalue, >> int expected_verbose) { >> + int c; >> + int aflag = 0; >> + int bflag = 0; >> + char *cvalue = NULL; >> + char *dvalue = NULL; >> + char *fvalue = NULL; >> + >> + printf("Running test: %s ...\n", argv[0]); >> + >> + opterr = 0; >> + optind = 0; >> + >> + while (1) { >> + struct option long_options[] = >> + { >> + /* These options set a flag. */ >> + {"verbose", no_argument, &verbose_flag, 1}, >> + {"brief", no_argument, &verbose_flag, 0}, >> + /* These options don’t set a flag. >> + We distinguish them by their indices. */ >> + {"add", no_argument, 0, 'a'}, >> + {"append", no_argument, 0, 'b'}, >> + {"delete", required_argument, 0, 'd'}, >> + {"create", required_argument, 0, 'c'}, >> + {"file", required_argument, 0, 'f'}, >> + {0, 0, 0, 0} >> + }; >> + /* getopt_long stores the option index here. */ >> + int option_index = 0; >> + >> + c = getopt_long(argc, argv, "abc:d:f:", >> + long_options, &option_index); >> + >> + /* Detect the end of the options. */ >> + if (c == -1) >> + break; >> + >> + switch (c) { >> + case 0: >> + /* If this option set a flag, do nothing else now. */ >> + if (long_options[option_index].flag != 0) >> + break; >> + printf("option %s", long_options[option_index].name); >> + if (optarg) >> + printf(" with arg %s", optarg); >> + printf("\n"); >> + break; >> + >> + case 'a': >> + aflag = 1; >> + break; >> + >> + case 'b': >> + bflag = 1; >> + break; >> + >> + case 'c': >> + printf("option -c with value `%s'\n", optarg); >> + cvalue = optarg; >> + break; >> + >> + case 'd': >> + printf("option -d with value `%s'\n", optarg); >> + dvalue = optarg; >> + break; >> + >> + case 'f': >> + printf("option -f with value `%s'\n", optarg); >> + fvalue = optarg; >> + break; >> + >> + case '?': >> + /* getopt_long already printed an error message. */ >> + assert(0); >> + >> + default: >> + assert(0); >> + } >> + } >> + >> + assert(expected_aflag == aflag); >> + assert(expected_bflag == bflag); >> + >> + if (expected_cvalue && cvalue) { >> + assert(strcmp(expected_cvalue, cvalue) == 0); >> + } else { >> + assert(!cvalue && !expected_cvalue); >> + } >> + >> + if (expected_dvalue && dvalue) { >> + assert(strcmp(expected_dvalue, dvalue) == 0); >> + } else { >> + assert(!dvalue && !expected_dvalue); >> + } >> + >> + if (expected_fvalue && fvalue) { >> + assert(strcmp(expected_fvalue, fvalue) == 0); >> + } else { >> + assert(!fvalue && !expected_fvalue); >> + } >> + >> + assert(verbose_flag == expected_verbose); >> + >> + if (optind < argc) { >> + printf("non-option ARGV-elements: "); >> + while (optind < argc) >> + printf("%s ", argv[optind++]); >> + putchar('\n'); >> + } >> +} >> + >> +int >> +main(int argc, char *argv[]) { >> + char *const test1[] = {(char *) "tst-getopt1", nullptr}; >> + test_getopt(1, test1, 0, 0, nullptr, nullptr); >> + >> + char *const test2[] = {(char *) "tst-getopt2", (char *) "-a", (char *) >> "-b", nullptr}; >> + test_getopt(3, test2, 1, 1, nullptr, nullptr); >> + >> + char *const test3[] = {(char *) "tst-getopt3", (char *) "-ab", nullptr}; >> + test_getopt(2, test3, 1, 1, nullptr, nullptr); >> + >> + char *const test4[] = {(char *) "tst-getopt4", (char *) "-c", (char *) >> "foo", nullptr}; >> + test_getopt(3, test4, 0, 0, "foo", nullptr); >> + >> + char *const test5[] = {(char *) "tst-getopt5", (char *) "-cfoo", >> nullptr}; >> + test_getopt(2, test5, 0, 0, "foo", nullptr); >> + >> + char *const test6[] = {(char *) "tst-getopt6", (char *) "arg1", >> nullptr}; >> + test_getopt(2, test6, 0, 0, nullptr, "arg1"); >> + >> + char *const test7[] = {(char *) "tst-getopt7", (char *) "-a", (char *) >> "arg1", nullptr}; >> + test_getopt(3, test7, 1, 0, nullptr, "arg1"); >> + >> + char *const test8[] = {(char *) "tst-getopt8", (char *) "-c", (char *) >> "foo", (char *) "arg1", nullptr}; >> + test_getopt(4, test8, 0, 0, "foo", "arg1"); >> + >> + char *const test9[] = {(char *) "tst-getopt9", (char *) "-a", (char *) >> "--", (char *) "-b", nullptr}; >> + test_getopt(4, test9, 1, 0, nullptr, "-b"); >> + >> + char *const test10[] = {(char *) "tst-getopt10", (char *) "-a", (char >> *) "-", nullptr}; >> + test_getopt(3, test10, 1, 0, nullptr, "-"); >> + >> + char *const long_test1[] = {(char *) "tst-getopt_long1", (char *) "-a", >> (char *) "--create", (char *) "bula", >> + nullptr}; >> + test_getopt_long(4, long_test1, 1, 0, "bula", nullptr, nullptr, 0); >> + >> + char *const long_test2[] = {(char *) "tst-getopt_long2", (char *) "-a", >> (char *) "--create", (char *) "bula", >> + (char *) "--append", (char *) "-f", (char >> *) "x.txt", (char *) "--verbose", nullptr}; >> + test_getopt_long(8, long_test2, 1, 1, "bula", nullptr, "x.txt", 1); >> + >> + return 0; >> +} >> -- >> 2.20.1 >> >> -- >> You received this message because you are subscribed to the Google Groups >> "OSv Development" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/3135F08E-4BB0-4C20-AC3E-40B926C14655%40gmail.com. For more options, visit https://groups.google.com/d/optout.
