From patchwork Tue Aug 26 21:26:15 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roland McGrath X-Patchwork-Id: 2543 Received: (qmail 13335 invoked by alias); 26 Aug 2014 21:26:20 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 13317 invoked by uid 89); 26 Aug 2014 21:26:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL, BAYES_50 autolearn=ham version=3.3.2 X-HELO: topped-with-meat.com MIME-Version: 1.0 From: Roland McGrath To: Samuel Thibault Cc: libc-alpha@sourceware.org, bug-hurd@gnu.org Subject: Re: [PATCH,Hurd] bind() fails when umask is 0000 In-Reply-To: Samuel Thibault's message of Monday, 25 August 2014 14:21:57 +0200 <20140825122157.GC3213@type.bordeaux.inria.fr> References: <53B83BBB.7060303@googlemail.com> <20140824192957.GY3053@type.youpi.perso.aquilenet.fr> <20140824200642.GD3053@type.youpi.perso.aquilenet.fr> <20140824211149.GG3053@type.youpi.perso.aquilenet.fr> <20140825120934.GB3213@type.bordeaux.inria.fr> <20140825122157.GC3213@type.bordeaux.inria.fr> Message-Id: <20140826212615.6E7F62C39C1@topped-with-meat.com> Date: Tue, 26 Aug 2014 14:26:15 -0700 (PDT) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.1 cv=SvUDtp+0 c=1 sm=1 tr=0 a=WkljmVdYkabdwxfqvArNOQ==:117 a=14OXPxybAAAA:8 a=oya8eBD9Z1sA:10 a=5ar2OUDr854A:10 a=Z6MIti7PxpgA:10 a=kj9zAlcOel0A:10 a=hOe2yjtxAAAA:8 a=qVlsZRq2mlfaFHwcWRcA:9 a=JCjmnSwR3EckbTdN:21 a=ys5iLClUu1dsCjOH:21 a=CjuIK1q_8ugA:10 I assume you mean umask is 0666 or more, such that (0600 & ~umask) is 0. The fix to use dir_lookup of "" to invoke the active translator seems correct and orthogonal to the umask issue. What that fixes is a race where you could wind up with a different node than the one you just created, if the by-name lookup happens after someone swooped in and renamed/linked another node over the name bind just dir_link'd. I don't know why I wrote it the wrong way in the first place. The EADDRINUSE check it does after the fact seems insufficient. Can you send a fix for just that issue separately first? (And be careful with your whitespace.) The ifsock translator checks the underlying node for the access allowed to enforce that you must have read access to the naming node to connect an AF_LOCAL socket to a listener. It's incidentally wrong that it's check for read access, because POSIX says that the permission necessary for connect is write access--to wit, on Linux you can connect to a socket you own with mode 0200 but not to one with mode 0400 (I haven't tested other Unices but I imagine they are the same). But more relevant is that it feels like it is using the wrong model for the check. Why is it checking how the user could have opened the underlying node, instead of checking how the user actually did open the translated node? i.e.: Oh yeah, but diskfs/ext2fs is short-circuiting and not actually using the ifsock translator. The diskfs ifsock_getsockaddr already checks for write access rather than read. It too does it on the underlying node, but AFAICT for S_IFSOCK diskfs never actually makes a distinction between the underlying and translated node. That's kind of grotty. Is it really true that a file port to an ext2fs S_IFSOCK node reads as having _HURD_IFSOCK as translator whether or not you used O_NOTRANS to get the port? When you have a port to the translated node (i.e. opened without O_NOTRANS), then you really should not be able to distinguish talking to /hurd/ifsock from talking to a diskfs that is short-cutting it, but this is a way you can. Then we'd still need to fix bind so it can get an O_WRITE-capable port to the new translator. But dir_lookup will usually deny O_WRITE open on an S_IFSOCK node. Hmm. I still don't like changing the mode after the mkfile, though it is probably indeed the easiest fix. Bletch. It looks like trivfs/ifsock doesn't constrain opening the virtual node with O_READ and/or O_WRITE and/or O_EXEC, as diskfs does for opening the underlying "shortcut" node. So with diskfs/ext2fs you could just use O_WRITE in the dir_mkfile call and then call ifsock_getsockaddr on the underlying node port after setting the translator; and with trivfs/ifsock you could do the dir_lookup with O_WRITE. But each only works because it's wrong! If we made diskfs distinguish the underlying node from the virtual node to fix the "appears to be infinite layers of _HURD_IFSOCK translator" bug, then it wouldn't let you get an O_WRITE port on the virtual node. If we made ifsock refuse to open the virtual node with O_* bits set, then it too wouldn't let you get an O_WRITE port on the virtual node. Harumph. diff --git a/trans/ifsock.c b/trans/ifsock.c index 4ed6589..b2ad02b 100644 --- a/trans/ifsock.c +++ b/trans/ifsock.c @@ -1,5 +1,5 @@ /* Server for S_IFSOCK nodes - Copyright (C) 1994, 1995, 2001, 02, 2006 Free Software Foundation + Copyright (C) 1994, 1995, 2001, 02, 2006, 2014 Free Software Foundation This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as @@ -134,19 +134,15 @@ error_t S_ifsock_getsockaddr (struct trivfs_protid *cred, mach_port_t *address) { - int perms; - error_t err; - if (!cred || cred->pi.bucket != port_bucket || cred->pi.class != node_class) return EOPNOTSUPP; - err = file_check_access (cred->realnode, &perms); - if (!err && !(perms & O_READ)) - err = EACCES; + /* Require write access to connect, per POSIX. */ + if (!(cred->po->openmodes & O_WRITE)) + return EACCES; - if (!err) - *address = address_port; - return err; + *address = address_port; + return 0; }