Re: [Libguestfs] [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()

On 2/15/23 23:28, Eric Blake wrote: Haha, yes :) Clearly I originally started the commit message with single-digit footnotes, but I ended up needing another digit :) Meh, those are the bane of my existence. :) Thanks for catching it! Thanks for the observation. I'll modify the commit message -- remove the note that this behavior is consistent with bash/Linux/glibc. I've re-checked [02], and it's implementation-defined what we do when PATH is set to the empty string. POSIX does not mandate (even as "legacy") the fallback to "." when PATH is empty. The code was extremely hard to review for myself (much harder than just writing it), so I'd *really* like to avoid "tweaking" it. I'd need to review the whole thing from the ground up, and I'm kind of "out of steam" for that, and this is such a corner case indeed that I don't think maintaining the above-noted consistency with linux/glibc/bash is worth repeating the whole effort. (Not to mention the unit tests in the next patch, which were super difficult to *write* as well.) Yes, I'll replace the FIXME with a NOTE -- that strictly per POSIX, we should lock the environment even for getenv(), but "glibc and any other high-quality libc will not be modifying environ during getenv()", and that no sane application should modify the environment after launching threads. If we ever get practical problems with PATH="", let's update the code then, incrementally. No, but it's not something that POSIX requires either. Certainly possible, but I don't think it's the job of this code to prevent that. I don't think we expect any libnbd application to run as a part of a setuid or setgid application (definitely not without the application properly separating privileges), so I think the user launching the libnbd application is responsible for setting their environment (including PATH) correctly. Also I think libnbd application writers are responsible for passing sane arguments to nbd_connect_systemd_socket_activation() and nbd_connect_command(). (To begin with -- if we such privilege escalations were relevant threats, we should be using the non-portable secure_getenv() already, not getenv()!) No, we don't do that. While chdir() is async-signal-safe, our two fork->exec paths have no reason for calling chdir(). Again, we don't call chdir() beween fork and exec, but even if we did -- from the user's perspective (and from the application's perspective that calls nbd_connect_systemd_socket_activation() or nbd_connect_command()), there's no difference between: - having a PATH with relative prefixes on it and changing the working directory *before* fork (either in the application or in libnbd), and - having a PATH with relative prefixes on it and changing the working directory in libnbd between fork and exec, for something the user might want. Having relative prefixes on PATH means that execvpe() will be cwd-sensitive one way or another. We have no reason for calling chdir() ourselves -- i.e., a libnbd-internal reason -- between fork and exec. The intent is to move the absolute minimum of work into the parent. Argv is indeed not meant to change between _init and execvpe, but that's just not enough reason for capturing argv in the context structure. The only piece of information needed for _init is the element count of argv, so that's what the function takes. If the contents of argv are modified afterwards (excluding resizing), things would work for now, but the intent is neither to prevent that practice programmatically nor to encourage it. I thought long and hard about asprintf(), and certainly expected this question :) My problem with asprintf (and to an extent with sprintf / snprintf) is that they return the number of bytes printed (or the number of bytes that would have been printed), without the terminating NUL, as a *signed int*. I can't fathom how that is an acceptable interface design when strlen() and sizeof both produce size_t, and when -- specifically regarding asprintf() -- malloc() takes a size_t. The glibc manual is super vague on this "int" overflow ("If memory allocation wasn't possible, or some other error occurs"): https://man7.org/linux/man-pages/man3/asprintf.3.html and I can't refer to the POSIX standard, because asprintf() is not POSIX. If I simply refer to fprintf() in C99 (which has similar return value semantics), I find (7.19.6.1p14-15): Returns The fprintf function returns the number of characters transmitted, or a negative value if an output or encoding error occurred. Environmental limits The number of characters that can be produced by any single conversion shall be at least 4095. Ugh... thanks? Sigh. If I refer to fprintf() in POSIX: https://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html it *does* say what I want to see: [EOVERFLOW] [CX] [Option Start] The value to be returned is greater than {INT_MAX}. [Option End] but it *does not* connect to asprintf(). Asprintf() is not in POSIX, and the EOVERFLOW language is not in the linux/glibc manual. (The linux/glibc manual on asprintf doesn't mention "errno" *at all*.) So, writing the code like this was actually not a huge problem (it's a relatively small complication compared to the rest), and it saved me an *assumption* about asprintf(). (And no, I didn't want to rely on the glibc source code happening to do what we want.) Well to be honest, I did this specifically because reference [02] on PATH states: When a non-zero-length prefix is applied to this filename, a <slash> shall be inserted between the prefix and the filename if the prefix did not end in <slash>. It happens to prevent "//binary" too, but that wasn't specifically my intent. How does "//binary" differ from "/binary", in pathname resolution? Yes, I'd agree about "int" being fine for "sep_copy_size" here; I just wanted to avoid asprintf altogether. (Once I do the checked additions up-front, asprintf() doesn't really save much.) (commenting under [3] below) Indeed. Again, the intent was to move the minimum amount of work required into the parent; in parallel, to move the minimum amount of state into the explicit "context" structure. Other than that, I'll make a comment of my own here. I could have used memcpy() in place of the loop, and I actually prototyped how that would look. (Memcpy() is async-signal-safe, and arguably faster on a high-quality libc than the manual loop.) However, for the last argument of memcpy(), I would have wanted to assert that the element size of "ctx->sh_argv" and that of argv->ptr were the same. Furthermore, I would have wanted to make that a static assertion. But a general static assertion would have been so much work (cf. STATIC_ASSERT_UNSIGNED_INT), in comparison to how few elements we tend to copy here, that I decided to go with the loop. The assignment operator in the loop is really nice because the compiler checks for element compatibility (i.e., it enforces that both arrays have (char*) elements). Thanks! I will update the commit message (fix the double space typo, and remove the allusion to bash compatibility near PATH=''), and replace the FIXME with a NOTE in get_path(). Thanks for the thorough review! Laszlo