Bug #1387

zero-size malloc and ps: kvm_getprocs: Bad address

Added by qhwt+dfly about 5 years ago. Updated about 5 years ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

Hello.
On recent -DEVELOPMENT ps command displays a non-intuitive error message
on a non-existent pid:
ps: kvm_getprocs: Bad address

Apparently the recent malloc reimplementation has changed malloc(3) family
so as malloc(0) now returns a pointer such that useracc() returns EFAULT
for it. kvm_getprocs() is one the functions affected by the new behavior
(no, I don't have the list of affected functions other than this
yet :). It used to return a non-NULL pointer and a 0 to *cnt, but now
it returns just NULL without affecting *cnt, so ps command displays the
error message. I think one way to fix is something like below (the fix
to the callers is taken from FreeBSD), but if we can restore the old
behavior of malloc(0), we don't need such fix.

%%%
diff --git a/bin/ps/ps.c b/bin/ps/ps.c
index 82e3e63..b66988a 100644
--- a/bin/ps/ps.c
+++ b/bin/ps/ps.c
@@ -385,7 +385,9 @@ main(int argc, char **argv)
/*
* select procs
*/
- if ((kp = kvm_getprocs(kd, what, flag, &nentries)) == NULL)
+ nentries = -1;
+ kp = kvm_getprocs(kd, what, flag, &nentries);
+ if ((kp == NULL && nentries > 0) || (kp != NULL && nentries < 0))
errx(1, "%s", kvm_geterr(kd));
if ((kinfo = malloc(nentries * sizeof(*kinfo))) == NULL)
err(1, NULL);
diff --git a/lib/libkvm/kvm_proc.c b/lib/libkvm/kvm_proc.c
index 1c39636..d0e388d 100644
--- a/lib/libkvm/kvm_proc.c
+++ b/lib/libkvm/kvm_proc.c
@@ -479,6 +479,10 @@ kvm_getprocs(kvm_t *kd, int op, int arg, int *cnt)
_kvm_syserr(kd, kd->program, "kvm_getprocs");
return (0);
}
+ if (size == 0) {
+ *cnt = 0;
+ return (0);
+ }
do {
size += size / 10;
kd->procbase = (struct kinfo_proc *)
diff --git a/usr.bin/fstat/fstat.c b/usr.bin/fstat/fstat.c
index 133fff7..7a8545d 100644
--- a/usr.bin/fstat/fstat.c
+++ b/usr.bin/fstat/fstat.c
@@ -241,7 +241,9 @@ main(int argc, char **argv)
if (kvm_nlist(kd, nl) != 0)
errx(1, "no namelist: %s", kvm_geterr(kd));
#endif
- if ((p = kvm_getprocs(kd, what, arg, &cnt)) == NULL)
+ cnt = -1;
+ p = kvm_getprocs(kd, what, arg, &cnt);
+ if ((p == NULL && cnt > 0) || (p != NULL && cnt < 0))
errx(1, "%s", kvm_geterr(kd));
if (nflg)
printf("USER %-*.*s %*.*s FD DEV %*.*s MODE SZ|DV R/W",
%%%

History

#1 Updated by corecode about 5 years ago

YONETANI Tomokazu wrote:
> Hello.
> On recent -DEVELOPMENT ps command displays a non-intuitive error message
> on a non-existent pid:
> ps: kvm_getprocs: Bad address
>
> Apparently the recent malloc reimplementation has changed malloc(3) family
> so as malloc(0) now returns a pointer such that useracc() returns EFAULT
> for it.

I think we should make malloc(0) return a valid pointer. Whether to
return NULL or a pointer is implementation specific, and I believe our
previous malloc, and most other mallocs out there indeed return a pointer
and don't return NULL.

cheers
simon

#2 Updated by joerg about 5 years ago

On Sun, May 24, 2009 at 10:26:24AM +0200, Simon 'corecode' Schubert wrote:
> I think we should make malloc(0) return a valid pointer.

For consistency, just round up malloc(0) to the minimal allocation size.
That avoids most problems with existing programs and keeps the spirit of
the standard.

Joerg

#3 Updated by corecode about 5 years ago

Matthew Dillon wrote:
> We currently return an invalid non-NULL pointer, ZERO_LENGTH_PTR,
> which I have set to ((void *)-8).
>
> We can change ZERO_LENGTH_PTR to be whatever we want, including
> making it a pointer to valid memory like the address of a dummy
> global.

The standard however demands a unique pointer to be returned - we can't
return a constant pointer. I think rounding up to the minimal size sounds
good.

cheers
simon

#4 Updated by dillon about 5 years ago

We currently return an invalid non-NULL pointer, ZERO_LENGTH_PTR,
which I have set to ((void *)-8).

We can change ZERO_LENGTH_PTR to be whatever we want, including
making it a pointer to valid memory like the address of a dummy
global.

It might be better to fix kvm_getprocs, though, to not try to do a
useracc test on a zero-length allocation. But I leave it up to you
guys.

-Matt
Matthew Dillon
<>

diff --git a/lib/libc/stdlib/nmalloc.c b/lib/libc/stdlib/nmalloc.c
index faa1e5c..29c1c4f 100644
--- a/lib/libc/stdlib/nmalloc.c
+++ b/lib/libc/stdlib/nmalloc.c
@@ -183,7 +183,7 @@ typedef struct slglobaldata {
*/
#define WEIRD_ADDR 0xdeadc0de
#define MAX_COPY sizeof(weirdary)
-#define ZERO_LENGTH_PTR ((void *)-8)
+#define ZERO_LENGTH_PTR ((void *)&nmalloc_dummy_storage)

#define BIGHSHIFT 10 /* bigalloc hash table */
#define BIGHSIZE (1 << BIGHSHIFT)
@@ -219,6 +219,7 @@ static struct slglobaldata SLGlobalData[SLGD_MAX];
static bigalloc_t bigalloc_array[BIGHSIZE];
static spinlock_t bigspin_array[BIGXSIZE];
static int malloc_panic;
+static int nmalloc_dummy_storage;

static const int32_t weirdary[16] = {
WEIRD_ADDR, WEIRD_ADDR, WEIRD_ADDR, WEIRD_ADDR,

#5 Updated by dillon about 5 years ago

:The standard however demands a unique pointer to be returned - we can't=20
:return a constant pointer. I think rounding up to the minimal size sound=
:s=20
:good.
:
:cheers
: simon

Sure, go ahead and make the change. I think that will actually reduce
the size of the code slightly since we would not longer have to deal
with the special pointer. Remember to deal with realloc properly as
well.

-Matt

Also available in: Atom PDF