On 2/15/23 18:04, Richard W.M. Jones wrote:
> On Wed, Feb 15, 2023 at 03:11:51PM +0100, Laszlo Ersek wrote:
>> It's bad hygiene to just assume dup2(), close() and signal() will succeed.
>> Check all return values.
>>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> Notes:
>> context:-U10
>>
>> generator/states-connect.c | 22 +++++++++++++++-----
>> 1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/generator/states-connect.c b/generator/states-connect.c
>> index e08608336fd6..d2476dc11c60 100644
>> --- a/generator/states-connect.c
>> +++ b/generator/states-connect.c
>> @@ -262,29 +262,41 @@ CONNECT_COMMAND.START:
>>
>> pid = fork ();
>> if (pid == -1) {
>> SET_NEXT_STATE (%.DEAD);
>> set_error (errno, "fork");
>> close (sv[0]);
>> close (sv[1]);
>> return 0;
>> }
>> if (pid == 0) { /* child - run command */
>> - close (sv[0]);
>> - dup2 (sv[1], STDIN_FILENO);
>> - dup2 (sv[1], STDOUT_FILENO);
>> + if (close (sv[0]) == -1) {
>> + nbd_internal_fork_safe_perror ("close");
>> + _exit (126);
>> + }
>> + if (dup2 (sv[1], STDIN_FILENO) == -1 ||
>> + dup2 (sv[1], STDOUT_FILENO) == -1) {
>> + nbd_internal_fork_safe_perror ("dup2");
>> + _exit (126);
>> + }
>> NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
>> NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
>> - close (sv[1]);
>> + if (close (sv[1]) == -1) {
>> + nbd_internal_fork_safe_perror ("close");
>> + _exit (126);
>> + }
>>
>> /* Restore SIGPIPE back to SIG_DFL. */
>> - signal (SIGPIPE, SIG_DFL);
>> + if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
>> + nbd_internal_fork_safe_perror ("signal");
>> + _exit (126);
>> + }
>>
>> execvp (h->argv.ptr[0], h->argv.ptr);
>> nbd_internal_fork_safe_perror (h->argv.ptr[0]);
>> if (errno == ENOENT)
>> _exit (127);
>> else
>> _exit (126);
>> }
>>
>> /* Parent. */
>
> For use of exit code 126, see:
>
> https://listman.redhat.com/archives/libguestfs/2019-September/022737.html
> https://listman.redhat.com/archives/libguestfs/2019-September/022739.html
> https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html
Yes, this is also from POSIX.
In fact, I sought to design the execvpe() replacement -- the _init API
and the actual execution API -- such that ENOENT here would continue to
stand for what it used to stand for before.
But, for this particular patch, the following argument is more important
than the above one:
I added _exit (126) on all these new error branches for *consistency*
with existent code. Namely, in the socket activation counterpart patch
(#14), the *pre-patch* state is that the child does nicely catch the
fcntl() failures -- and exits with status 126. So that's the primary
reason why I used _exit (126) on the new error branches in both patch
#14 and here (#22).
>
> Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
Thanks!
Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
