Re: RFC: [PATCH] trans/fakeroot.c

[Svante Signell]

On Thu, 2015-05-21 at 10:56 +0200, Svante Signell wrote:
> 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.
...
> A few notes:
> - 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.
> 

The above condition is really happening when building (patched to
build, not related to fakeroot) gpsd.

I have now built the failing packages with the patch inlined below:
- gpsd 3.15-1 (patched to build, bug #<tbd>)
- frama-c 20150201+sodium+dfsg-2
- snd 11.7-4 (patched to build, bug #<tbd>) Have to check the failing
package. With a fixed patch snd b11.7-4 uilds fine with the old
fakeroot and with the corrected one. 
- gcc-snapshot 20150817-1, (20150913-1 has problems with some pthread
headers when building g++,same problem on the buildd)

 --- a/trans/fakeroot.c.orig    2015-10-08 22:32:09.000000000 +0200
+++ b/trans/fakeroot.c  2015-10-08 22:34:47.000000000 +0200
@@ -216,9 +216,9 @@
     {
       /* 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 with no inclusion. `file' doesn't fit
either,
+         /* Intersecting sets. `file' doesn't fit either,
             we need yet another new peropen on this node.  */
          mach_port_deallocate (mach_task_self (), file);
          file = MACH_PORT_NULL;

I'll be submitting bug reports to make the packages build later
(outside fakeroot), I'm currently connected a very low quota wireless
connection (the main one is broken due to an unexpected cable being cut
off. Repairs will be made later this week.) <bbl>

I told you, the test should be inclusive, not exclusive :)