Bug #5679
closed
be_sort_list(): Possible null pointer dereference
100%
Description
In a case the realloc() fails, we will crash at line 649 in be_sort_list():
646 for (p = *pstart, nbe = 0; p != NULL; nbe++, p = p->be_next_node) {
647 ptrlist = realloc(ptrlist,
648 sizeof (be_node_list_t *) * (nbe + 2));
649 ptrlist[nbe] = p;
650 }
Related issues
Updated by Gary Mills over 8 years ago
Is there any reason to fix this bug? The realloc() call on that line is unlikely to fail because it extends the space by only one pointer. If it did fail, the subsequent malloc() would also fail. I can devise a change to prevent a core dump if realloc() failed, but is there any reason to do so?
Updated by Marcel Telka over 8 years ago
Gary Mills wrote:
Is there any reason to fix this bug? The realloc() call on that line is unlikely to fail because it extends the space by only one pointer. If it did fail, the subsequent malloc() would also fail. I can devise a change to prevent a core dump if realloc() failed, but is there any reason to do so?
I see the main reason to fix this in a possibility to kill the bad habits with the realloc() usage. I believe we have a lot of similar pieces of the code through the gate, so it would be great to have some kind of project "review/fix all the realloc() callers" so the new code writers are not inspired by them.
Updated by Gary Mills over 8 years ago
As a start, I've reviewed all the illumos source, looking for instances where a realloc() failure is not handled by the code. I found 45 such files. These are listed in bug #5970 . One of them is the file in question here.
Updated by Marcel Telka over 8 years ago
- Is duplicate of Bug #5970: Many source files don't handle realloc() failure added
Updated by Gary Mills over 8 years ago
I can confirm that beadm does dump core in be_sort_list() whenever realloc() returns NULL. I tested this in Solaris 11.2, oi_151a9, and a recent build of illumos-gate. All of them dumped core.
Updated by Marcel Telka over 8 years ago
- Status changed from Closed to In Progress
- Assignee set to Gary Mills
Updated by Electric Monk over 8 years ago
- Status changed from In Progress to Closed
- % Done changed from 0 to 100
git commit 0afb687bf3724077cd5f304f176b5ee2b92aa2c2
commit 0afb687bf3724077cd5f304f176b5ee2b92aa2c2
Author: Gary Mills <gary_mills@fastmail.fm>
Date: 2015-06-12T03:15:33.000Z
5679 be_sort_list(): Possible null pointer dereference
Reviewed by: Garrett D'Amore <garrett@damore.org>
Reviewed by: Marcel Telka <marcel@telka.sk>
Reviewed by: Toomas Soome <tsoome@me.com>
Approved by: Gordon Ross <gwr@nexenta.com>