Date: Fri, 13 Sep 2019 08:33:31 +0200
From: Thomas Klausner <t...@giga.or.at>
Message-ID: <20190913063330.jl6qb35ifszulh3z@danbala>
| I'm sorry, I totally do not get it the problem with -- in general --
| writing the code in such a way that it properly frees any allocations
| it made.
There is no problem with doing that, if that's your thing, the problem
is requiring it be that way for memory that cannot be freed before the
program exits - exit() incldudes free(everything), requiring that be done
in advance for no particularly good reason is unjustifiable.
| I suspect there will be corner cases where it will be really hard, and
| we can discuss those separately. But discussing about adding a free()
| in ps? (If it was done incorrectly, let's fix it.)
|
| Please enlighten me.
It turns out that dealing with ps (which of itself is not a particularly
interesting case) is harder than it looks.
As I noted in a previous message, the *pinfo data struct which started all
of this is a an array of struct pinfo. Freeing that array is as simple as
the free(pinfo) that Kamil added. However each of those structs (may)
contain 3 pointers (plus some non-interesting for this purpose int type stuff).
One of those is a char * (prefix) - this one is only ever used if ps is run
with the -d option, which my guess is was not done in the LSan tests (I doubt
it is used all that often compared with the number of times ps is used).
So, usually that pointer will be NULL, with nothing to free. But if -d
is used there will be lots of strings "leaked" if we don't go free them all,
but do free the pinfo that contains the pointers to them.
Another is a struct kinfo_proc2 * (ki) ... initially I thought that this would
be a separate allocation for each pinfo, but on closer inspection it turns
out it isn't.
Rather we have:
kinfo = getkinfo_kvm(kd, what, flag, &nentries)
which returns an array of struct kinfo_proc2 (and the size of the
array in nentries) - and then we take pointers to each element of
that array and assign those to the ki fields of the pinfo (which has
nentries elements in its array - one for each struct kinfo_proc2 in
the kinfo array).
So, we definitely do not free each pinfo[n].ki - but rather after
freeing pinfo we should also free(kinfo).
If only it were so simple... the kinfo array comes from livkvm, which
from a brief glance is somewhat cavalier with its memory allocation
strategy (Kamil: if you are looking for memory leaks to fix, go experiment
with libkvm, you are likely find a lot - but it might take code reading
to do it - the actual leaks are mostly in error cases from what I can see,
and in normal usage those won't happen.)
What makes it harder, is that livkvm can use either sysctl (which tends to
return flat structures, not containing pointers, for the obvious reason) or
/dev/mem (or /dev/kmem, or even a kcore file probably) and which seems to
be different, but I did not dig deep enough to work out just what is
happening.
getkinfo_kvm() is just a wrapper around kvm_getproc2() - but neither
kvm_getprocs(3) nor the sources say exactly how one is supposed to
free the results. There is no kvm_freeprocs() that we could call.
kem_getprocs(3) does say (but of kvm_getargv())
The memory allocated to the argv pointers and string storage is
owned by the kvm library.
which suggests that the kvm library does not want its users arbitrarily
calling free() on the data it returns.
Instead there is kvm_close() which does a lot of free() calls, which ps
also never calls (it does call kvm_openfiles() however). Whether a
kvm_close() would free everything allocated by libkvm I am not sure (it
looks as if it tries to make that happen).
The third pointer in the struct pinfo is a struct kinfo_lwp * (li) - which
I did not look into at all, but that's fairly obviously more kvm data.
But wait ... it turns out there is more, ps also does
if (argc > 1)
argv[1] = kludge_oldps_options(argv[1]);
kludge_oldps_options() malloc's space in which to rewrite the
options from ancient unix sytle ("ps axt") into the new getopt()
form ("ps -axT") ('t' into 'T' as there are no optional options
in getopt).
That string is never freed either (it could be, any time after getopt()
has finished process it).
With all of this, there are still no actual problems (at least in this
area) with ps, it does its thing, and exits, and everything it allocated
is freed. Is it really worth someone chasing down everything that is
going on here for no better reason that some abstract ideal of code purity ?
kre
