Re: BUG: file descriptors leak when sys_pipe failed with -EFAULT

Re: BUG: file descriptors leak when sys_pipe failed with -EFAULT

[Andrew Morton]

On Thu, 13 Dec 2007 20:34:11 +0800
"Changli Gao" <xiaosuo@gmail.com> wrote:

> If an invalid address is passed to system call pipe as argument, file
> descriptors will leak.

Yup.  I added linux-kernel to cc.

> System call pipe is implemented as following on most architectures:
> 
>     int fd[2];
>     int error;
> 
>     error = do_pipe(fd);
>     if (!error) {
>         if (copy_to_user(fildes, fd, 2*sizeof(int)))
>             error = -EFAULT;
>     }
>     return error;
> 
> Invalid memory address makes copy_to_user failed. But the descriptors
> allocated for the pipe will be left open.
> A workaround fix will be like this:
> 
>     int fd[2];
>     int error;
> 
>     error = do_pipe(fd);
>     if (!error) {
>         if (copy_to_user(fildes, fd, 2*sizeof(int))) {
>             sys_close(fd[0]);
>             sys_close(fd[1]);
>             error = -EFAULT;
>         }
>     }
>     return error;
> 
> I don't understand the others architectures(such as
> sh/sh64/mips/sparc/sparc64)  which implement pipe in the other ways,
> so I just indicate this bug and provide my fixing way instead of
> patching it.

The consequences of this are that the application may eventually run out of
file descriptors and they will be cleaned up when the application exits
anyway, so it isn't terribly serious.

However it does seem fairly dumb of us to leave the fds open given that
at least one or possibly both of the file descriptors are unknown to the
application anyway.  Probably it'd be better to close them off immediately.

This would be an application-visible change: subsequent open()s will return
lower-numbered descriptors than they do at present.  That shouldn't matter.

Re: BUG: file descriptors leak when sys_pipe failed with -EFAULT

[Jens Axboe]

On Thu, Dec 13 2007, Andrew Morton wrote:
> On Thu, 13 Dec 2007 20:34:11 +0800
> "Changli Gao" <xiaosuo@gmail.com> wrote:
> 
> > If an invalid address is passed to system call pipe as argument, file
> > descriptors will leak.
> 
> Yup.  I added linux-kernel to cc.
> 
> > System call pipe is implemented as following on most architectures:
> > 
> >     int fd[2];
> >     int error;
> > 
> >     error = do_pipe(fd);
> >     if (!error) {
> >         if (copy_to_user(fildes, fd, 2*sizeof(int)))
> >             error = -EFAULT;
> >     }
> >     return error;
> > 
> > Invalid memory address makes copy_to_user failed. But the descriptors
> > allocated for the pipe will be left open.
> > A workaround fix will be like this:
> > 
> >     int fd[2];
> >     int error;
> > 
> >     error = do_pipe(fd);
> >     if (!error) {
> >         if (copy_to_user(fildes, fd, 2*sizeof(int))) {
> >             sys_close(fd[0]);
> >             sys_close(fd[1]);
> >             error = -EFAULT;
> >         }
> >     }
> >     return error;
> > 
> > I don't understand the others architectures(such as
> > sh/sh64/mips/sparc/sparc64)  which implement pipe in the other ways,
> > so I just indicate this bug and provide my fixing way instead of
> > patching it.
> 
> The consequences of this are that the application may eventually run out of
> file descriptors and they will be cleaned up when the application exits
> anyway, so it isn't terribly serious.
> 
> However it does seem fairly dumb of us to leave the fds open given that
> at least one or possibly both of the file descriptors are unknown to the
> application anyway.  Probably it'd be better to close them off immediately.

I agree with the solution, closing the descriptors that do_pipe() opened
is clearly the right thing to do.

> This would be an application-visible change: subsequent open()s will return
> lower-numbered descriptors than they do at present.  That shouldn't matter.

I don't think that is a concern in this case :)

-- 
Jens Axboe

Re: BUG: file descriptors leak when sys_pipe failed with -EFAULT

[Herbert Xu]

Andrew Morton <akpm@linux-foundation.org> wrote:
>
> The consequences of this are that the application may eventually run out of
> file descriptors and they will be cleaned up when the application exits
> anyway, so it isn't terribly serious.

If an application is calling pipe(2) with bogus pointers then
leaving open file descriptors behind is the least of its worries.
So I think fixing this is really a waste of time.  But whatever.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt