Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL

[Omar Polo]

Hello,

On 2023/03/28 00:53:05 +0800, lux <l...@shellcodes.org> wrote:
> Index: region.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mg/region.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 region.c
> --- region.c  8 Mar 2023 04:43:11 -0000       1.40
> +++ region.c  27 Mar 2023 16:48:08 -0000
> @@ -21,6 +21,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <paths.h>
>  
>  #include "def.h"
>  
> @@ -486,9 +487,8 @@ shellcmdoutput(char* const argv[], char*
>               return (FALSE);
>       }
>  
> -     shellp = getenv("SHELL");
> -
> -     ret = pipeio(shellp, argv, text, len, bp);
> +     ret = pipeio((shellp = getenv("SHELL")) == NULL ? _PATH_BSHELL
> : shellp,
> +                  argv, text, len, bp);
>  
>       if (ret == TRUE) {
>               eerase();

Thanks for the diff!  I don't think it's a bad idea to fall back to
/bin/sh if $SHELL is (somehow) undefined, but I do have some nits on
the patch itself (which was mangled btw, make sure that your MUA won't
fold lines), but are mostly stylistics.

I'd avoid the ternary operator and choose another way to spell the
same thing that keeps the code more readable.  Also, since we're
always going to call pipeio() with a non-NULL path now there's a check
there that becomes unnecessary.  See patch below.

Is _PATH_BSHELL portable though?  I can see a few stuff that uses it
(vi among others) but I'm not sure.

Also, if you look at the callers of shellcmdoutput() they all prepare
the argv array as {"sh", "-c", "command provided", NULL} so I'm
wondering if we should just ignore $SHELL and always use /bin/sh, or
change that "sh" accordingly to $SHELL.

Index: region.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/region.c,v
retrieving revision 1.42
diff -u -p -r1.42 region.c
--- region.c    27 Mar 2023 17:54:20 -0000      1.42
+++ region.c    27 Mar 2023 17:58:11 -0000
@@ -15,6 +15,7 @@
 #include <sys/wait.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <paths.h>
 #include <poll.h>
 #include <signal.h>
 #include <stdio.h>
@@ -483,7 +484,8 @@ shellcmdoutput(char* const argv[], char*
                return (FALSE);
        }
 
-       shellp = getenv("SHELL");
+       if ((shellp = getenv("SHELL")) == NULL)
+               shellp = _PATH_BSHELL;
 
        ret = pipeio(shellp, argv, text, len, bp);
 
@@ -529,8 +531,6 @@ pipeio(const char* const path, char* con
                if (dup2(s[1], STDOUT_FILENO) == -1)
                        _exit(1);
                if (dup2(s[1], STDERR_FILENO) == -1)
-                       _exit(1);
-               if (path == NULL)
                        _exit(1);
 
                execv(path, argv);