Debian Bug report logs - #102437
auth does not pass page aligned memory to mig

Package: hurd; Maintainer for hurd is GNU Hurd Maintainers <debian-hurd@lists.debian.org>; Source for hurd is src:hurd.

Reported by: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>

Date: Wed, 27 Jun 2001 02:05:04 UTC

Severity: normal

Tags: upstream

Forwarded to https://savannah.gnu.org/bugs/index.php?func=detailitem&item_id=15322

Reply or subscribe to this bug.

Toggle useless messages

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
New Bug report received and forwarded. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #5 received at submit@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: submit@bugs.debian.org
Subject: proc leaks memory with the bootstrap filesystem entry
Date: Wed, 27 Jun 2001 03:59:03 +0200
Package: hurd
Version: current

Hi,

this is really a nice obscure bug.

Calling proc_getprocinfo on PID 4 (root filesystem server) causes proc
to leak a couple of kilobytes (pages?) each time.  This can easily be
reproduced by

ps -F hurd 0
ps -F hurd 4
ps -F hurd 0

etc.  Do this right after booting, so RSS for proc is still under 1 MB (this
makes it easier to see).  To prove that PID 4 is the only process with this
problem, the below program iterates over all pids.  If you specify any command
line arguments, it will skip PID 4.  In this case, no memory is leaked.

This is most certainly a bootstrap issue.  Interestingly, the same behaviour
is exposed in a sub hurd (for the sub hurds root filesystem and the sub hurds
proc server).  I have no concrete idea yet to what the actual bug is.
Any hints, as always, appreciated.

BTW, fork() makes proc leak memory as well.  Reproduce with "forks 1000 10000".
This seems to be an unrelated bug,
which I plan to investigate after this one is fixed.

Thanks,
Marcus




Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #10 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: 102437@bugs.debian.org
Subject: Re: Bug#102437: proc leaks memory with the bootstrap filesystem entry
Date: Wed, 27 Jun 2001 11:26:53 +0200
On Wed, Jun 27, 2001 at 03:59:03AM +0200, Marcus Brinkmann wrote:
> To prove that PID 4 is the only process with this
> problem, the below program iterates over all pids.  If you specify any command
> line arguments, it will skip PID 4.  In this case, no memory is leaked.

#define _GNU_SOURCE

#include <hurd.h>
#include <hurd/process.h>

int
main (int argc, char *argv[])
{
  int i;
  int err;
  mach_port_t proc = getproc();
  pid_t *pids;
  int pidslen;
  int flags = PI_FETCH_THREADS | PI_FETCH_THREAD_BASIC;
  int *piarray;
  int picount = 0;
  char *waits;
  int waitslen = 0;

  pids = 0;
  pidslen = 0;
  err =  proc_getallpids (proc, &pids, &pidslen);
  if (err)
    {
      printf ("%s: proc_getallpids(): %s\n", argv[0], strerror(errno));
      exit (1);
    }

  for (i = 0; i < pidslen; i++)
    {
      /* Specify an argument, and the symptoms go away.  */
      if (argc >= 2 && i == 4) 
        continue;
      piarray = waits = picount = waitslen = 0;

      err = proc_getprocinfo (proc, pids[i], &flags, &piarray, &picount, &waits, &waitslen);
      if (err)
	{
	  printf ("%s: proc_getprocinfo() for %i: %s\n", argv[0], pids[i], strerror(errno));
	  exit (1);
	}
	printf ("%i: %i, %i\n", pids[i], picount, waitslen);

      if (piarray)
	munmap (piarray, sizeof(int) * picount);
      if (waits)
        munmap (waits, sizeof(char) * waitslen);

    }
}

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #15 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: 102437@bugs.debian.org
Subject: dealloc missing on out parameters (was: Re: Bug#102437: proc leaks memory with the bootstrap filesystem entry
Date: Wed, 27 Jun 2001 12:35:25 +0200
On Wed, Jun 27, 2001 at 03:59:03AM +0200, Marcus Brinkmann wrote:
> this is really a nice obscure bug.

Oh man.  This is a prime example for what you can read out of data if you
want to.

The real cause of the bug is the allocation of *piarray, which only happens
if the number of threads in the task exceed a certain amount (about 30?). 
So, this will happen with all tasks that have many threads, and the root
filesystem starts out with 41 threads on my system.

A quick look at the mig server routines reveals the bug:

  routine proc_getprocinfo (
          process: process_t;
          which: pid_t;
          inout flags: int;
-         out procinfo: procinfo_t;
+         out procinfo: procinfo_t, dealloc;
          out threadwaits: data_t, dealloc);

There are other arrays which are used as out parameter but not have dealloc
specified.  Is any of these appropriate?  We should really clean this up, as
all of these can potentially leak memory if I am not mistaken.

  routine file_get_storage_info (
          file: file_t;
          RPT
-         out ports: portarray_t;
-         out ints: intarray_t;          
-         out offsets: off_array_t;
-         out data: data_t);
+         out ports: portarray_t, dealloc;
+         out ints: intarray_t, dealloc;
+         out offsets: off_array_t, dealloc;
+         out data: data_t, dealloc);

  routine file_get_fs_options (
          file: file_t;
          RPT
-         out options: data_t);
+         out options: data_t, dealloc);

  routine fsys_get_options (
          server: fsys_t;
          RPT
-         out options: data_t);
+         out options: data_t, dealloc);

  routine auth_getids (
          handle: auth_t;
-         out euids: idarray_t;
-         out auids: idarray_t;
-         out egids: idarray_t;
-         out agids: idarray_t);
+         out euids: idarray_t, dealloc;
+         out auids: idarray_t, dealloc;
+         out egids: idarray_t, dealloc;
+         out agids: idarray_t, dealloc);

  routine auth_server_authenticate (
          handle: auth_t;
          sreplyport reply: mach_port_poly_t;
          rendezvous: mach_port_send_t;
          newport: mach_port_poly_t;
-         out euids: idarray_t;
-         out auids: idarray_t;
-         out egids: idarray_t;
-         out agids: idarray_t);
+         out euids: idarray_t, dealloc;
+         out auids: idarray_t, dealloc;
+         out egids: idarray_t, dealloc;
+         out agids: idarray_t, dealloc);

Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Roland McGrath <roland@gnu.org>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #20 received at 102437@bugs.debian.org (full text, mbox):

From: Roland McGrath <roland@gnu.org>
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>, 102437@bugs.debian.org
Subject: Re: Bug#102437: dealloc missing on out parameters (was: Re: Bug#102437: proc leaks memory with the bootstrap filesystem entry
Date: Wed, 27 Jun 2001 07:42:57 -0400 (EDT)
You need to look at each individual server function and see what is the
right thing for it.  There is no generic answer (some things are copying
data they hold elsewhere, some should dealloc).  Note there is also
`dealloc[]' to give an argument the server can fill in.



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #25 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: 102437@bugs.debian.org
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Wed, 27 Jun 2001 14:52:17 +0200
Hi,

ok, here a detailed analysis.  It seems I found four other bugs along the
way. ;)

On Wed, Jun 27, 2001 at 12:35:25PM +0200, Marcus Brinkmann wrote:
>   routine proc_getprocinfo (
>           process: process_t;
>           which: pid_t;
>           inout flags: int;
> -         out procinfo: procinfo_t;
> +         out procinfo: procinfo_t, dealloc;
>           out threadwaits: data_t, dealloc);

proc_getprocinfo uses mmap if the buffer is not big enough, and copies the
out data into it.  This one definitely needs a dealloc.

>   routine file_get_storage_info (
>           file: file_t;
>           RPT
> -         out ports: portarray_t;
> -         out ints: intarray_t;          
> -         out offsets: off_array_t;
> -         out data: data_t);
> +         out ports: portarray_t, dealloc;
> +         out ints: intarray_t, dealloc;
> +         out offsets: off_array_t, dealloc;
> +         out data: data_t, dealloc);

store_encode (used by ext2fs, ufs mmaps new buffers if the provided ones
are not big enough.  Other callers I checked (default in libnetfs) also
do this.  So, all these should get dealloc, too.

BTW, the comment in libstore enc.c seems to be wrong:

/* Copy out the parameters from ENC into the given variables suitably for
   returning from a file_get_storage_info rpc, and deallocate ENC.  */
void
store_enc_return (struct store_enc *enc,
 
ENC is not allocated, and it is not deallocated either.

>   routine file_get_fs_options (
>           file: file_t;
>           RPT
> -         out options: data_t);
> +         out options: data_t, dealloc);

This one uses iohelp_return_malloced_buffer in libdiskfs/libnetfs/libtrivfs,
which mmaps a new buffer if the provided one is not big enough and copies
the string.  So, dealloc!

>   routine fsys_get_options (
>           server: fsys_t;
>           RPT
> -         out options: data_t);
> +         out options: data_t, dealloc);

See file_get_fs_options, the code is very similar.
 
>   routine auth_getids (
>           handle: auth_t;
> -         out euids: idarray_t;
> -         out auids: idarray_t;
> -         out egids: idarray_t;
> -         out agids: idarray_t);
> +         out euids: idarray_t, dealloc;
> +         out auids: idarray_t, dealloc;
> +         out egids: idarray_t, dealloc;
> +         out agids: idarray_t, dealloc);

This one is questionable.  auth_getids uses idvec_copyout, which provides
the internal buffer:

inline void idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
{
  if (idvec->num > *nids)
    *ids = idvec->ids;
  else
    memcpy (*ids, idvec->ids, idvec->num * sizeof *ids);
  *nids = idvec->num;
}

So, this should not use dealloc.  However, idvec->ids is NOT a mmap'ed
buffer!  It is a malloced/realloced buffer (coming from
libshouldbeinlibc/idvec.c).  Also, the code should not use
sizeof *ids, but sizeof (uid_t) = sizeof **ids for the calculation!

Is it correct that this should better use iohelp_return_malloced_buffer
and dealloc or mmap in idvec.h for the performance?

>   routine auth_server_authenticate (
>           handle: auth_t;
>           sreplyport reply: mach_port_poly_t;
>           rendezvous: mach_port_send_t;
>           newport: mach_port_poly_t;
> -         out euids: idarray_t;
> -         out auids: idarray_t;
> -         out egids: idarray_t;
> -         out agids: idarray_t);
> +         out euids: idarray_t, dealloc;
> +         out auids: idarray_t, dealloc;
> +         out egids: idarray_t, dealloc;
> +         out agids: idarray_t, dealloc);


Again, I am not sure.  The server routine has:

  /* Extract the ids.  We must use a separate reply stub so
     we can deref the user auth handle after the reply uses its
     contents.  */
  auth_server_authenticate_reply (reply, reply_type, 0,
                                  user->euids.ids, user->euids.num,
                                  user->auids.ids, user->auids.num,
                                  user->egids.ids, user->egids.num,
                                  user->agids.ids, user->agids.num);
  ports_port_deref (user);

Because those arrays are not marked as dealloc, the pointer is copied by
the reply stub.  Again, we are returning malloc'ed buffers.  But this time
we are returning buffers without keeping a reference to them!  The comment
is actually useless, as the buffers are not copied.  Am I incorrect, or is
this a blatant error?

It seems that using return-buffer and dealloc is the only chance, if we
don't want to giver the server a reference to the user.

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #30 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: 102437@bugs.debian.org
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Wed, 27 Jun 2001 15:04:20 +0200
On Wed, Jun 27, 2001 at 02:52:17PM +0200, Marcus Brinkmann wrote:
> So, this should not use dealloc.  However, idvec->ids is NOT a mmap'ed
> buffer!  It is a malloced/realloced buffer (coming from
> libshouldbeinlibc/idvec.c).
> 
> Is it correct that this should better use iohelp_return_malloced_buffer
> and dealloc or mmap in idvec.h for the performance?

It just occured to me that it is probably safe to use malloc()'ed buffers,
as long as you don't use dealloc with them?  It's a bit awkward, because
this is mixing Mach level with glibc level stuff.  I don't know if it is
safe to return a buffer that is not starting at a mmap'ed region, for
example.

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to tb@becket.net (Thomas Bushnell, BSG):
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #35 received at 102437@bugs.debian.org (full text, mbox):

From: tb@becket.net (Thomas Bushnell, BSG)
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
Cc: 102437@bugs.debian.org
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: 29 Jun 2001 11:10:46 -0700
Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de> writes:

> It just occured to me that it is probably safe to use malloc()'ed buffers,
> as long as you don't use dealloc with them?  It's a bit awkward, because
> this is mixing Mach level with glibc level stuff.  I don't know if it is
> safe to return a buffer that is not starting at a mmap'ed region, for
> example.

In general it's bad mojo.  If the buffer is not page aligned, then
Mach will DTRT, but data in the boundary pages outside the region
might be sent in the RPC too, and the result would be a security
failure.

Also, we don't want to depend on that behavior of Mach; other kernels
with other RPC systems might only allow sending page-aligned regions.




Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #40 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: 102437@bugs.debian.org
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Sat, 30 Jun 2001 21:16:17 +0200
On Wed, Jun 27, 2001 at 02:52:17PM +0200, Marcus Brinkmann wrote:

[auth_getids]
 
> This one is questionable.  auth_getids uses idvec_copyout, which provides
> the internal buffer:
> 
> inline void idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
> {
>   if (idvec->num > *nids)
>     *ids = idvec->ids;
>   else
>     memcpy (*ids, idvec->ids, idvec->num * sizeof *ids);
>   *nids = idvec->num;
> }
> 
> So, this should not use dealloc.  However, idvec->ids is NOT a mmap'ed
> buffer!  It is a malloced/realloced buffer (coming from
> libshouldbeinlibc/idvec.c).  Also, the code should not use
> sizeof *ids, but sizeof (uid_t) = sizeof **ids for the calculation!
> 
> Is it correct that this should better use iohelp_return_malloced_buffer
> and dealloc or mmap in idvec.h for the performance?

I would suggest dealloc + iohelp_return_malloced_buffer, as usually those
uid vectors are small, and keeping them on one page each is a waste of
space.
 
> >   routine auth_server_authenticate (

[...]

> Again, I am not sure.  The server routine has:
> 
>   /* Extract the ids.  We must use a separate reply stub so
>      we can deref the user auth handle after the reply uses its
>      contents.  */
>   auth_server_authenticate_reply (reply, reply_type, 0,
>                                   user->euids.ids, user->euids.num,
>                                   user->auids.ids, user->auids.num,
>                                   user->egids.ids, user->egids.num,
>                                   user->agids.ids, user->agids.num);
>   ports_port_deref (user);
> 
> Because those arrays are not marked as dealloc, the pointer is copied by
> the reply stub.  Again, we are returning malloc'ed buffers.  But this time
> we are returning buffers without keeping a reference to them!  The comment
> is actually useless, as the buffers are not copied.  Am I incorrect, or is
> this a blatant error?

I was incorrect, I think.  Small uid vectors are copied in the message
inlined, while out-of-band memory is logically copied.  Although the
physical page is shared, it will not be removed unless the server and client
both called vm_deallocate on it (if I understood this correctly).
 
> It seems that using return-buffer and dealloc is the only chance, if we
> don't want to giver the server a reference to the user.

Not in this case.  But again, because we use malloc for the uids, I'd
suggest to use return-buffer and dealloc anyway.

I will make an appropriate patch and post it here.

Thanks,
Marcus



-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to tb@becket.net (Thomas Bushnell, BSG):
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #45 received at 102437@bugs.debian.org (full text, mbox):

From: tb@becket.net (Thomas Bushnell, BSG)
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
Cc: 102437@bugs.debian.org
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: 30 Jun 2001 20:54:12 -0700
Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de> writes:

> I would suggest dealloc + iohelp_return_malloced_buffer, as usually those
> uid vectors are small, and keeping them on one page each is a waste of
> space.

iohelp_return_malloced_buffer is not available in auth, nor should it
be.... just duplicated its functionality perhaps.  it's not that
complex.

I haven't reviewed your logic tho; I'll think about that later.



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #50 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: "Thomas Bushnell, BSG" <tb@becket.net>, 102437@bugs.debian.org
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Wed, 11 Jul 2001 00:18:32 +0200
On Sat, Jun 30, 2001 at 08:54:12PM -0700, Thomas Bushnell, BSG wrote:
> Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de> writes:
> 
> > I would suggest dealloc + iohelp_return_malloced_buffer, as usually those
> > uid vectors are small, and keeping them on one page each is a waste of
> > space.
> 
> iohelp_return_malloced_buffer is not available in auth, nor should it
> be.... just duplicated its functionality perhaps.  it's not that
> complex.

Right.  We can't use it anyway, as it will free the buffer.  Duplicating the
functionality is the thing to do (we do it in a lot of places).
 
> I haven't reviewed your logic tho; I'll think about that later.

I committed my changes for the other interfaces, which just require the
dealloc parameter.  For the auth server, I will send a patch for review
soon.

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #55 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: "Thomas Bushnell, BSG" <tb@becket.net>, 102437@bugs.debian.org
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Wed, 11 Jul 2001 21:25:01 +0200
On Wed, Jul 11, 2001 at 12:18:32AM +0200, Marcus Brinkmann wrote:
> For the auth server, I will send a patch for review soon.

Here it comes.  I am not too happy about the macro magic, but it works out
ok.  The code for S_auth_getids is completely reduplicated in
S_auth_server_authenticate.  Do you want me to simply call S_auth_getids
instead?

Problem the patch wants to solve: Returned buffers are not page aligned if
they don't fit into the provided buffers, because they are malloc()'ed.

Way to fix it: If buffer is not big enough, allocate new buffer with mmap.
For this to work, all parameters must be "dealloc" for MiG (including the in
paramters of the reply stub).

Thanks,
Marcus

diff -ru hurd/auth/ChangeLog hurd-work/auth/ChangeLog
--- hurd/auth/ChangeLog	Mon Feb 12 22:47:07 2001
+++ hurd-work/auth/ChangeLog	Wed Jul 11 21:18:00 2001
@@ -1,3 +1,22 @@
+2001-07-11  Marcus Brinkmann  <marcus@gnu.org>
+
+	* auth.c : Include <sys/mman.h>.
+	(idvec_copyout): Change return type from void to int.
+	Return buffer is allocated if not big enough.  Clean up
+	calculation of array size.  Return an error on failure and 0 on
+	success.
+	(C): Only call idvec_copyout if no error occured so far.
+	(OUTIDS): Change syntax to accomodate changes to definition of C.
+	(D, SAVEIDS): New macros to define local variables EUIDS_SAVED,
+	EGIDS_SAVED, AUIDS_SAVED, AGIDS_SAVED and store the respective
+	arguments in them.
+	(F, FREEIDS): New macros to free allocated buffers, to be used
+	on errors in conjunction with SAVEIDS.
+	(S_auth_getids): Use SAVEIDS and FREEIDS.
+	New variable err.
+	(S_auth_server_authenticate): Before calling the reply stub, copy
+	out the ids just as in S_auth_getids.
+
 2001-02-12  Marcus Brinkmann  <marcus@gnu.org>
 
 	* auth.c (main): New variable ARGP defining a doc string.
diff -ru hurd/auth/auth.c hurd-work/auth/auth.c
--- hurd/auth/auth.c	Mon Feb 12 22:39:42 2001
+++ hurd-work/auth/auth.c	Wed Jul 11 21:16:51 2001
@@ -27,6 +27,7 @@
 #include <hurd/ports.h>
 #include <hurd/ihash.h>
 #include <idvec.h>
+#include <sys/mman.h>
 #include <assert.h>
 #include <argp.h>
 #include <error.h>
@@ -82,18 +83,30 @@
 
 /* id management.  */
 
-inline void idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
+inline int idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
 {
   if (idvec->num > *nids)
-    *ids = idvec->ids;
-  else
-    memcpy (*ids, idvec->ids, idvec->num * sizeof *ids);
+    {
+      *ids = mmap (0, idvec->num * sizeof (**ids), PROT_READ|PROT_WRITE,
+		   MAP_ANON, 0, 0);
+      if (*ids == (uid_t *) -1)
+	return errno;
+    }
+  memcpy (*ids, idvec->ids, idvec->num * sizeof (**ids));
   *nids = idvec->num;
+
+  return 0;
 }
 
-#define C(auth, ids)	idvec_copyout (&auth->ids, ids, n##ids)
-#define OUTIDS(auth)	(C (auth, euids), C (auth, egids), \
-			 C (auth, auids), C (auth, agids))
+#define C(auth, ids)	if (! err) \
+			  err = idvec_copyout (&auth->ids, ids, n##ids)
+#define OUTIDS(auth)	{ C (auth, euids); C (auth, egids); \
+			  C (auth, auids); C (auth, agids); }
+#define D(ids)	*ids##_saved = *ids
+#define SAVEIDS	uid_t D (euids), D (egids), D (auids), D (agids)
+#define F(ids)	if (*ids != ids##_saved) \
+		  munmap (*ids, *n##ids * sizeof (uid_t))
+#define FREEIDS { F (euids); F (egids); F (auids); F (agids); }
 
 /* Implement auth_getids as described in <hurd/auth.defs>. */
 kern_return_t
@@ -107,12 +120,17 @@
 	       uid_t **agids,
 	       u_int *nagids)
 {
+  int err = 0;
+  SAVEIDS;
+
   if (! auth)
     return EOPNOTSUPP;
-
+  
   OUTIDS (auth);
-
-  return 0;
+  if (err)
+    FREEIDS;
+      
+  return err;
 }
 
 /* Implement auth_makeauth as described in <hurd/auth.defs>. */
@@ -421,6 +439,18 @@
   /* Extract the ids.  We must use a separate reply stub so
      we can deref the user auth handle after the reply uses its
      contents.  */
+  {
+    int err = 0;
+    SAVEIDS;
+
+    OUTIDS (user);
+
+    if (err)
+      {
+	FREEIDS;
+	return err;
+      }
+  }
   auth_server_authenticate_reply (reply, reply_type, 0,
 				  user->euids.ids, user->euids.num,
 				  user->auids.ids, user->auids.num,
diff -ru hurd/hurd/ChangeLog hurd-work/hurd/ChangeLog
--- hurd/hurd/ChangeLog	Wed Jul 11 00:15:12 2001
+++ hurd-work/hurd/ChangeLog	Wed Jul 11 19:31:47 2001
@@ -1,5 +1,14 @@
 2001-07-11  Marcus Brinkmann  <marcus@gnu.org>
 
+	* auth_reply.defs (simpleroutine auth_server_authentcate_reply):
+	Add dealloc to in paramters (GEN_UIDS, AUX_UIDS, GEN_GIDS, AUX_GIDS).
+
+	* auth.defs (routine auth_getids): Add dealloc to all out parameters
+	(EUIDS, AUIDS, EGIDS, AGIDS).
+	(routine auth_server_authenticate): Likewise.
+
+2001-07-11  Marcus Brinkmann  <marcus@gnu.org>
+
 	* fs.defs (routine file_get_storage_info): Add dealloc to all out
 	parameters (PORTS, INTS, OFFSETS, DATA).
 	(routine file_get_fs_options): Add dealloc to out parameter OPTIONS.
diff -ru hurd/hurd/auth.defs hurd-work/hurd/auth.defs
--- hurd/hurd/auth.defs	Fri May  3 22:56:21 1996
+++ hurd-work/hurd/auth.defs	Wed Jul 11 19:28:18 2001
@@ -1,5 +1,5 @@
 /* Definitions for the authentication server
-   Copyright (C) 1991, 1992, 1993, 1994, 1996 Free Software Foundation
+   Copyright (C) 1991, 1992, 1993, 1994, 1996, 2001 Free Software Foundation
 
 This file is part of the GNU Hurd.
 
@@ -38,10 +38,10 @@
 /* Given an authentication handle, return the identification. */
 routine auth_getids (
 	handle: auth_t;
-	out euids: idarray_t;
-	out auids: idarray_t;
-	out egids: idarray_t;
-	out agids: idarray_t);
+	out euids: idarray_t, dealloc;
+	out auids: idarray_t, dealloc;
+	out egids: idarray_t, dealloc;
+	out agids: idarray_t, dealloc);
 
 /* Create a new authentication handle.  */
 routine auth_makeauth (
@@ -72,9 +72,7 @@
 	sreplyport reply: mach_port_poly_t;
 	rendezvous: mach_port_send_t;
 	newport: mach_port_poly_t;
-	out euids: idarray_t;
-	out auids: idarray_t;
-	out egids: idarray_t;
-	out agids: idarray_t);
-
-
+	out euids: idarray_t, dealloc;
+	out auids: idarray_t, dealloc;
+	out egids: idarray_t, dealloc;
+	out agids: idarray_t, dealloc);
diff -ru hurd/hurd/auth_reply.defs hurd-work/hurd/auth_reply.defs
--- hurd/hurd/auth_reply.defs	Mon Jan 24 23:38:38 1994
+++ hurd-work/hurd/auth_reply.defs	Wed Jul 11 19:31:36 2001
@@ -1,5 +1,5 @@
 /* Reply-only side of auth interface
-   Copyright (C) 1991, 1993, 1994 Free Software Foundation
+   Copyright (C) 1991, 1993, 1994, 2001 Free Software Foundation
 
 This file is part of the GNU Hurd.
 
@@ -37,8 +37,7 @@
 simpleroutine auth_server_authenticate_reply (
 	reply_port: reply_port_t;
 	in return_code: kern_return_t;
-	in gen_uids: idarray_t;
-	in aux_uids: idarray_t;
-	in gen_gids: idarray_t;
-	in aux_gids: idarray_t);
-	
+	in gen_uids: idarray_t, dealloc;
+	in aux_uids: idarray_t, dealloc;
+	in gen_gids: idarray_t, dealloc;
+	in aux_gids: idarray_t, dealloc);

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Roland McGrath <roland@gnu.org>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #60 received at 102437@bugs.debian.org (full text, mbox):

From: Roland McGrath <roland@gnu.org>
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>, 102437@bugs.debian.org
Cc: "Thomas Bushnell, BSG" <tb@becket.net>
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Wed, 11 Jul 2001 16:49:17 -0400 (EDT)
That's fine by me if it's tested.  BTW add -p to your diff switches.



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #65 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: Roland McGrath <roland@gnu.org>, 102437@bugs.debian.org
Cc: "Thomas Bushnell, BSG" <tb@becket.net>
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Thu, 12 Jul 2001 21:06:12 +0200
On Wed, Jul 11, 2001 at 04:49:17PM -0400, Roland McGrath wrote:
> That's fine by me if it's tested.  BTW add -p to your diff switches.

Actually, I goofed up.  The arguments to the reply stub have not been
changed by my patch.  In fact, as we now copy the user data all the time, a
seperate reply stub is not necessary anymore!  There is also a bug
somewhere.

I will work out something better.

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #70 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: Roland McGrath <roland@gnu.org>, 102437@bugs.debian.org
Cc: "Thomas Bushnell, BSG" <tb@becket.net>
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Thu, 12 Jul 2001 21:37:51 +0200
On Thu, Jul 12, 2001 at 09:06:12PM +0200, Marcus Brinkmann wrote:
> On Wed, Jul 11, 2001 at 04:49:17PM -0400, Roland McGrath wrote:
> > That's fine by me if it's tested.  BTW add -p to your diff switches.
> 
> Actually, I goofed up.  The arguments to the reply stub have not been
> changed by my patch.  In fact, as we now copy the user data all the time, a
> seperate reply stub is not necessary anymore!  There is also a bug
> somewhere.

The bug was actually what I said above.  The following version is tested and
seems to work fine.

Note that it got much simpler, because the extra reply stub is not needed. 
I left the change to auth_reply.defs in anyway.  However, nothing in the
Hurd uses the stubs in auth_reply.defs anymore.

Thanks,
Marcus

hurd/
2001-07-11  Marcus Brinkmann  <marcus@gnu.org>

        * auth_reply.defs (simpleroutine auth_server_authentcate_reply):
        Add dealloc to in paramters (GEN_UIDS, AUX_UIDS, GEN_GIDS, AUX_GIDS).

        * auth.defs (routine auth_getids): Add dealloc to all out parameters
        (EUIDS, AUIDS, EGIDS, AGIDS).
        (routine auth_server_authenticate): Likewise.

auth/
2001-07-11  Marcus Brinkmann  <marcus@gnu.org>

        * auth.c : Include <sys/mman.h>.
        (idvec_copyout): Change return type from void to int.
        Return buffer is allocated if not big enough.  Clean up
        calculation of array size.  Return an error on failure and 0 on
        success.
        (C): Only call idvec_copyout if no error occured so far.
        (OUTIDS): Change syntax to accomodate changes to definition of C.
        (D, SAVEIDS): New macros to define local variables EUIDS_SAVED,
        EGIDS_SAVED, AUIDS_SAVED, AGIDS_SAVED and store the respective
        arguments in them.
        (F, FREEIDS): New macros to free allocated buffers, to be used
        on errors in conjunction with SAVEIDS.
        (S_auth_getids): Use SAVEIDS and FREEIDS.
        New variable err.
        (S_auth_server_authenticate): Use SAVEIDS, make ERR a local variable
	of the whole function.  Instead calling the reply stub, copy out the
	ids just as in S_auth_getids.  Return ERR.


--- hurd/hurd/auth.defs	Fri May  3 22:56:21 1996
+++ /mnt/marcus/gnu/hurd/hurd/hurd-20010610/hurd/auth.defs	Wed Jul 11 20:44:38 2001
@@ -1,5 +1,5 @@
 /* Definitions for the authentication server
-   Copyright (C) 1991, 1992, 1993, 1994, 1996 Free Software Foundation
+   Copyright (C) 1991, 1992, 1993, 1994, 1996, 2001 Free Software Foundation
 
 This file is part of the GNU Hurd.
 
@@ -38,10 +38,10 @@
 /* Given an authentication handle, return the identification. */
 routine auth_getids (
 	handle: auth_t;
-	out euids: idarray_t;
-	out auids: idarray_t;
-	out egids: idarray_t;
-	out agids: idarray_t);
+	out euids: idarray_t, dealloc;
+	out auids: idarray_t, dealloc;
+	out egids: idarray_t, dealloc;
+	out agids: idarray_t, dealloc);
 
 /* Create a new authentication handle.  */
 routine auth_makeauth (
@@ -72,9 +72,7 @@
 	sreplyport reply: mach_port_poly_t;
 	rendezvous: mach_port_send_t;
 	newport: mach_port_poly_t;
-	out euids: idarray_t;
-	out auids: idarray_t;
-	out egids: idarray_t;
-	out agids: idarray_t);
-
-
+	out euids: idarray_t, dealloc;
+	out auids: idarray_t, dealloc;
+	out egids: idarray_t, dealloc;
+	out agids: idarray_t, dealloc);

--- hurd/hurd/auth_reply.defs	Mon Jan 24 23:38:38 1994
+++ /mnt/marcus/gnu/hurd/hurd/hurd-20010610/hurd/auth_reply.defs	Wed Jul 11 20:44:38 2001
@@ -1,5 +1,5 @@
 /* Reply-only side of auth interface
-   Copyright (C) 1991, 1993, 1994 Free Software Foundation
+   Copyright (C) 1991, 1993, 1994, 2001 Free Software Foundation
 
 This file is part of the GNU Hurd.
 
@@ -37,8 +37,7 @@
 simpleroutine auth_server_authenticate_reply (
 	reply_port: reply_port_t;
 	in return_code: kern_return_t;
-	in gen_uids: idarray_t;
-	in aux_uids: idarray_t;
-	in gen_gids: idarray_t;
-	in aux_gids: idarray_t);
-	
+	in gen_uids: idarray_t, dealloc;
+	in aux_uids: idarray_t, dealloc;
+	in gen_gids: idarray_t, dealloc;
+	in aux_gids: idarray_t, dealloc);

diff -pru hurd/auth/auth.c /mnt/marcus/gnu/hurd/hurd/hurd-20010610/auth/auth.c
--- hurd/auth/auth.c	Mon Feb 12 22:39:42 2001
+++ /mnt/marcus/gnu/hurd/hurd/hurd-20010610/auth/auth.c	Thu Jul 12 21:09:31 2001
@@ -27,6 +27,7 @@
 #include <hurd/ports.h>
 #include <hurd/ihash.h>
 #include <idvec.h>
+#include <sys/mman.h>
 #include <assert.h>
 #include <argp.h>
 #include <error.h>
@@ -82,18 +83,30 @@ auth_port_to_handle (auth_t auth)
 
 /* id management.  */
 
-inline void idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
+inline int idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
 {
   if (idvec->num > *nids)
-    *ids = idvec->ids;
-  else
-    memcpy (*ids, idvec->ids, idvec->num * sizeof *ids);
+    {
+      *ids = mmap (0, idvec->num * sizeof (**ids), PROT_READ|PROT_WRITE,
+		   MAP_ANON, 0, 0);
+      if (*ids == (uid_t *) -1)
+	return errno;
+    }
+  memcpy (*ids, idvec->ids, idvec->num * sizeof (**ids));
   *nids = idvec->num;
+
+  return 0;
 }
 
-#define C(auth, ids)	idvec_copyout (&auth->ids, ids, n##ids)
-#define OUTIDS(auth)	(C (auth, euids), C (auth, egids), \
-			 C (auth, auids), C (auth, agids))
+#define C(auth, ids)	if (! err) \
+			  err = idvec_copyout (&auth->ids, ids, n##ids)
+#define OUTIDS(auth)	{ C (auth, euids); C (auth, egids); \
+			  C (auth, auids); C (auth, agids); }
+#define D(ids)	*ids##_saved = *ids
+#define SAVEIDS	uid_t D (euids), D (egids), D (auids), D (agids)
+#define F(ids)	if (*ids != ids##_saved) \
+		  munmap (*ids, *n##ids * sizeof (uid_t))
+#define FREEIDS { F (euids); F (egids); F (auids); F (agids); }
 
 /* Implement auth_getids as described in <hurd/auth.defs>. */
 kern_return_t
@@ -107,12 +120,17 @@ S_auth_getids (struct authhandle *auth,
 	       uid_t **agids,
 	       u_int *nagids)
 {
+  int err = 0;
+  SAVEIDS;
+
   if (! auth)
     return EOPNOTSUPP;
-
+  
   OUTIDS (auth);
-
-  return 0;
+  if (err)
+    FREEIDS;
+      
+  return err;
 }
 
 /* Implement auth_makeauth as described in <hurd/auth.defs>. */
@@ -358,8 +376,10 @@ S_auth_server_authenticate (struct authh
 			    uid_t **agids,
 			    u_int *nagids)
 {
+  error_t err = 0;
   struct pending *u;
   struct authhandle *user;
+  SAVEIDS;
 
   if (! serverauth)
     return EOPNOTSUPP;
@@ -392,7 +412,6 @@ S_auth_server_authenticate (struct authh
       /* No pending user RPC for this port.
 	 Create a pending server RPC record.  */
       struct pending s;
-      error_t err;
 
       err = ihash_add (pending_servers, rendezvous, &s, &s.locp);
       if (! err)
@@ -418,17 +437,13 @@ S_auth_server_authenticate (struct authh
       user = s.user;
     }
 
-  /* Extract the ids.  We must use a separate reply stub so
-     we can deref the user auth handle after the reply uses its
-     contents.  */
-  auth_server_authenticate_reply (reply, reply_type, 0,
-				  user->euids.ids, user->euids.num,
-				  user->auids.ids, user->auids.num,
-				  user->egids.ids, user->egids.num,
-				  user->agids.ids, user->agids.num);
+  OUTIDS (user);
+  if (err)
+    FREEIDS;
+
   ports_port_deref (user);
   mach_port_deallocate (mach_task_self (), rendezvous);
-  return MIG_NO_REPLY;
+  return err;
 }
 
 

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Roland McGrath <roland@gnu.org>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #75 received at 102437@bugs.debian.org (full text, mbox):

From: Roland McGrath <roland@gnu.org>
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
Cc: 102437@bugs.debian.org, "Thomas Bushnell, BSG" <tb@becket.net>
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Thu, 12 Jul 2001 16:34:45 -0400 (EDT)
> On Wed, Jul 11, 2001 at 04:49:17PM -0400, Roland McGrath wrote:
> > That's fine by me if it's tested.  BTW add -p to your diff switches.
> 
> Actually, I goofed up.  The arguments to the reply stub have not been
> changed by my patch.  In fact, as we now copy the user data all the time, a
> seperate reply stub is not necessary anymore!  There is also a bug
> somewhere.
> 
> I will work out something better.

I didn't look closely, but now I am thinking twice.  If the data is small
enough to fit inline, and you use a separate reply stub, then you can send
it without copying.  That ought to be the common case.



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #80 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: Roland McGrath <roland@gnu.org>
Cc: 102437@bugs.debian.org, "Thomas Bushnell, BSG" <tb@becket.net>
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Thu, 12 Jul 2001 23:02:16 +0200
On Thu, Jul 12, 2001 at 04:34:45PM -0400, Roland McGrath wrote:
> > On Wed, Jul 11, 2001 at 04:49:17PM -0400, Roland McGrath wrote:
> > > That's fine by me if it's tested.  BTW add -p to your diff switches.
> > 
> > Actually, I goofed up.  The arguments to the reply stub have not been
> > changed by my patch.  In fact, as we now copy the user data all the time, a
> > seperate reply stub is not necessary anymore!  There is also a bug
> > somewhere.
> > 
> > I will work out something better.
> 
> I didn't look closely, but now I am thinking twice.  If the data is small
> enough to fit inline, and you use a separate reply stub, then you can send
> it without copying.  That ought to be the common case.

Yes, we can save one copy this way.  This is actually true in all
places where we return small amounts of data in an array.  It looks a bit
like micro-optimization, but I can do the change, null problemo.  It just
requires a different OUTIDS (idvec_copyout) function in
auth_server_authenticate, which sets *ids to user->ids.ids if the number
is small (and provides *ids and *n##ids to the reply stub).

Want me to do this?

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Information forwarded to debian-bugs-dist@lists.debian.org, GNU Hurd Maintainers <bug-hurd@gnu.org>:
Bug#102437; Package hurd. Full text and rfc822 format available.

Acknowledgement sent to Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>:
Extra info received and forwarded to list. Copy sent to GNU Hurd Maintainers <bug-hurd@gnu.org>. Full text and rfc822 format available.

Message #85 received at 102437@bugs.debian.org (full text, mbox):

From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: Roland McGrath <roland@gnu.org>, 102437@bugs.debian.org
Cc: "Thomas Bushnell, BSG" <tb@becket.net>
Subject: Re: Bug#102437: dealloc analysis for each server function, more bugs!
Date: Thu, 12 Jul 2001 23:13:16 +0200
On Thu, Jul 12, 2001 at 04:34:45PM -0400, Roland McGrath wrote:
> I didn't look closely, but now I am thinking twice.  If the data is small
> enough to fit inline, and you use a separate reply stub, then you can send
> it without copying.  That ought to be the common case.

This would be what you want.  I can't say I like it.  Maybe if we were to
use a seperate reply stub in auth_getids as well, we might use the same code
again.  Otherwise I am afraid we have two slightly different versions of the
macros.  (The argument why to use a seperate reply stub for auth_getids is
the same as for auth_server_authenticate, we can avoid a copy).

Thanks,
Marcus

--- auth.c	Thu Jul 12 22:52:53 2001
+++ auth.c.new	Thu Jul 12 23:11:05 2001
@@ -27,6 +27,7 @@
 #include <hurd/ports.h>
 #include <hurd/ihash.h>
 #include <idvec.h>
+#include <sys/mman.h>
 #include <assert.h>
 #include <argp.h>
 #include <error.h>
@@ -82,18 +83,57 @@ auth_port_to_handle (auth_t auth)
 
 /* id management.  */
 
-inline void idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
+inline int idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
 {
   if (idvec->num > *nids)
-    *ids = idvec->ids;
+    {
+      *ids = mmap (0, idvec->num * sizeof (**ids), PROT_READ|PROT_WRITE,
+		   MAP_ANON, 0, 0);
+      if (*ids == (uid_t *) -1)
+	return errno;
+    }
+  memcpy (*ids, idvec->ids, idvec->num * sizeof (**ids));
+  *nids = idvec->num;
+
+  return 0;
+}
+
+#define C(auth, ids)	if (! err) \
+			  err = idvec_copyout (&auth->ids, ids, n##ids)
+#define OUTIDS(auth)	{ C (auth, euids); C (auth, egids); \
+			  C (auth, auids); C (auth, agids); }
+#define D(ids)	*ids##_saved = *ids
+#define SAVEIDS	uid_t D (euids), D (egids), D (auids), D (agids)
+#define F(ids)	if (*ids != ids##_saved) \
+		  munmap (*ids, *n##ids * sizeof (uid_t))
+#define FREEIDS { F (euids); F (egids); F (auids); F (agids); }
+
+/* In auth_server_authenticate, we use a reply stub to avoid copying
+   small inlined data twice.  */
+inline int idvec_copyout_reply (struct idvec *idvec, uid_t **ids, uid_t *nids)
+{
+  if (idvec->num > *nids)
+    {
+      *ids = mmap (0, idvec->num * sizeof (**ids), PROT_READ|PROT_WRITE,
+		   MAP_ANON, 0, 0);
+      if (*ids == (uid_t *) -1)
+	return errno;
+      memcpy (*ids, idvec->ids, idvec->num * sizeof (**ids));
+    }
   else
-    memcpy (*ids, idvec->ids, idvec->num * sizeof *ids);
+    *ids = idvec->ids;
+
   *nids = idvec->num;
+  return 0;
 }
 
-#define C(auth, ids)	idvec_copyout (&auth->ids, ids, n##ids)
-#define OUTIDS(auth)	(C (auth, euids), C (auth, egids), \
-			 C (auth, auids), C (auth, agids))
+#define C_R(auth, ids)	if (! err) \
+			  err = idvec_copyout_reply (&auth->ids, ids, n##ids)
+#define OUTIDS_R(auth)	{ C_R (auth, euids); C_R (auth, egids); \
+			  C_R (auth, auids); C_R (auth, agids); }
+#define F_R(idsr)	if (*idsr != user->idsr.ids) \
+			  munmap (*idsr, *n##idsr * sizeof (uid_t))
+#define FREEIDS_R { F_R (euids); F_R (egids); F_R (auids); F_R (agids); }
 
 /* Implement auth_getids as described in <hurd/auth.defs>. */
 kern_return_t
@@ -107,12 +147,17 @@ S_auth_getids (struct authhandle *auth,
 	       uid_t **agids,
 	       u_int *nagids)
 {
+  int err = 0;
+  SAVEIDS;
+
   if (! auth)
     return EOPNOTSUPP;
-
+  
   OUTIDS (auth);
-
-  return 0;
+  if (err)
+    FREEIDS;
+      
+  return err;
 }
 
 /* Implement auth_makeauth as described in <hurd/auth.defs>. */
@@ -358,6 +403,7 @@ S_auth_server_authenticate (struct authh
 			    uid_t **agids,
 			    u_int *nagids)
 {
+  error_t err = 0;
   struct pending *u;
   struct authhandle *user;
 
@@ -392,7 +438,6 @@ S_auth_server_authenticate (struct authh
       /* No pending user RPC for this port.
 	 Create a pending server RPC record.  */
       struct pending s;
-      error_t err;
 
       err = ihash_add (pending_servers, rendezvous, &s, &s.locp);
       if (! err)
@@ -418,17 +463,21 @@ S_auth_server_authenticate (struct authh
       user = s.user;
     }
 
-  /* Extract the ids.  We must use a separate reply stub so
-     we can deref the user auth handle after the reply uses its
-     contents.  */
+  OUTIDS_R (user);
+  if (err)
+    FREEIDS_R;
+
+  /* Extract the ids.  We use a separate reply stub to avoid copying
+     small data twice.  For this to work we must keep the user reference
+     until the reply stub did its work.  */
   auth_server_authenticate_reply (reply, reply_type, 0,
-				  user->euids.ids, user->euids.num,
-				  user->auids.ids, user->auids.num,
-				  user->egids.ids, user->egids.num,
-				  user->agids.ids, user->agids.num);
+                                  *euids, *neuids,
+                                  *auids, *nauids,
+                                  *egids, *negids,
+                                  *agids, *nagids);
   ports_port_deref (user);
   mach_port_deallocate (mach_task_self (), rendezvous);
-  return MIG_NO_REPLY;
+  return err;
 }
 
 

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de



Changed Bug title. Request was from Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de> to control@bugs.debian.org. Full text and rfc822 format available.

Tags added: upstream Request was from Samuel Thibault <samuel.thibault@ens-lyon.org> to control@bugs.debian.org. Full text and rfc822 format available.

Noted your statement that Bug has been forwarded to https://savannah.gnu.org/bugs/index.php?func=detailitem&item_id=15322. Request was from Samuel Thibault <samuel.thibault@ens-lyon.org> to control@bugs.debian.org. Full text and rfc822 format available.

Send a report that this bug log contains spam.


Debian bug tracking system administrator <owner@bugs.debian.org>. Last modified: Wed Apr 16 04:43:49 2014; Machine Name: buxtehude.debian.org

Debian Bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.