[prev in list] [next in list] [prev in thread] [next in thread] 

List:       net-snmp-coders
Subject:    Re: [REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux
From:       Magnus Fromreide <magfr () lysator ! liu ! se>
Date:       2020-08-18 17:28:10
Message-ID: 20200818172809.GA3616 () fukushima ! lysator ! liu ! se
[Download RAW message or body]

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 <magfr@lysator.liu.se> 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(&args, 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
[prev in list] [next in list] [prev in thread] [next in thread]