Multiple possible null-pointer dereferences

[Daniil Berendeev]

Hello, its cppcheck guy again.

I'm digging through error messages, and there are lots of them related
to null pointer dereferences. But I'm not sure if those should be
considered as bugs and fixed. Maybe I'm missing a point?

Here are some common examples of how it looks like:

1) First snippet:
static int dbd_freetds_end_transaction(apr_dbd_transaction_t *trans)
{
int dummy;
if (trans) { // <-- Here we check whether trans is a valid pointer
// skipped irrelevant code
}

// But here we dereference is without a fuss.
return (trans->handle->err == SUCCEED) ? 0 : 1;
}

2) Second snippet:
static int dbd_oracle_end_transaction(apr_dbd_transaction_t *trans)
{
int ret = 1; /* no transaction is an error cond */
sword status;

// *** We dereference the pointer ***
apr_dbd_t *handle = trans->handle;
if (trans) { // <-- and check if it is valid after that, lol
//...

3) Third snippet

// *** Again, here we dereference the pointer ***
assert(stab->n_type != N_FUN || (iidescp->ii_type != II_GFUN &&
iidescp->ii_type != II_SFUN) || scope == 0);
//...
if (scope && stab->n_type != N_PSYM) {
if (iidescp) // <-- and here check if it's valid
iidesc_free(iidescp, NULL);


And there are tons (973 to be precise) of examples like these above.
Should those be considered as bugs and be fixed, or they are fine?
--
Cheers~

PGP key fingerprint:
07B3 2177 3E27 BF41 DC65 CC95 BDA8 88F1 E9F9 CEEF

You can retrieve my public key at pgp.mit.edu.

[Benjamin Kaduk]

Post by Daniil Berendeev
Hello, its cppcheck guy again.
I'm digging through error messages, and there are lots of them related
to null pointer dereferences. But I'm not sure if those should be
considered as bugs and fixed. Maybe I'm missing a point?

I only looked at the first one, but it looks like a bug.

But, all the pasted examples looked like they were or were likely to be in
contrib code, so fixing them is more tedious, as it has to go through the
upstream code owner and then get imported into FreeBSD.

-Ben

[Daniil Berendeev]

Post by Benjamin Kaduk
But, all the pasted examples looked like they were or were likely to be in
contrib code

Yes, the pasted examples are from contrib/ code, but similar code exists
in usr.sbin/, sys/, crypto/, lib/, libexec/, sbin/, just a few examples
from sys:

1) sys/boot/ficl/ficl.c:274
void ficlFreeVM(FICL_VM *pVM)
{
// Again, we at first dereference the pointer
FICL_SYSTEM *pSys = pVM->pSys;
FICL_VM *pList = pSys->vmList;

// And then check if it is valid
assert(pVM != 0);
// ...

2) sys/dev/iwn/if_iwn.c:6853
if (ss != NULL) { // we check if ss is valid
if (ss->ss_ssid[0].len != 0) {

// then some operations are performed over ss,
// but they are all done inside the if expression.
// Nothing is done in case ss == NULL.

// Then, a after a bunch of lines
// we do this (line 6933):
if (ss->ss_nssid > 0)
chan->flags |= htole32(IWN_CHAN_NPBREQS(1));

// Nothing is done with ss between the if() statement
// and the dereference



So, if these are actually bugs, I'd mark them as needed for fixing (as,
sometimes, it's not clear what should be done in the fail case and
should be better left up to the maintainer to decide) and send the
patches to the mailing list (among others).

[Warner Losh]

On Thu, Sep 15, 2016 at 12:36 PM, Daniil Berendeev

Post by Daniil Berendeev

Post by Benjamin Kaduk
But, all the pasted examples looked like they were or were likely to be in
contrib code

Yes, the pasted examples are from contrib/ code, but similar code exists
in usr.sbin/, sys/, crypto/, lib/, libexec/, sbin/, just a few examples
1) sys/boot/ficl/ficl.c:274
void ficlFreeVM(FICL_VM *pVM)
{
// Again, we at first dereference the pointer
FICL_SYSTEM *pSys = pVM->pSys;
FICL_VM *pList = pSys->vmList;
// And then check if it is valid
assert(pVM != 0);
// ...

While technically a bug, this bug would never be triggered given how
the boot loader works.

It's super easy to fix, so we might as well, but to be clear it will
zero affect on the actual runtime performance of the code give the
greater structure of the code.

Warner