Thu 29 Dec 2005 09:26:37 PM UTC, original submission:
From http://bugs.debian.org/102437
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: -unavailable-
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
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: -unavailable-
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.
See prog1.c
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: -unavailable-
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
From: Roland McGrath <roland@gnu.org>
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>,
-unavailable-
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.
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: -unavailable-
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
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: -unavailable-
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
From: -unavailable- (Thomas Bushnell, BSG)
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
Cc: -unavailable-
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.
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: -unavailable-
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
From: -unavailable- (Thomas Bushnell, BSG)
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
Cc: -unavailable-
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.
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: "Thomas Bushnell, BSG" <tb@becket.net>, -unavailable-
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
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: "Thomas Bushnell, BSG" <tb@becket.net>, -unavailable-
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
See patch1
From: Roland McGrath <roland@gnu.org>
To: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>,
-unavailable-
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.
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: Roland McGrath <roland@gnu.org>, -unavailable-
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
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: Roland McGrath <roland@gnu.org>, -unavailable-
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
See patch2
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.
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
Message received at -unavailable- (full text, mbox):
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: Roland McGrath <roland@gnu.org>, -unavailable-
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
See patch3
|