On Fri, 2015-05-15 at 20:31 +0200, Samuel Thibault wrote:
> Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
> > - if (file != MACH_PORT_NULL && (nn->openmodes & ~newmodes))
> > + if (file != MACH_PORT_NULL && (nn->openmodes & newmodes )) /* works
> > */
>
> This change needs to be motivated and explained.
The updated patch is now much smaller, see attached trans_fakeroot.patch
and comes with an explanation, however not yet with a changelog entry.
A few notes:
- fakeroot works OK even without limiting the openmodes, the crucial fix
is the changed condition, to be explained below.
- there are two cosmetic changes (I know they should not be there, but
for code unification/readability)
- struct netnode contains
int openmodes; /* O_READ | O_WRITE | O_EXEC */
therefore I chose to limit the openmodes.
- An openmode equal to zero should not be allowed, but seems to be
unavoidable. A zero openmode would mean no read or write access
according to fcntl.h:
# define O_NORW 0 /* Open without R/W access. */
However, things seems to work, even with openmodes being zero.
- the condition (nn->openmodes & ~newmodes) is wrong since this triggers
on newmodes being complementary to nn->openmodes but the comment in the
code says:
/* Intersecting sets.
We need yet another new peropen on this node. */
The correct condition for intersection is the and operation, i.e.
(nn->openmodes & newmodes ), one example being
nn->openmodes = 1 = O_READ and
newmodes = 3 = O_READ | O_WRITE,; the intersection being O_READ.
For the test case given before and the wrong condition triggers the
following:
./my_fakeroot-hurd rpctrace make 2>&1 | tee rpctrace_nOK.out
...
9<--154(pid16637)->dir_lookup ("dev/null" 1 0)
Intersecting sets
newmodes=1, (nn->openmodes=2 & ~newmodes=37777777776) = 2
(nn->openmodes & newmodes) = 0
file == MACH_PORT_NULL
nn->file=62
(nn->openmodes=2 | newmodes=1) = 3
bad_retryname=NULL, file=60
= 0 1 "" 175<--183(pid16637)
with no bad effects but
146<--178(pid16637)->dir_lookup ("gnatvsn_from/alloc.ali" 1 0)
Intersecting sets
newmodes=1, (nn->openmodes=2 & ~newmodes=37777777776) = 2
(nn->openmodes & newmodes) = 0
file == MACH_PORT_NULL
nn->file=84
(nn->openmodes=2 | newmodes=1) = 3
bad_retryname=NULL, file=0
= 0x4000000d (Permission denied)
has, since the file returned is zero.
According to dir_lookup in fs.defs a file name of null is equivalent to
a reopen of that file.
err = dir_lookup (nn->file, "", nn->openmodes | newmodes, 0,
&bad_retry, bad_retryname, &file);
We also see from the printout that
nn->openmodes = 2 = O_WRITE and
newmodes = 1 = O_READ i.e. no intersecting sets.
- With the correct condition this code is not triggered in the test
code, but does e.g. when building gnat-4.9.
I hope this explanation is sufficient. The pached fakerot has been
build-tested as written before on gnumach, hurd, glibc, gnat-4.9 and
gcc-5.
Thanks!
Index: hurd-0.6/trans/fakeroot.c
===================================================================
--- hurd-0.6.orig/trans/fakeroot.c
+++ hurd-0.6/trans/fakeroot.c
@@ -91,7 +91,11 @@ new_node (file_t file, mach_port_t idpor
}
nn = netfs_node_netnode (*np);
nn->file = file;
- nn->openmodes = openmodes;
+
+ /* FIXME: Forbid nn->openmodes == 0 and openmodes == 0? */
+ /* XXX: Limit nn->openmodes */
+ nn->openmodes = openmodes & (O_RDWR | O_EXEC);
+
if (idport != MACH_PORT_NULL)
nn->idport = idport;
else
@@ -207,11 +211,13 @@ check_openmodes (struct netnode *nn, int
{
error_t err = 0;
- if (newmodes &~ nn->openmodes)
+ /* XXX: Limit nn->openmodes */
+ nn->openmodes &= (O_RDWR | O_EXEC);
+ if (~nn->openmodes & newmodes)
{
/* The user wants openmodes we haven't tried before. */
- if (file != MACH_PORT_NULL && (nn->openmodes & ~newmodes))
+ if (file != MACH_PORT_NULL && (nn->openmodes & newmodes ))
{
/* Intersecting sets.
We need yet another new peropen on this node. */
@@ -388,7 +394,7 @@ netfs_S_dir_lookup (struct protid *dirus
}
err = check_openmodes (netfs_node_netnode (np),
- (flags & (O_RDWR|O_EXEC)), file);
+ flags & (O_RDWR|O_EXEC), file);
pthread_mutex_unlock (&idport_ihash_lock);
}
else
