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

On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote: You KNOW this is going to be a juicy commit message when the first line contains a 2-digit footnote ;) Makes sense. A shame that it requires 3 functions, and that we have to duplicate so much work that libc would normally do for us if we don't care about async-safety, but I think the result is worth it. Accidental double space. Yeah - if you are actively changing out binaries on PATH at the same time as anything else running in earnest on a production system, you've got more issues than just libnbd's potential race. _Technically_, PATH= should behave identically to PATH=.; if you have an executable named './ls' in your working directory, bash WILL execute it! $ mkdir -p /tmp/blah $ cd /tmp/blah $ cp /bin/ls . $ PATH= ls -a . .. ls But whether you want to tweak this patch to actually comply with that corner case is up to you (no one in their right mind should set PATH to empty). Below, I'll refer to this as [1]. Very useful bibliography. The interface looks good for now (if you can't tell, I'm doing a stream-of-consciousness review, and may have further comments about the interface once I see the implementation and/or usage). Are the FIXME comments still necessary? I know we discussed about getenv() being technically non-async-safe in POSIX, but that in all practical aspects, we consider that a multi-threaded app should be smart enough to never modify the environment in parallel with calling into a library like libnbd. At best, you could reword the comment to get rid of FIXME and instead just state our reasoning for why we don't bother with locking the environment: glibc and any other high-quality libc will not be modifying environ during getenv(). Clever, and definitely worth that comment. Nice. [1] This might need tweaking if you treat empty PATH or _CS_PATH as equivalent to "." instead of an unconditional ENOENT. [1] And this simplifies if empty PATH can imply searching in ".". Are we guaranteed that if we get to this fallback, pathnames[i] does not begin with '-' or '+'? Or is it possible to write PATH=/usr/bin:+my/evil/relative/path:... and/or the user to attempt to execute '+my/evil/name' which skips the PATH search because the binary name contains '/', such that the resulting argument to /bin/sh might need to be protected with an extra "--" argument to avoid being misinterpreted as options to the shell rather than the script name to be executed? Is the current working directory something we change between fork() and exec()? If so, should we canonicalize the entries in pathnames[] to always start with '/', so that the child process can reposition directories at will without changing the meaning of the pre-scanned candidates? (Note that canonicalizing names would automatically resolve my above question about safety in the presence of an unusual PATH or binary name beginning with [-+]). [2] Should the interface take the proposed argv[] here pre-fork(), or are we trying to keep this interface generic enough where the child may alter some of the argv post-fork()? On the other hand, I already know we are altering the environment post-fork() (setting LISTEN_PID= cannot be done pre-fork, for starters), so keeping the selection of argv solely in the child rather than making the parent generate it pre-fork seems like the right division of labor. Another point where we may want consistency on whether we do space after cast. [1] Here is where you would code up an empty PATH as being equivalent to ".", if we think it's worth having, probably by just dropping this condition and letting the do-loop below handle it. Since we are in the parent process during this _init() function, can't we just use asprintf() instead of concatenating it all out by hand? [2] Here is where we would want to consider whether to (semi-)canonicalize any relative names into a corresponding absolute prefix, to avoid issues with a relative PATH element starting with [-+]. Clever way to avoid turning PATH=/:/usr/bin:... into an attempt to execute "//binary" which is NOT the same as "/binary". But also possible if you use asprintf(..., "...%.*s...", ..., (int) sep_copy_size, "/", ...) or declare sep_copy_size as int to begin with (because we know it will only be 0 or 1). [2] and here we'd have to canonicalize 'file' itself if we are worrying about the child changing directories and/or starting with a character that could change the behavior of /bin/sh if mistaken as an option instead of a script name. Should we prepopulate sh_argv[0] as a placeholder that can then be easily swapped to ctx->pathnames[i], where the child first attempts execve(pathnames[i], argv+1, env) with the ENOEXEC fallback to argv[0]=pathnames[i]; execve("/bin/sh", argv, env), rather than having to shuffle things in the child process? [3] [3] Oh, I see - the shuffle is still probably needed, because ctx->sh_argv is different than user's argv which does not have a pre-allocated filler slot that we can just swap out. Unless we decide that the _init() function should take argv itself rather than making the child do the work of shifting everything from one array to another, but then we are inconsistent for grabbing argv in the parent and env in the child. Overall looks like nice work. Based on my review, you may decide to make further tweaks, but I didn't see any hard reason why we couldn't use the patch as-written if you don't think my review warrants changes. Thus: Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org