[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: RFC: [PATCH] trans/fakeroot.c
From: |
Svante Signell |
Subject: |
Re: RFC: [PATCH] trans/fakeroot.c |
Date: |
Thu, 21 May 2015 10:56:24 +0200 |
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!
trans_fakeroot.patch
Description: Text Data
Re: RFC: [PATCH] trans/fakeroot.c, Samuel Thibault, 2015/05/15