[BZ,#18433] Check file access/existence before forking.

  For second patch should I create another request for review or sending
it here is not problematic (since in new patch I am using pipes to
communicate between parent and child it is kinda v2 ) ?

best wishes,
-navid


On Fri, Sep 11, 2015 at 10:52 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> doing an access/stat/whatever doesn't solve the issue you described --
> as others have pointed out, there's a TOCTOU race, but there's also no
> guarantee that doing a stat/access on mode bits means you can actually
> execute that file and/or that the exec will succeed.  since this doesn't
> actually fix the problem you've described (and others have pointed out
> that it's not a spec violation), then this patch as-is is dead.
> -mike
  

On Sat, 12 Sep 2015, navid Rahimi wrote:

> +  /* Open Read/Write pipe for parent/child communication */
> +  if (__pipe2 (pipefd, O_CLOEXEC))
> +    return errno;

You can't assume that pipe2 is supported unless __ASSUME_PIPE2 is defined.  
It might be an ENOSYS stub, or it might be unsupported at runtime because 
the kernel isn't recent enough.  In either case, an ENOSYS error from 
pipe2 must not result in such an error from posix_spawn.
  

On Sat, Sep 12, 2015 at 12:28 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Sat, 12 Sep 2015, navid Rahimi wrote:
> You can't assume that pipe2 is supported unless __ASSUME_PIPE2 is defined.
> It might be an ENOSYS stub, or it might be unsupported at runtime because
> the kernel isn't recent enough.  In either case, an ENOSYS error from
> pipe2 must not result in such an error from posix_spawn.
> Joseph S. Myers
> joseph@codesourcery.com

Freebsd uses vfork (and its blocking nature for parent process) for
implementation of posix_spawn , but I have found cert article about
not using vfork

https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=1703954

pipes was more reliable in overall sight (in my opinion, because I
have seen so much people complaining about vfork) , but I can
implement another patch which fixes this bug with using vfork.
But I don't know should I use vfork ?
  

That article talks about the complications of using vfork with standard
library interfaces.  That is not directly relevant to using vfork in the
implementation of the library.  The vfork semantics raise subtle issues and
great care needs to be taken when using it, but the article you cited is
not really relevant to implementation choices here.
  

On Sat, 12 Sep 2015, navid Rahimi wrote:

> On Sat, Sep 12, 2015 at 12:28 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> > On Sat, 12 Sep 2015, navid Rahimi wrote:
> > You can't assume that pipe2 is supported unless __ASSUME_PIPE2 is defined.
> > It might be an ENOSYS stub, or it might be unsupported at runtime because
> > the kernel isn't recent enough.  In either case, an ENOSYS error from
> > pipe2 must not result in such an error from posix_spawn.
> > Joseph S. Myers
> > joseph@codesourcery.com
> 
> Freebsd uses vfork (and its blocking nature for parent process) for
> implementation of posix_spawn , but I have found cert article about
> not using vfork
> 
> https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=1703954
> 
> pipes was more reliable in overall sight (in my opinion, because I
> have seen so much people complaining about vfork) , but I can

Well, you can use pipes provided you have a fallback from pipe2 to pipe in 
the case where __ASSUME_PIPE2 is not defined and pipe2 fails with ENOSYS 
at runtime.  (There would be a race if another thread forks and execs 
between the calls to pipe and fcntl FD_CLOEXEC, but that's unavoidable in 
the absence of pipe2.)
  

In this case I think implementing with vfork would be more readable
and clearer than this tricky hack with pipes and this unavoidable race
condition in fallback mode is something we can avoid with vfork.

If you guys can give any advise I would be more than happy to hear. I
will sent patch for this bug (vfork version ).


I have hard time understanding this part of __spawni function :

  /* Generate the new process.  */
  if ((flags & POSIX_SPAWN_USEVFORK) != 0
      /* If no major work is done, allow using vfork.  Note that we
might perform the path searching.  But this would be done by
a call to execvp(), too, and such a call must be OK according
to POSIX.  */
      || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF
   | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER
   | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS)) == 0
 && file_actions == NULL))
    new_pid = __vfork ();
  else
    new_pid = __fork ();

Why it is trying to limit usage of vfork ?

best wishes,
-navid


On Sat, Sep 12, 2015 at 1:31 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> Well, you can use pipes provided you have a fallback from pipe2 to pipe in
> the case where __ASSUME_PIPE2 is not defined and pipe2 fails with ENOSYS
> at runtime.  (There would be a race if another thread forks and execs
> between the calls to pipe and fcntl FD_CLOEXEC, but that's unavoidable in
> the absence of pipe2.)
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
  

> Why it is trying to limit usage of vfork ?

I think that's just being conservative.  For each modification that can be
applied in the child, you need to examine it carefully and figure out
whether it is in fact (or can be made to be) compatible with vfork.
  

On 12 Sep 2015 00:16, navid Rahimi wrote:
> +  /* Open Read/Write pipe for parent/child communication */

GNU style says comments are full sentences (so ends in a period) and
there should be two spaces after that.
  /* Open read/write pipe for parent/child communication.  */
-mike