Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On Tue, 28 Mar 2023 16:19:42 +0200, Omar Polo wrote: > sigh... forgot to advance the pointer after strrchr otherwise argv[0] > would have been /ksh instead of "ksh". OK millert@ for this version. - todd
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On 2023/03/28 10:21:59 +0200, Omar Polo wrote:
> On 2023/03/27 18:58:07 -0600, Todd C. Miller wrote:
> > It might be best to use the basename of the actual shell for argv[0].
> > Our ksh for instance has slightly different behavior when invoked
> > as sh.
>
> like this? :)
>
> (need an up-to-date tree since it builds on the previous, committed,
> diff.)
>
> I've tested with ksh and csh. I guess it's fine to assume the user'
> shell has a -c flag. (vi does the same.)
>
>
> Thanks!
sigh... forgot to advance the pointer after strrchr otherwise argv[0]
would have been /ksh instead of "ksh".
Index: region.c
===
RCS file: /cvs/src/usr.bin/mg/region.c,v
retrieving revision 1.43
diff -u -p -r1.43 region.c
--- region.c28 Mar 2023 08:01:40 - 1.43
+++ region.c28 Mar 2023 14:13:26 -
@@ -34,7 +34,7 @@ staticint iomux(int, char * const, int,
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const, char * const, int);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -415,7 +415,6 @@ piperegion(int f, int n)
struct region region;
int len;
char *cmd, cmdbuf[NFILEN], *text;
- char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
/* C-u M-| is not supported yet */
if (n > 1)
@@ -431,8 +430,6 @@ piperegion(int f, int n)
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
return (ABORT);
- argv[2] = cmd;
-
if (getregion() != TRUE)
return (FALSE);
@@ -446,7 +443,7 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ return shellcmdoutput(cmd, text, len);
}
/*
@@ -456,7 +453,6 @@ int
shellcommand(int f, int n)
{
char *cmd, cmdbuf[NFILEN];
- char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
return (ABORT);
@@ -465,15 +461,14 @@ shellcommand(int f, int n)
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
return (ABORT);
- argv[2] = cmd;
-
- return shellcmdoutput(argv, NULL, 0);
+ return shellcmdoutput(cmd, NULL, 0);
}
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const cmd, char* const text, int len)
{
struct buffer *bp;
+ char*argv[] = {NULL, "-c", cmd, NULL};
char*shellp;
int ret;
@@ -486,6 +481,11 @@ shellcmdoutput(char* const argv[], char*
if ((shellp = getenv("SHELL")) == NULL)
shellp = _PATH_BSHELL;
+
+ if ((argv[0] = strrchr(shellp, '/')) != NULL)
+ argv[0]++;
+ else
+ argv[0] = shellp;
ret = pipeio(shellp, argv, text, len, bp);
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On Tue, 2023-03-28 at 11:22 +0200, Omar Polo wrote:
> On 2023/03/28 17:02:18 +0800, lux wrote:
> > On Mon, 2023-03-27 at 18:58 -0600, Todd C.Miller wrote:
> > >
> > > > - _exit(1);
> > > > - if (path == NULL)
> > > > _exit(1);
> > > >
> >
> > Hi, `pipeio' looks like a common function, so maby called in others
> > code, checking the path is NULL is a safe check, to prevent writing
> > wrong code, I think the condition that path is NULL should not be
> > removed.
>
> pipeio() is a common _internal_ function. There are requirements
> that
> callers need to fulfill when calling other functions. Otherwise
> you'd
> have to check also that argv is non-NULL and that it is NULL
> terminated, that len is non-negative, that text is a valid pointer if
> len is positive, that outbp is non-NULL and a valid pointer etc.
> Quite a few checks for a function only called twice and always with
> proper parameters :)
>
> % grep 'pipeio(' *.c
> buffer.c: ret = pipeio(DIFFTOOL, argv, text, len, bp);
> region.c: ret = pipeio(shellp, argv, text, len, bp);
> region.c:pipeio(const char* const path, char* const argv[],
>
> Furthermore, path is only looked at in the child process after
> fork(),
> even for the paranoids it won't cause issues in the editor itself.
>
> So I don't think we need to be pedantic and check the path there
> given
> that 1. it is always called with proper arguments and 2. there's no
> way it could do something useful with a NULL first argument.
>
> I should have added a note about this in the commit message.
> apologies.
>
Okay, I understand now, thank you :-)
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On 2023/03/28 17:02:18 +0800, lux wrote:
> On Mon, 2023-03-27 at 18:58 -0600, Todd C.Miller wrote:
> >
> > > - _exit(1);
> > > - if (path == NULL)
> > > _exit(1);
> > >
>
> Hi, `pipeio' looks like a common function, so maby called in others
> code, checking the path is NULL is a safe check, to prevent writing
> wrong code, I think the condition that path is NULL should not be
> removed.
pipeio() is a common _internal_ function. There are requirements that
callers need to fulfill when calling other functions. Otherwise you'd
have to check also that argv is non-NULL and that it is NULL
terminated, that len is non-negative, that text is a valid pointer if
len is positive, that outbp is non-NULL and a valid pointer etc.
Quite a few checks for a function only called twice and always with
proper parameters :)
% grep 'pipeio(' *.c
buffer.c: ret = pipeio(DIFFTOOL, argv, text, len, bp);
region.c: ret = pipeio(shellp, argv, text, len, bp);
region.c:pipeio(const char* const path, char* const argv[],
Furthermore, path is only looked at in the child process after fork(),
even for the paranoids it won't cause issues in the editor itself.
So I don't think we need to be pedantic and check the path there given
that 1. it is always called with proper arguments and 2. there's no
way it could do something useful with a NULL first argument.
I should have added a note about this in the commit message.
apologies.
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On Mon, 2023-03-27 at 18:58 -0600, Todd C.Miller wrote: > > > - _exit(1); > > - if (path == NULL) > > _exit(1); > > Hi, `pipeio' looks like a common function, so maby called in others code, checking the path is NULL is a safe check, to prevent writing wrong code, I think the condition that path is NULL should not be removed.
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On 2023/03/27 18:58:07 -0600, Todd C. Miller wrote:
> On Mon, 27 Mar 2023 20:06:30 +0200, Omar Polo wrote:
>
> > Is _PATH_BSHELL portable though? I can see a few stuff that uses it
> > (vi among others) but I'm not sure.
>
> The paths.h header is a BSD invention, though it may be present on
> some other systems. I don't think that's a reason not to use it
> though.
>
> > 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.
>
> It might be best to use the basename of the actual shell for argv[0].
> Our ksh for instance has slightly different behavior when invoked
> as sh.
like this? :)
(need an up-to-date tree since it builds on the previous, committed,
diff.)
I've tested with ksh and csh. I guess it's fine to assume the user'
shell has a -c flag. (vi does the same.)
Thanks!
Index: region.c
===
RCS file: /cvs/src/usr.bin/mg/region.c,v
retrieving revision 1.43
diff -u -p -r1.43 region.c
--- region.c28 Mar 2023 08:01:40 - 1.43
+++ region.c28 Mar 2023 08:17:30 -
@@ -34,7 +34,7 @@ staticint iomux(int, char * const, int,
static int preadin(int, struct buffer *);
static voidpwriteout(int, char **, int *);
static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const, char * const, int);
/*
* Kill the region. Ask "getregion" to figure out the bounds of the region.
@@ -415,7 +415,6 @@ piperegion(int f, int n)
struct region region;
int len;
char *cmd, cmdbuf[NFILEN], *text;
- char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
/* C-u M-| is not supported yet */
if (n > 1)
@@ -431,8 +430,6 @@ piperegion(int f, int n)
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
return (ABORT);
- argv[2] = cmd;
-
if (getregion() != TRUE)
return (FALSE);
@@ -446,7 +443,7 @@ piperegion(int f, int n)
region_get_data(, text, len);
- return shellcmdoutput(argv, text, len);
+ return shellcmdoutput(cmd, text, len);
}
/*
@@ -456,7 +453,6 @@ int
shellcommand(int f, int n)
{
char *cmd, cmdbuf[NFILEN];
- char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
if (n > 1)
return (ABORT);
@@ -465,16 +461,15 @@ shellcommand(int f, int n)
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
return (ABORT);
- argv[2] = cmd;
-
- return shellcmdoutput(argv, NULL, 0);
+ return shellcmdoutput(cmd, NULL, 0);
}
int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const cmd, char* const text, int len)
{
struct buffer *bp;
- char*shellp;
+ char*argv[] = {NULL, "-c", cmd, NULL};
+ char*shellp, *shell;
int ret;
bp = bfind("*Shell Command Output*", TRUE);
@@ -486,6 +481,10 @@ shellcmdoutput(char* const argv[], char*
if ((shellp = getenv("SHELL")) == NULL)
shellp = _PATH_BSHELL;
+
+ if ((shell = strrchr(shellp, '/')) == NULL)
+ shell = shellp;
+ argv[0] = shell;
ret = pipeio(shellp, argv, text, len, bp);
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On Mon, 2023-03-27 at 20:06 +0200, Omar Polo wrote: Hello, thank you for your correction. > > if (dup2(s[1], STDERR_FILENO) == -1) > - _exit(1); > - if (path == NULL) > _exit(1); But, I think the condition that path is NULL should not be removed, because `pipeio' looks like a common function, so maby called in others code, checking the path is NULL is a safe check.
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
On Mon, 27 Mar 2023 20:06:30 +0200, Omar Polo wrote:
> Is _PATH_BSHELL portable though? I can see a few stuff that uses it
> (vi among others) but I'm not sure.
The paths.h header is a BSD invention, though it may be present on
some other systems. I don't think that's a reason not to use it
though.
> 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.
It might be best to use the basename of the actual shell for argv[0].
Our ksh for instance has slightly different behavior when invoked
as sh.
OK millert@ for the diff as-is.
- todd
> 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 - 1.42
> +++ region.c 27 Mar 2023 17:58:11 -
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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);
>
Re: [patch] usr.bin/mg/region.c: Set default shell path if SHELL is NULL
Hello,
On 2023/03/28 00:53:05 +0800, lux 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 - 1.40
> +++ region.c 27 Mar 2023 16:48:08 -
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
>
> #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.c27 Mar 2023 17:54:20 - 1.42
+++ region.c27 Mar 2023 17:58:11 -
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -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);
