Date: Wed, 18 Apr 2001 22:07:25 +0000 (GMT) From: Terry Lambert <tlambert@primenet.com> To: arch@freebsd.org Subject: Found BAD BUG: squashed Message-ID: <200104182207.PAA11716@usr08.primenet.com>
next in thread | raw e-mail | index | archive | help
I have identified the bug.
It turns out that the bug causes memory corruption by freeing
but continuing to use a credential, and it only occurs in big
resource usage cases, and then seemingly at random.
It is the fact that there are two credentials per socket, one
for the socket, and one for the descriptor itself.
The cr_ref is an unsigned short, and ...
There is no test for overflow.
Thus if I open 65537 files, and close 2, my cred structure is
freed.
If invariants are on, "0xdeadc0de" is spammed over top of the
reference count, and then each time the cread is released, it
is decremented: "0xdeadc0dd", "0xdeadc0dc", "0xdeadc0db", ...
...until another "0xc0de" (49374) closes, at which point it
frees the already freed credential _again_.
The way FreeBSD code is currently structured, this means that
the quick-and-ugly fix is simply to change this from u_short
to u_long. Further, I moved the cr_ref below the cr_groups
declartion -- well out of stomping range of the INVARIANTS
code. I have successfully opened 60,000 socket connections
(240,000 credential references between the client and server
processes combined, and 120,000 each) with this change.
--
The real fix is to do as I suggested on -arch earlier today
(and then some), and change:
crhold(curproc->p_ucred);
sigio->sio_ucred = curproc->p_ucred;
style things into:
sigio->sio_ucred = crhold(curproc->p_ucred);
style things.
But that is only part of it. Additionally, it needs to be
further changed to something like:
sigio->sio_ucred = crhold(&curproc->p_ucred);
-- passing the address of the credential pointer -- and then
(pseudocode), the credential code in crhold() needs to check
for an imminent overflow, and when it sees it:
struct ucred *
crhold(struct ucred **cpp)
{
struct ucred *ret;
if( about_to_overflow) {
ret = *cpp;
*cpp = crdup(save);
} else {
ret = *cpp;
ret ->cr_ref++;
}
return ret;
}
The point of doing this would be to give the FD the overflowing
cred, so that new files opened by the process can start counting
at 1 again.
NOTE: I also suggest chagning free() in kern/kern_malloc.c in
free() in the INVARIANTS case, from:
---
#ifdef INVARIANTS
/*
* Check for multiple frees. Use a quick check to see if
* it looks free before laboriously searching the freelist.
*/
if (freep->spare0 == WEIRD_ADDR) {
fp = (struct freelist *)kbp->kb_next;
while (fp) {
if (fp->spare0 != WEIRD_ADDR)
panic("free: free item %p modified", fp);
else if (addr == (caddr_t)fp)
panic("free: multiple freed item %p", addr);
fp = (struct freelist *)fp->next;
}
}
---
#ifdef INVARIANTS
/*
* Check for multiple frees. We can't use a quick check, since
* that will give us a false negative for something like a
* stomped reference counter or mutex, if it is the first item.
*/
fp = (struct freelist *)kbp->kb_next;
while (fp) {
if (fp->spare0 != WEIRD_ADDR)
panic("free: free item %p modified", fp);
else if (addr == (caddr_t)fp)
panic("free: multiple freed item %p", addr);
fp = (struct freelist *)fp->next;
}
---
Terry Lambert
terry@lambert.org
---
Any opinions in this posting are my own and not those of my present
or previous employers.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200104182207.PAA11716>