Thanks again for the explanations, that's exactly what I needed to
manage to have a look at your proposed changes quickly :)
Svante Signell, le Thu 21 May 2015 10:56:24 +0200, a écrit :
> - 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.
This is not only purely cosmetic: it actually changes something in the
code. This is a browntape fix. That's typically where electrical
engineers and computer scientists can't agree: a browntape fix is no
good in computer science, while it can be completely reasonable for
electrical engineering :) Instead, I have commited a hard assertion:
callers of new_node and check_openmodes are supposed to be careful (this
is what the existing code does, except in one place).
> - the condition (nn->openmodes & ~newmodes) is wrong since this triggers
> on newmodes being complementary to nn->openmodes
Not exactly: it triggers when both `nn->openmodes' has some flag
that `newmodes' doesn't have, and `newmodes' has some flag that
`nn->openmodes' doesn't have. I.e. `nn->file' is missing an openmode,
but `file' is missing one too, so we really need to create another
file_t, in order to have all the openmodes that we need.
> /* Intersecting sets.
> We need yet another new peropen on this node. */
> The correct condition for intersection is the and operation, i.e.
I think it's the comment which is not precise enough, it'd rather be
"Intersecting sets with no inclusion".
> one example being
> nn->openmodes = 1 = O_READ and
> newmodes = 3 = O_READ | O_WRITE,; the intersection being O_READ.
The goal of check_openmodes is not the intersection, but the union,
actually, see nn->openmodes | newmodes in the dir_lookup call.
> 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.
Yes, but the ground reason is not a bogus test: here, we previously had
write access to gnatvsn_from/alloc.ali (nn->openmodes = 2), but now we
want read access too (newmodes = 1), so we reopen it, in order to get
both. Yes, we want to have both. Using your patch would mean that we
lose the write access. The test is really right: we want to reopen the
file, with the union of the modes. And it's the reopen which fails.
> 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);
Yes, and *that* is what produces the permission denied error. In
the case of /dev/null, there is no error since /dev/null is always
world-readable/writable. In the case of gnatvsn_from/alloc.ali, it
depends on the mode of the underlying file. And then I realize what you
wrote earlier in your mail:
> - 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.
Well, I guess this is where things don't actually work: an initial open
of gnatvsn_from/alloc.ali with modes = 0, and thus fakeroot creates the
underlying file with no access right, and then when we want to re-open
it we can't since it doesn't have the permissions.
So, in the end, although the effect happens in check_openmodes, the
bug is somewhere between the creation of gnatvsn_from/alloc.ali
and the reopen here. In your testcase, it's tar x which creates
gnatvsn_from/alloc.ali, the testcase can be reduced to:
(cd dst/gnatvsn_to; tar xf ../../file.tar)
md5sum dst/gnatvsn_to/gnatvsn_from/alloc.ali
within the same fakeroot-hurd session. To create the file, tar does:
10<--67(pid3274)->dir_lookup ("gnatvsn_from/alloc.ali" 4194362 256) = 0 1 ""
86<--89(pid3274)
86<--89(pid3274)->term_getctty () = 0xfffffed1 ((ipc/mig) bad request message
ID)
86<--89(pid3274)->io_write ("V "GNAT Lib v4.9"\nA -nostdinc\nA -O2\nA -fPIC\nA
-g\nA -mtune=generic\nA -march=i586\n" -1) = 0 2267
86<--89(pid3274)->file_utimes ({0 0} {1418859518 0}) = 0
86<--89(pid3274)->file_chown (1000 1000) = 0
86<--89(pid3274)->file_chmod (292) = 0
i.e. initially mode 0, and only after, chmods it to 0444. Err, that's
why the reopen with mode = 3 can't work of course! So the bug is that
since we still have the write openmode from the tar process, we don't
want to chmod to something which is not writable. Bingo.
Could you try the attached patch? It makes your testcase work fine, but
perhaps it has other unexpected consequences.
Samuel
diff --git a/trans/fakeroot.c b/trans/fakeroot.c
index a223bc4..feaab15 100644
--- a/trans/fakeroot.c
+++ b/trans/fakeroot.c
@@ -525,14 +529,27 @@ real_from_fake_mode (mode_t mode)
error_t
netfs_attempt_chmod (struct iouser *cred, struct node *np, mode_t mode)
{
+ struct netnode *nn;
+ mode_t real_mode;
+
if ((mode & S_IFMT) == 0)
mode |= np->nn_stat.st_mode & S_IFMT;
if ((mode & S_IFMT) != (np->nn_stat.st_mode & S_IFMT))
return EOPNOTSUPP;
+ /* Make sure that `check_openmodes' will still always be able to reopen it */
+ real_mode = mode;
+ nn = netfs_node_netnode (np);
+ if (nn->openmodes & O_READ)
+ real_mode |= S_IRUSR;
+ if (nn->openmodes & O_WRITE)
+ real_mode |= S_IWUSR;
+ if (nn->openmodes & O_EXEC)
+ real_mode |= S_IXUSR;
+
/* We don't bother with error checking since the fake mode change should
always succeed--worst case a later open will get EACCES. */
- (void) file_chmod (netfs_node_netnode (np)->file, mode);
+ (void) file_chmod (nn->file, real_mode);
set_faked_attribute (np, FAKE_MODE);
np->nn_stat.st_mode = mode;
return 0;
