bug-hurd
[Top][All Lists]
Advanced

[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!

Attachment: trans_fakeroot.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]