linux-kernel - Re: [PATCH] open: introduce O

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 28 Aug 2009 06:45:33 -0600
From:	Eric Blake <ebb9@....net>
To:	Florian Weimer <fweimer@....de>
CC:	Davide Libenzi <davidel@...ilserver.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	bug-coreutils@....org, bug-gnulib@....org,
	Ulrich Drepper <drepper@...hat.com>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH] open: introduce O_NOSTD

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Florian Weimer on 8/27/2009 8:35 AM:
> * Eric Blake:
> 
>> int open_safer (const char *name, int flags, int mode)
>> {
>>   int fd = open (name, flags | O_CLOEXEC, mode);
>>   if (0 <= fd && fd <= 2)
>>     {
>>       int dup = fcntl (fd, ((flags & O_CLOEXEC)
>>                             ? F_DUPFD_CLOEXEC : F_DUPFD), 3);
>>       int saved_errno = errno;
>>       close (fd);
>>       errno = saved_errno;
>>       fd = dup;
>>     }
>>   else if (!(flags & O_CLOEXEC))
>>     {
>>       if ((flags = fcntl (fd, F_GETFD)) < 0
>>           || fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>>         {
>>           int saved_errno = errno;
>>           close (fd);
>>           fd = -1;
>>           errno = saved_errno;
>>         }
>>     }
>>   return fd;
>> }
> 
>> This solves the fd leak,
> 
> It's still buggy.

In what manner?  Are you stating that F_DUPFD_CLOEXEC is not atomic?

>  You need something like this:
> 
> int open_safer(const char *name, int flags, int mode)
> {
>   int opened_fd[3] = {0, 0, 0};
>   int fd, i, errno_saved;
>   while (1) {
>     fd = open(name, flags | O_CLOEXEC, mode);
>     if (fd < 0 || fd > 2) {
>       break;
>     }
>     opened_fd[fd] = 1;
>   }
>   for (int i = 0; i <= 2; ++i) {
>     if (opened_fd[i]) {
>       errno_saved = errno;
>       close(i);
>       errno = errno_saved;
>     }
>   }
>   return fd;
> }
> 
> It's untested, so it's probably still buggy.

Your version fails to clear the cloexec bit of the final fd if the
original caller didn't request O_CLOEXEC.  If the caller requested
O_CLOEXEC, then your version takes 3, 5, or 7 syscalls depending on how
many std fds were closed, while my version takes 3 syscalls regardless of
how many std fds were closed.  Also, your suggestion has a definite race
in that you are calling open() multiple times rather than cloning an
existing fd after the first open(), such that another process could alter
which file is visited between your first and last open().

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@....net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqX0W0ACgkQ84KuGfSFAYDYdwCeOB8dt0Rx2QYJkfIsfP452AYj
V7QAoL1FACwnRPhhQ2aTh2EB38i+d34o
=ouPs
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/