Re: [REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux
On Sun, Sep 06, 2020 at 07:34:57PM -0700, Bart Van Assche wrote: > On 2020-08-18 10:28, Magnus Fromreide wrote: > > I think the child code in the vfork case should be exactly > > > > execv(argv[0], argv); > > _exit(1); > > > > as that seems to be about what POSIX allows. > > > >(From POSIX.1) The vfork() function has the same effect as > > fork(2), > >except that the behavior is undefined if the process created by > > vfork() > >either > > modifies any data other than a variable of type pid_t used to > > store the return value from vfork(), or > > > > returns from the function in which vfork() was called, or > > > > calls any other function before successfully calling _exit(2) or > > one of the exec(3) family of functions. > > > > Yes, vfork is quite limited. > > Feel free to take a look at commit 8fabe044e316f9ecda53148d9afeaf5e5854bb1a. This looks nice, thanks. /MF ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux
On 2020-08-18 10:28, Magnus Fromreide wrote: > I think the child code in the vfork case should be exactly > > execv(argv[0], argv); > _exit(1); > > as that seems to be about what POSIX allows. > >(From POSIX.1) The vfork() function has the same effect as fork(2), >except that the behavior is undefined if the process created by vfork() >either > modifies any data other than a variable of type pid_t used to > store the return value from vfork(), or > >returns from the function in which vfork() was called, or > >calls any other function before successfully calling _exit(2) or >one of the exec(3) family of functions. > > Yes, vfork is quite limited. Feel free to take a look at commit 8fabe044e316f9ecda53148d9afeaf5e5854bb1a. Thanks, Bart. ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux
On Sun, Aug 16, 2020 at 06:15:17PM -0700, Bart Van Assche wrote:
> On 2020-08-16 15:58, Wes Hardaker wrote:
> > Magnus Fromreide writes:
> >
> >> That is a more lenient option than just dropping it so I think 'happy with
> >> dropping it' covers this variant. On the other hand I usually am far from
> >> the most conservative one in discussions like this one.
> >
> > I think off by default makes a lot more sense than an out-right removal.
>
> How about restoring uClinux support with the patch below? parse_cmd() and
> dup() are called before fork(). That change makes it possible to use a
> regular pipe between the parent and the child, even when using vfork(). The
> patch below has been tested on Linux (but not on uClinux).
Not like this, see below.
> Thanks,
>
> Bart.
>
>
> +}
> +NETSNMP_IGNORE_RESULT(dup2(STDOUT_FILENO, STDERR_FILENO));
> +#ifdef __uClinux__
> +if ((*pid = vfork()) == 0) {
> +#else
> +if ((*pid = fork()) == 0) {
> /*
> * close all non-standard open file descriptors
> */
> -netsnmp_close_fds(1);
> -NETSNMP_IGNORE_RESULT(dup(1)); /* stderr */
> +netsnmp_close_fds(STDOUT_FILENO);
> +#endif
Here I think
pid_t tmppid;
#if HAVE_FORK
tmppid = fork();
netsnmp_close_fds(STDOUT_FILENO);
#elif HAVE_VFORK
tmppid = vfork();
#else
#error "How to one spawn a process?"
#endif
is better since it removes the pointer access from the vfork child
>
> -argv = parse_cmd(, cmd);
> -if (!argv) {
> -DEBUGMSGTL(("util_funcs", "get_exec_pipes(): argv == NULL\n"));
> -return 0;
> -}
> -DEBUGMSGTL(("util_funcs", "get_exec_pipes(): argv[0] = %s\n",
> argv[0]));
> +/* Child process */
> execv(argv[0], argv);
> perror(argv[0]);
> free(argv);
> free(args);
These frees are a big no-no in the vfork child. Remember that the parent
and the child share all memory pages.
> -exit(1);
> +return 1;
Returning from the calling function is explicitly listed as forbidden in
the man page.
I think the child code in the vfork case should be exactly
execv(argv[0], argv);
_exit(1);
as that seems to be about what POSIX allows.
(From POSIX.1) The vfork() function has the same effect as fork(2),
except that the behavior is undefined if the process created by vfork()
either
modifies any data other than a variable of type pid_t used to
store the return value from vfork(), or
returns from the function in which vfork() was called, or
calls any other function before successfully calling _exit(2) or
one of the exec(3) family of functions.
Yes, vfork is quite limited.
/MF
___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux
On 2020-08-16 15:58, Wes Hardaker wrote:
> Magnus Fromreide writes:
>
>> That is a more lenient option than just dropping it so I think 'happy with
>> dropping it' covers this variant. On the other hand I usually am far from
>> the most conservative one in discussions like this one.
>
> I think off by default makes a lot more sense than an out-right removal.
How about restoring uClinux support with the patch below? parse_cmd() and
dup() are called before fork(). That change makes it possible to use a
regular pipe between the parent and the child, even when using vfork(). The
patch below has been tested on Linux (but not on uClinux).
Thanks,
Bart.
diff --git a/agent/mibgroup/util_funcs.c b/agent/mibgroup/util_funcs.c
index f39ea1adcd89..d42277b18f7e 100644
--- a/agent/mibgroup/util_funcs.c
+++ b/agent/mibgroup/util_funcs.c
@@ -458,53 +458,60 @@ int
get_exec_pipes(const char *cmd, int *fdIn, int *fdOut, netsnmp_pid_t *pid)
{
#if defined(HAVE_EXECV)
-int fd[2][2];
+int fd[2][2], stdin_fd, stdout_fd, stderr_fd;
char **argv, *args;
+argv = parse_cmd(, cmd);
+if (!argv) {
+DEBUGMSGTL(("util_funcs", "get_exec_pipes(): argv == NULL\n"));
+return 0;
+}
/*
- * Setup our pipes
+ * Setup pipes
*/
if (pipe(fd[0]) || pipe(fd[1])) {
setPerrorstatus("pipe");
return 0;
}
-if ((*pid = fork()) == 0) { /* First handle for the child */
-close(fd[0][1]);
-close(fd[1][0]);
-close(0);
-if (dup(fd[0][0]) != 0) {
-setPerrorstatus("dup 0");
-return 0;
-}
-close(fd[0][0]);
-close(1);
-if (dup(fd[1][1]) != 1) {
-setPerrorstatus("dup 1");
-return 0;
-}
-close(fd[1][1]);
-
-/*
- * write standard output and standard error to pipe.
- */
+/* Save stdin, stdout and stderr */
+stdin_fd = dup(STDIN_FILENO);
+stdout_fd = dup(STDOUT_FILENO);
+stderr_fd = dup(STDERR_FILENO);
+/* Redirect stdin, stdout and stderr to the child side of the pipe */
+if (dup2(fd[0][0], STDIN_FILENO) != 0) {
+setPerrorstatus("dup 0");
+return 0;
+}
+if (dup2(fd[1][1], STDOUT_FILENO) != 1) {
+setPerrorstatus("dup 1");
+return 0;
+}
+NETSNMP_IGNORE_RESULT(dup2(STDOUT_FILENO, STDERR_FILENO));
+#ifdef __uClinux__
+if ((*pid = vfork()) == 0) {
+#else
+if ((*pid = fork()) == 0) {
/*
* close all non-standard open file descriptors
*/
-netsnmp_close_fds(1);
-NETSNMP_IGNORE_RESULT(dup(1)); /* stderr */
+netsnmp_close_fds(STDOUT_FILENO);
+#endif
-argv = parse_cmd(, cmd);
-if (!argv) {
-DEBUGMSGTL(("util_funcs", "get_exec_pipes(): argv == NULL\n"));
-return 0;
-}
-DEBUGMSGTL(("util_funcs", "get_exec_pipes(): argv[0] = %s\n",
argv[0]));
+/* Child process */
execv(argv[0], argv);
perror(argv[0]);
free(argv);
free(args);
-exit(1);
+return 1;
} else {
+/* Parent process. */
+/* Restore stdin, stdout and stderr */
+dup2(stdin_fd, STDIN_FILENO);
+close(stdin_fd);
+dup2(stdout_fd, STDOUT_FILENO);
+close(stdout_fd);
+dup2(stderr_fd, STDERR_FILENO);
+close(stderr_fd);
close(fd[0][0]);
close(fd[1][1]);
if (*pid < 0) {
___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: [REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux
Magnus Fromreide writes: > That is a more lenient option than just dropping it so I think 'happy with > dropping it' covers this variant. On the other hand I usually am far from > the most conservative one in discussions like this one. I think off by default makes a lot more sense than an out-right removal. -- Wes Hardaker Please mail all replies to net-snmp-coders@lists.sourceforge.net ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
