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

2015-10-13 Thread Svante Signell 
On Mon, 2015-10-12 at 15:21 +0200, Samuel Thibault wrote:

> I believe they will.  But perhaps in your tests they didn't (but did
> your tests really try that?), in which case it'd mean there is a bug.
> But for now nothing said there was a bug there.

FYI: gcc-snapshot (20150817, not 20150913) and frama-c (20150201+sodium
+dfsg-2) with downgraded dependencies (probably an FTBFS issue if
rebuilt on other arches) build fine with the latest fakeroot :)

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

2015-10-13 Thread Samuel Thibault 
Svante Signell, le Tue 13 Oct 2015 13:42:22 +0200, a écrit :
> On Mon, 2015-10-12 at 15:21 +0200, Samuel Thibault wrote:
> 
> > I believe they will.  But perhaps in your tests they didn't (but did
> > your tests really try that?), in which case it'd mean there is a bug.
> > But for now nothing said there was a bug there.
> 
> FYI: gcc-snapshot (20150817, not 20150913) and frama-c (20150201+sodium
> +dfsg-2) with downgraded dependencies (probably an FTBFS issue if
> rebuilt on other arches) build fine with the latest fakeroot :) 

I've seen that on buildds, yes, but snd & such still have an issue with
temporary files in /tmp

Samuel

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

2015-10-13 Thread Svante Signell 
On Tue, 2015-10-13 at 13:49 +0200, Samuel Thibault wrote:
> Svante Signell, le Tue 13 Oct 2015 13:42:22 +0200, a écrit :
> > On Mon, 2015-10-12 at 15:21 +0200, Samuel Thibault wrote:

> > FYI: gcc-snapshot (20150817, not 20150913) and frama-c (20150201+sodium
> > +dfsg-2) with downgraded dependencies (probably an FTBFS issue if
> > rebuilt on other arches) build fine with the latest fakeroot :) 
> 
> I've seen that on buildds, yes, but snd & such still have an issue with
> temporary files in /tmp

Well, with the updated 06-hurd.diff patch in #801604 I don't see any
fakeroot problems (old or new version)

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

2015-10-12 Thread Samuel Thibault 
Svante Signell, le Mon 12 Oct 2015 15:16:19 +0200, a écrit :
> On Mon, 2015-10-12 at 15:12 +0200, Samuel Thibault wrote:
> > Svante Signell, le Mon 12 Oct 2015 15:04:40 +0200, a écrit :
> > > OK I buy your explanation; though there seems to be remaining bugs.
> > 
> > Which bugs?
> 
> You wrote it not me:
> > Subsequent writes to the file will then fail (and if they don't,
> that's another bug to be fixed).

"If they don't"

I believe they will.  But perhaps in your tests they didn't (but did
your tests really try that?), in which case it'd mean there is a bug.
But for now nothing said there was a bug there.

Samuel

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

2015-10-12 Thread Svante Signell 
On Mon, 2015-10-12 at 14:49 +0200, Samuel Thibault wrote:
> Svante Signell, le Mon 12 Oct 2015 14:45:09 +0200, a écrit :
> > On Mon, 2015-10-12 at 02:08 +0200, Samuel Thibault wrote:
> > > The commit I have just pushed, which does make sense, does fix gpsd at
> > > least.

> Please erase your build tree and restart the build from scratch. You
> probably have a remaining non-writable gpsd.udev file.

Yes, I had forgotten to remove it. Thanks!

> > Since the packages I've built builds fine using the condition below:
> > if (file != MACH_PORT_NULL && (nn->openmodes & newmodes))
> > please explain what is wrong with it and why it works while your does
> > not?
> 
> I have explained at length how the function is supposed to behave in
> my previous mail.  Your proposed change does not meet what I said.
> Notably, in the nn->openmodes = O_READ and newmodes = O_WRITE case,
> it'll be wrong: nn->openmodes & newmodes will be zero, and thus the
> 'file' port will be used as such, but it will not have O_WRITE mode!
> Subsequent writes to the file will then fail (and if they don't, that's
> another bug to be fixed).

OK I buy your explanation; though there seems to be remaining bugs.

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

2015-10-12 Thread Svante Signell 
On Mon, 2015-10-12 at 02:08 +0200, Samuel Thibault wrote:
> The commit I have just pushed, which does make sense, does fix gpsd at
> least.

Building gpsd (3.15-1) with your patch applied:
cp /home/srs/DEBs/gpsd/gpsd-3.15/gpsd.rules 
/home/srs/DEBs/gpsd/gpsd-3.15/debian/gpsd.udev
cp: cannot create regular file
‘/home/srs/DEBs/gpsd/gpsd-3.15/debian/gpsd.udev’: Permission denied

Since the packages I've built builds fine using the condition below:
if (file != MACH_PORT_NULL && (nn->openmodes & newmodes))
please explain what is wrong with it and why it works while your does
not?

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

2015-10-12 Thread Svante Signell 
On Mon, 2015-10-12 at 15:12 +0200, Samuel Thibault wrote:
> Svante Signell, le Mon 12 Oct 2015 15:04:40 +0200, a écrit :
> > OK I buy your explanation; though there seems to be remaining bugs.
> 
> Which bugs?

You wrote it not me:
> Subsequent writes to the file will then fail (and if they don't,
that's another bug to be fixed).

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

2015-10-12 Thread Samuel Thibault 
Svante Signell, le Mon 12 Oct 2015 15:04:40 +0200, a écrit :
> OK I buy your explanation; though there seems to be remaining bugs.

Which bugs?

Samuel

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

2015-10-12 Thread Samuel Thibault 
Svante Signell, le Mon 12 Oct 2015 14:45:09 +0200, a écrit :
> On Mon, 2015-10-12 at 02:08 +0200, Samuel Thibault wrote:
> > The commit I have just pushed, which does make sense, does fix gpsd at
> > least.
> 
> Building gpsd (3.15-1) with your patch applied:
> cp /home/srs/DEBs/gpsd/gpsd-3.15/gpsd.rules 
> /home/srs/DEBs/gpsd/gpsd-3.15/debian/gpsd.udev
> cp: cannot create regular file
> ‘/home/srs/DEBs/gpsd/gpsd-3.15/debian/gpsd.udev’: Permission denied

Please erase your build tree and restart the build from scratch. You
probably have a remaining non-writable gpsd.udev file.

> Since the packages I've built builds fine using the condition below:
> if (file != MACH_PORT_NULL && (nn->openmodes & newmodes))
> please explain what is wrong with it and why it works while your does
> not?

I have explained at length how the function is supposed to behave in
my previous mail.  Your proposed change does not meet what I said.
Notably, in the nn->openmodes = O_READ and newmodes = O_WRITE case,
it'll be wrong: nn->openmodes & newmodes will be zero, and thus the
'file' port will be used as such, but it will not have O_WRITE mode!
Subsequent writes to the file will then fail (and if they don't, that's
another bug to be fixed).

Samuel

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

2015-10-12 Thread Samuel Thibault 
Samuel Thibault, le Mon 12 Oct 2015 14:49:43 +0200, a écrit :
> Your proposed change does not meet what I said.
> Notably, in the nn->openmodes = O_READ and newmodes = O_WRITE case,
> it'll be wrong: nn->openmodes & newmodes will be zero, and thus the
> 'file' port will be used as such, but it will not have O_WRITE mode!
> Subsequent writes to the file will then fail (and if they don't, that's
> another bug to be fixed).

Grmbl, I actually meant: in the nn->openmodes = O_WRITE and newmodes =
O_READ case.

Samuel

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

2015-10-11 Thread Samuel Thibault 
Svante Signell, le Sun 11 Oct 2015 23:02:16 +0200, a écrit :
> > 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.

Ok. It makes complete sense: we've been writing to a file, and now we
are reading from it.

>  --- a/trans/fakeroot.c.orig  2015-10-08 22:32:09.0 +0200
> +++ b/trans/fakeroot.c2015-10-08 22:34:47.0 +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,

But this doesn't make sense!  Let me explain in detail what
check_openmodes does:

- we have an existing node, nn, which contains an opened file nn->file,
  opened with modes nn->openmodes.
- we want to have it opened with nn->openmodes | newmodes.
- the 'file' port parameter happens to be that file opened with
newmodes.

so:

- either nn->openmodes already contains the newmodes bits, nn->file is
thus already the file opened with modes nn->openmodes | newmodes
- or it doesn't, and so we have to do something:
  - if newmodes actually contains all the nn->openmodes bit, then the
  'file' port is just what we need, and there is no need to reopen the
  file.
  - otherwise the 'file' port is useless, so we should just close it,
  and really reopen the file with nn->openmodes | newmodes.

So the condition for closing the file and making it null is
"nn->openmodes has bits that newmodes doesn't have".  That happens to
translate to "nn->openmodes & ~newmodes".  If I'm wrong somewhere in my
reasoning (which can probably be very true), please tell me where.

Just saying "that testcase works with my patch" is not enough to
convince me to apply it, because there are so many other cases. In the
testcase you mention above:

> > nn->openmodes = 2 = O_WRITE and
> > newmodes = 1 = O_READ i.e. no intersecting sets.

(nn->openmodes & ~newmodes) will be O_WRITE, which shows that we have an
existing file opened with O_WRITE, but the provided 'file' port doesn't
have it (as announced by newmodes). And thus we *have* to destroy the
'file' port and not use it, and instead reopen the file to really get
O_WRITE|O_READ: otherwise we wouldn't have O_WRITE any more, which will
pose problem whenever we want to write to it again.

In the other testcase you mentioned in your previous mail:

> nn->openmodes = 1 = O_READ and
> newmodes = 3 = O_READ | O_WRITE,

We have an existing file opened in O_READ, and now we want
O_READ|O_WRITE. The existing file is not enough, it's missing O_WRITE
(newmodes &~ nn->openmodes returns O_WRITE), but the provided 'file'
port has both O_READ|O_WRITE, so we can just use it, there is no need to
reopen the file, it'll cover both the original need (O_READ) as tested
by nn->openmodes & ~newmodes being 0, and the new need (O_READ|O_WRITE).

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

You didn't prove why. Testcases are good when they cover all cases.
Otherwise they are just an indication of something is going wrong, not
that the proposed fix is always right.

Either what I said above is wrong at some point, or the bug is somewhere
else (see the previous changes we had to make in netfs_attempt_chmod for
instance).  What call *actually* produces the permission denied error,
BTW?  In our previous exchange it was actually the reopen operation.

Samuel

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

2015-10-11 Thread 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=376) = 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=376) = 2
> (nn->openmodes & newmodes) = 0
> 
> file == MACH_PORT_NULL
> nn->file=84
> (nn->openmodes=2 | newmodes=1) = 3
> bad_retryname=NULL, file=0
>  = 0x400d (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,
> _retry, bad_retryname, );
> 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 #)
- frama-c 20150201+sodium+dfsg-2
- snd 11.7-4 (patched to build, bug #) 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.orig2015-10-08 22:32:09.0 +0200
+++ b/trans/fakeroot.c  2015-10-08 22:34:47.0 +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.) 

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

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

2015-10-11 Thread Samuel Thibault 
The commit I have just pushed, which does make sense, does fix gpsd at
least.

Samuel

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

2015-06-10 Thread Svante Signell 
On Tue, 2015-06-09 at 17:15 +0200, Svante Signell wrote:

 I've built pycorrfit and hurd so far, no issues yet. Will build glibc,
 gnat-4.9 and gcc-5 to be sure.

glibc and gnat-4.9 built fine. gcc-5 will take several hours.

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

2015-06-10 Thread Svante Signell 
On Wed, 2015-06-10 at 10:05 +0200, Svante Signell wrote:
 On Tue, 2015-06-09 at 17:15 +0200, Svante Signell wrote:
 
  I've built pycorrfit and hurd so far, no issues yet. Will build glibc,
  gnat-4.9 and gcc-5 to be sure.
 
 glibc and gnat-4.9 built fine. gcc-5 will take several hours.

And gcc-5: 5.1.1-9 built fine too (after a hang and reboot, memory
starvation)

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

2015-06-09 Thread Svante Signell 
On Tue, 2015-06-09 at 11:11 +0200, Samuel Thibault wrote:

 So the package is actually doing something stupid (yes, that's what you
 should have written in your mail to explain what is happening, actually
 :) ). But it does work as root as specified by POSIX, so we have to
 support it.

Yes I think there is a package bug (it's up to the package maintainer to
write good code, not me pointing fingers):
The command should have read
chmod -R 644
debian/pycorrfit/usr/share/doc/pycorrfit/examples/external_model_functions/*
since there are no directories below.

Nevertheless, as you write, it has to be fixed.

 I'd say in netfs_attempt_chmod just do
 
   real_mode |= S_IRUSR;
   real_mode |= S_IWUSR;
   if (nn is a directory)
 real_mode |= S_IXUSR;
 
 so we're sure of having all rights that root would have on the
 underlying file.

New patch attached. Maybe you want to remove the second condition to add
S_IXUSR unconditionally for directories.


Index: hurd-0.6.git20150523/trans/fakeroot.c
===
--- hurd-0.6.git20150523.orig/trans/fakeroot.c
+++ hurd-0.6.git20150523/trans/fakeroot.c
@@ -560,12 +560,10 @@ netfs_attempt_chmod (struct iouser *cred
  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;
+if ((real_mode  S_IFDIR)  ((real_mode  S_IXUSR) == 0))
+  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.  */

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

2015-06-09 Thread Svante Signell 
On Tue, 2015-06-09 at 10:55 +0200, Svante Signell wrote:
 On Tue, 2015-06-09 at 10:43 +0200, Samuel Thibault wrote:
  Svante Signell, le Tue 09 Jun 2015 10:13:46 +0200, a écrit :
   Done so now. Attached are two versions of a patch to fakeroot.c. In
   netfs_get_dirents(): call netfs_attempt_chmod() before dir_readdir() to
   make sure that directories are accessible (executable) before changing
   the underlying files.
  
  Why are they not accessible?
  
  We don't want to browntape-fix the issue, but really understand what is
  happening.
 
 chmod -R 644 directory makes all directories drwr--r-- i.e. not
 accessible, as I wrote. Is that a brown-tape fix?
 
 
mkdir test
touch test/a

chmod -R test
chmod: cannot access ‘test/a’: Permission denied

ls -ld test
drw-r--r-- 2 srs srs 4096 Jun  9 11:00 test

ls -l test
ls: cannot access test/a: Permission denied
total 0
-? ? ? ? ?? a

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

2015-06-09 Thread Samuel Thibault 
Svante Signell, le Tue 09 Jun 2015 10:55:35 +0200, a écrit :
 On Tue, 2015-06-09 at 10:43 +0200, Samuel Thibault wrote:
  Svante Signell, le Tue 09 Jun 2015 10:13:46 +0200, a écrit :
   Done so now. Attached are two versions of a patch to fakeroot.c. In
   netfs_get_dirents(): call netfs_attempt_chmod() before dir_readdir() to
   make sure that directories are accessible (executable) before changing
   the underlying files.
  
  Why are they not accessible?
  
  We don't want to browntape-fix the issue, but really understand what is
  happening.
 
 chmod -R 644 directory makes all directories drwr--r-- i.e. not
 accessible, as I wrote.

Ah, right :)

Sorry I'm so busy I can't even follow the thread.

So the package is actually doing something stupid (yes, that's what you
should have written in your mail to explain what is happening, actually
:) ). But it does work as root as specified by POSIX, so we have to
support it.

 Is that a brown-tape fix?

Yes: It's still trying to fix the file mode only after the fact, only
when we happen to need more permissions, rather than at the right
time, i.e. when the mode is erroneously modified. That was the point
of the patch commited in netfs_attempt_chmod before: when modifying the
mode, keep what we need to work this the files. Thinking again about it,
we added

  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;

to make sure that the mode we actually set on the underlying file permit
that we can continue accessing the file as needed.  But root has all
rights anyway, the only exception is executing a file which doesn't have
+x. So netfs_attempt_chmod should could just allow everything on the
underlying file. I.e. I'd say in netfs_attempt_chmod just do

  real_mode |= S_IRUSR;
  real_mode |= S_IWUSR;
  if (nn is a directory)
real_mode |= S_IXUSR;

so we're sure of having all rights that root would have on the
underlying file.

Samuel

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

2015-06-09 Thread Svante Signell 
On Fri, 2015-06-05 at 11:18 +0530, Samuel Thibault wrote:
 Hello,
 
 It seems there is still a corner case which doesn't work: the pycorrfit
 package fails to build with:
 
 chmod -R 644 
 debian/pycorrfit/usr/share/doc/pycorrfit/examples/external_model_functions/
 chmod: cannot access 
 'debian/pycorrfit/usr/share/doc/pycorrfit/examples/external_model_functions/ExampleFunc_CS_2D+2D+S+T.txt':
  Permission denied
 
 etc.  Could you have a look?

Done so now. Attached are two versions of a patch to fakeroot.c. In
netfs_get_dirents(): call netfs_attempt_chmod() before dir_readdir() to
make sure that directories are accessible (executable) before changing
the underlying files. The resulting directory (and file) access
permissions are the same as with fakeroot-tcp and Linux.
The first patch adds S_IXUSR to real_mode, the other adds O_EXEC to
nn-openmodes. Your choice (if OK, of course, maybe I've missed
something.)

Thanks!

Index: hurd-0.6.git20150523/trans/fakeroot.c
===
--- hurd-0.6.git20150523.orig/trans/fakeroot.c
+++ hurd-0.6.git20150523/trans/fakeroot.c
@@ -796,8 +796,19 @@ netfs_get_dirents (struct iouser *cred,
 		   mach_msg_type_number_t *datacnt,
 		   vm_size_t bufsize, int *amt)
 {
-  return dir_readdir (netfs_node_netnode (dir)-file, data, datacnt,
+  struct netnode *nn = netfs_node_netnode (dir);
+  mode_t real_mode = dir-nn_stat.st_mode;
+  error_t err = 0;
+
+  if ((real_mode  S_IFDIR)  ((real_mode  S_IXUSR) == 0))
+{
+  err = netfs_attempt_chmod (cred, dir, real_mode | S_IXUSR);
+  if (err)
+	return err;
+}
+  err = dir_readdir (nn-file, data, datacnt,
 		  entry, nentries, bufsize, amt);
+  return err;
 }
 
 error_t
Index: hurd-0.6.git20150523/trans/fakeroot.c
===
--- hurd-0.6.git20150523.orig/trans/fakeroot.c
+++ hurd-0.6.git20150523/trans/fakeroot.c
@@ -796,8 +796,20 @@ netfs_get_dirents (struct iouser *cred,
 		   mach_msg_type_number_t *datacnt,
 		   vm_size_t bufsize, int *amt)
 {
-  return dir_readdir (netfs_node_netnode (dir)-file, data, datacnt,
+  struct netnode *nn = netfs_node_netnode (dir);
+  mode_t real_mode = dir-nn_stat.st_mode;
+  error_t err = 0;
+
+  if ((real_mode  S_IFDIR)  ((real_mode  S_IXUSR) == 0))
+{
+  nn-openmodes |= O_EXEC;
+  err = netfs_attempt_chmod (cred, dir, real_mode);
+  if (err)
+	return err;
+}
+  err = dir_readdir (nn-file, data, datacnt,
 		  entry, nentries, bufsize, amt);
+  return err;
 }
 
 error_t

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

2015-06-09 Thread Samuel Thibault 
Svante Signell, le Tue 09 Jun 2015 10:13:46 +0200, a écrit :
 Done so now. Attached are two versions of a patch to fakeroot.c. In
 netfs_get_dirents(): call netfs_attempt_chmod() before dir_readdir() to
 make sure that directories are accessible (executable) before changing
 the underlying files.

Why are they not accessible?

We don't want to browntape-fix the issue, but really understand what is
happening.

Samuel

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

2015-06-09 Thread Svante Signell 
On Tue, 2015-06-09 at 10:43 +0200, Samuel Thibault wrote:
 Svante Signell, le Tue 09 Jun 2015 10:13:46 +0200, a écrit :
  Done so now. Attached are two versions of a patch to fakeroot.c. In
  netfs_get_dirents(): call netfs_attempt_chmod() before dir_readdir() to
  make sure that directories are accessible (executable) before changing
  the underlying files.
 
 Why are they not accessible?
 
 We don't want to browntape-fix the issue, but really understand what is
 happening.

chmod -R 644 directory makes all directories drwr--r-- i.e. not
accessible, as I wrote. Is that a brown-tape fix?

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

2015-06-09 Thread Samuel Thibault 
Svante Signell, le Tue 09 Jun 2015 11:41:01 +0200, a écrit :
 On Tue, 2015-06-09 at 11:11 +0200, Samuel Thibault wrote:
 
  So the package is actually doing something stupid (yes, that's what you
  should have written in your mail to explain what is happening, actually
  :) ). But it does work as root as specified by POSIX, so we have to
  support it.
 
 Yes I think there is a package bug (it's up to the package maintainer to
 write good code, not me pointing fingers):

Well, the code is supposed to be run by root, so it's actually sorta
correct :)

 New patch attached. Maybe you want to remove the second condition to add
 S_IXUSR unconditionally for directories.

There is no point in testing for the flag before setting it, indeed.

You also need to fix the indentation.

 +if ((real_mode  S_IFDIR) 

I don't think it's so simple, see the precise S_IFMT bit matter. That
should most probably be rather an S_ISDIR call.

Samuel

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

2015-06-05 Thread Samuel Thibault 
Hello,

It seems there is still a corner case which doesn't work: the pycorrfit
package fails to build with:

chmod -R 644 
debian/pycorrfit/usr/share/doc/pycorrfit/examples/external_model_functions/
chmod: cannot access 
'debian/pycorrfit/usr/share/doc/pycorrfit/examples/external_model_functions/ExampleFunc_CS_2D+2D+S+T.txt':
 Permission denied

etc.  Could you have a look?

Samuel

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

2015-05-23 Thread Samuel Thibault 
gnat-5 built fine indeed.

Congrats again for having tracked this bug!

I have switched the buildds to using fakeroot-hurd by default, and will
probably bump the alternatives priority soon too.

Samuel

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

2015-05-23 Thread Justus Winter 
Quoting Samuel Thibault (2015-05-23 23:53:07)
 I have switched the buildds to using fakeroot-hurd by default, and will
 probably bump the alternatives priority soon too.

Awesome :)

Justus

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

2015-05-22 Thread Samuel Thibault 
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 21 | tee rpctrace_nOK.out
 ...
   9--154(pid16637)-dir_lookup (dev/null 1 0)
 Intersecting sets
 newmodes=1, (nn-openmodes=2  ~newmodes=376) = 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=376) = 2
 (nn-openmodes  newmodes) = 0
 
 file == MACH_PORT_NULL
 nn-file=84
 (nn-openmodes=2 | newmodes=1) = 3
 bad_retryname=NULL, file=0
  = 0x400d (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 () = 0xfed1 ((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)

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

2015-05-22 Thread Samuel Thibault 
Samuel Thibault, le Sat 23 May 2015 02:04:24 +0530, a écrit :
 Could you try the attached patch? It makes your testcase work fine, but
 perhaps it has other unexpected consequences.

Thinking again about it, I believe it will be really fine, so commited
it.

Samuel

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

2015-05-21 Thread Svante Signell 
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 21 | tee rpctrace_nOK.out
...
  9--154(pid16637)-dir_lookup (dev/null 1 0)
Intersecting sets
newmodes=1, (nn-openmodes=2  ~newmodes=376) = 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=376) = 2
(nn-openmodes  newmodes) = 0

file == MACH_PORT_NULL
nn-file=84
(nn-openmodes=2 | newmodes=1) = 3
bad_retryname=NULL, file=0
 = 0x400d (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

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

2015-05-15 Thread Samuel Thibault 
Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
 +  /* XXX: Limit openmodes */
 +  if (openmodes  O_ALLOWED)
 +openmodes = O_ALLOWED; /* works */

More precisely, which openmode posed problem here?  Perhaps this is not
the best place to exclude them.  At the very least we need to know what
exactly was posing problem before patching the source code.

Also, use if (openmodes  ~O_ALLOWED) rather than openmodes  O_ALLOWED.
It happens that O_ALLOWED contains contiguous bits from bit #0, and thus
 O_ALLOWED happens to produce the expected result, but that's only by
luck.

 +   /* XXX: Change nn-openmodes selectively */
 +   //if (newmodes  nn-openmodes)
 + nn-openmodes = newmodes;

Ditto.

 -  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.

Samuel

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

2015-05-12 Thread Samuel Thibault 
Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
 The idea is to limit the openmodes according to the values defined in
 struct netnode and to change the test for overlapping sets.

In which case is this needed? What happens in that case and how the
patch fixes this?

A patch should be provided with not only an explanation of what it does,
but also why this is the right way to fix the eventual bug you want to
fix.

Samuel

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

2015-05-12 Thread Svante Signell 
On Tue, 2015-05-12 at 09:42 +0200, Samuel Thibault wrote:
 Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
  The idea is to limit the openmodes according to the values defined in
  struct netnode and to change the test for overlapping sets.
 
 In which case is this needed? What happens in that case and how the
 patch fixes this?

I already supplied a test case. What's up?

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

2015-05-12 Thread Svante Signell 
On Tue, 2015-05-12 at 11:22 +0200, Samuel Thibault wrote:
 Svante Signell, le Tue 12 May 2015 10:00:26 +0200, a écrit :
  On Tue, 2015-05-12 at 09:42 +0200, Samuel Thibault wrote:
   Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
The idea is to limit the openmodes according to the values defined in
struct netnode and to change the test for overlapping sets.
   
   In which case is this needed? What happens in that case and how the
   patch fixes this?
  
  I already supplied a test case. What's up?
 
 Explain what happens.
 
 Only providing a testcase means the reviewer has to

 For instance here, just inventing dumb things, it's just to give an
 example of what I need to see explained to avoid having to spend a few
 hours to understand what's happening: the testcase creates a socket,

Sorry for the confusion: This patch is not about the socket creation
case. It is about the build failure of gnat-4.9 with fakeroot-hurd. two
different bugs!

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

2015-05-12 Thread Samuel Thibault 
Svante Signell, le Tue 12 May 2015 10:00:26 +0200, a écrit :
 On Tue, 2015-05-12 at 09:42 +0200, Samuel Thibault wrote:
  Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
   The idea is to limit the openmodes according to the values defined in
   struct netnode and to change the test for overlapping sets.
  
  In which case is this needed? What happens in that case and how the
  patch fixes this?
 
 I already supplied a test case. What's up?

Explain what happens.

Only providing a testcase means the reviewer has to

- understand what the testcase is doing
- infer how the patched source code gets triggered
- find out how that source code does things wrong
- understand why the proposed changes fixes that behavior

Which takes a lot of time, while the submitter already know all this and
just has to explain it.

For instance here, just inventing dumb things, it's just to give an
example of what I need to see explained to avoid having to spend a few
hours to understand what's happening: the testcase creates a socket,
whose mode is not just read/write, and thus it test at line 1234 it
erroneously takes the then branch, while it shouldn't, since it's just
a socket. With the proposed changes, the function will just reject the
mode, and thus the caller will correctly revert back to code path bar
which does things properly: it calls foobaz(), which is what we want for
the testcase.

Really, doing this won't take you much time, and save the reviewer a lot
of time (which is just duplicate since it's exactly what you did in your
work).

Samuel

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

2015-05-12 Thread Samuel Thibault 
Svante Signell, le Tue 12 May 2015 11:39:19 +0200, a écrit :
 On Tue, 2015-05-12 at 11:22 +0200, Samuel Thibault wrote:
  Svante Signell, le Tue 12 May 2015 10:00:26 +0200, a écrit :
   On Tue, 2015-05-12 at 09:42 +0200, Samuel Thibault wrote:
Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
 The idea is to limit the openmodes according to the values defined in
 struct netnode and to change the test for overlapping sets.

In which case is this needed? What happens in that case and how the
patch fixes this?
   
   I already supplied a test case. What's up?
  
  Explain what happens.
  
  Only providing a testcase means the reviewer has to
 
  For instance here, just inventing dumb things, it's just to give an
  example of what I need to see explained to avoid having to spend a few
  hours to understand what's happening: the testcase creates a socket,
 
 Sorry for the confusion: This patch is not about the socket creation
 case. It is about the build failure of gnat-4.9 with fakeroot-hurd. two
 different bugs!

Ok, this shows how explanation is more than welcome :)

Samuel

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

2015-05-12 Thread Samuel Thibault 
Svante Signell, le Tue 12 May 2015 10:00:26 +0200, a écrit :
 On Tue, 2015-05-12 at 09:42 +0200, Samuel Thibault wrote:
  Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
   The idea is to limit the openmodes according to the values defined in
   struct netnode and to change the test for overlapping sets.
  
  In which case is this needed? What happens in that case and how the
  patch fixes this?
 
 I already supplied a test case. What's up?

Also, in the test case, you didn't explain what happens and what is
supposed to happen. Your patch seems to be dealing with modes, so
perhaps it's the modes which should be observed? But when running
fakeroot-hurd make the resulting file has mode 444, so it seems right.
Or perhaps it's the md5sum part which is bogus? But I do get the proper
md5sum.

When reporting a bug, *always* explain all of

- the steps to reproduce
- what you get
- what you expected to get

and when providing a patch, explain what I mentioned in my other mail.

Samuel