possible NULL && dereferencing NULL pointer

[Mark]

Hello

Consider the snippet (this is from the code base I have to maintain):

#define NAME_LEN 10
struct foo
{
char name[NAME_LEN];
unsigned int time;
/* more other fields */
};

struct master
{
struct foo *f;
unisgned int flags;
struct hw_callbacks *hw_cb;
};

struct master *p_master = NULL;
....
/* allocate storage for struct master and assign ptr to p_master */
....

if (! p_master->f && memcmp (p_master->f->name, "test", NAME_LEN))
{
return ERR;
}

(and I have such construct all over the code)

I'm not sure that memcmp() will not fail if p_master->bridge is NULL (i.e.
dereferencing NULL pointer), would it be more clear and correct to have:

if (! p_master->f)
return ERR;

if (memcmp(p_master->f->name, "test", NAME_LEN))
return ERR;

Thanks.

Mark

 

[Jorgen Grahn]

mem* and str* functions are generally undefined on null pointers. However it's
easy enough to build your own function on top of these that do handle nulls:

static inline int memorycmp(void *a, void *b, size_t n) {
return a==b ? 0 : !a ? -1 : !b ? 1 : memcmp(a, b, n);
}

extends memcmp to nulls, with the additional relations NULL==NULL and
NULL<nonnull. On the other hand if you want NULL=="",

static inline int memorycmp(void *a, void *b, size_t n) {
if (!a) a = "";
if (!b) b = "";
return memcmp(a, b, n);
}

You shouldn't be afraid of developping your own library on top of the std*
libraries to adapt them to your own notion of rightness.

I agree, but then IMHO you should (a) document the "notion of
rightness", e.g.

/* In Foobar, it's normal for Baz to be NULL, so we
* have helper functions which make it safe to compare a
* Baz.name in that case, as if a NULL Baz had a "" name.
*/

And (b) choose a more narrow name than memorycmp(), which to me
implies memcmp(), which implies the semantics of memcmp().

/Jorgen

 

[Keith Thompson]

mem* and str* functions are generally undefined on null pointers. However it's
easy enough to build your own function on top of these that do handle nulls:

static inline int memorycmp(void *a, void *b, size_t n) {
return a==b ? 0 : !a ? -1 : !b ? 1 : memcmp(a, b, n);
}

extends memcmp to nulls, with the additional relations NULL==NULL and
NULL<nonnull. On the other hand if you want NULL=="",

static inline int memorycmp(void *a, void *b, size_t n) {
if (!a) a = "";
if (!b) b = "";
return memcmp(a, b, n);
}

You shouldn't be afraid of developping your own library on top of the std*
libraries to adapt them to your own notion of rightness.

Sure, but it's not clear to me that your memorycmp() is "right".

memcmp() compares the memory pointed to by one pointer to the memory
pointed to by another pointer. A null pointer doesn't point to
anything, so I'd argue that if either pointer argument is null,
the comparison just doesn't make sense. If you attempt to pass
one or two null pointers to memcmp(), it probably indicates a an
error in your program logic; hiding this error by quieting returning
a "sensible" value is not, IMHO, a good idea.

Mark, my advice is to fix your code's logic so it doesn't try to call
memcpy() with null arguments.

 

[gwowen]

Mark, my advice is to fix your code's logic so it doesn't try to call
memcpy() with null arguments.

But isn't a wrapper function a not-unreasonable way to do that?

I wouldn't recommend DwtCBD's particular level of clever-clever
obfuscation, but *at the very least* it changes undefined behaviour to
defined behaviour.

 

[Keith Thompson]

Ah. So you're advocating programming to someone else's libraries that are
decades old rather than coding what your own intentions. Got it.

No. I doubt that Mark's code, as posted, actually reflected his
intentions.

Here's the code in question:

if (! p_master->f && memcmp (p_master->f->name, "test", NAME_LEN))
{
return ERR;
}

It calls memcmp() *only* if p_master->f is a null pointer.
That's almost certainly a bug. It might not be there in Mark's
actual code, but it's in what he posted.

I advocate using the standard library when it's appropriate -- not
because it's obviously superior to any alternative, but because it's
standard.

Perhaps Mark would be better off developing his own library, but I see
no evidence of it so far.

Oh, and you might want to read the rest of what I wrote.

 

[Ben Bacarisse]

Consider the snippet (this is from the code base I have to maintain):

#define NAME_LEN 10
struct foo
{
char name[NAME_LEN];
unsigned int time;
/* more other fields */
};

struct master
{
struct foo *f;
unisgned int flags;
struct hw_callbacks *hw_cb;
};

struct master *p_master = NULL;
...
/* allocate storage for struct master and assign ptr to p_master */
...

if (! p_master->f && memcmp (p_master->f->name, "test", NAME_LEN))
{
return ERR;
}

(and I have such construct all over the code)

It's been suggested that this is a typo -- i.e. that the ! is there by
accident. That's possible, but the code is wrong in other ways that
make it hard to know what was intended.

memcmp(p_master->f->name, "test", NAME_LEN)

is undefined since "test" is not 10 bytes in size. The posted code is
probably a simplification of the original, but the original might have
more clues about what the "if" is supposed to do.

I'm not sure that memcmp() will not fail if p_master->bridge is NULL (i.e.
dereferencing NULL pointer), would it be more clear and correct to have:

if (! p_master->f)
return ERR;

if (memcmp(p_master->f->name, "test", NAME_LEN))
return ERR;

The original is wrong, but I can't say if this re-write is what is
intended. What you've written here is equivalent to

if (!p_master->f || memcmp(p_master->f->name, "test", 5))
return ERR;

but returning ERR for all strings except "test" looks odd to me.

It's possible that the original author thought that the ! applied to the
whole expression. I.e that the test should have been

if (!(p_master->f && memcmp (p_master->f->name, "test", 5)))
return ERR;

This is safe and returns ERR when either the pointer is null or the name
is the special value "test".

To avoid more guessing, what's the actual code?

 

[Malcolm McLean]

Here's the code in question:

    if (! p_master->f && memcmp (p_master->f->name, "test", NAME_LEN))
    {
       return ERR;
    }

It calls memcmp() *only* if p_master->f is a null pointer.
That's almost certainly a bug.  It might not be there in Mark's
actual code, but it's in what he posted.

Yes, that's pretty obviously a bug.
The OP needs to think when p_master->f can be null and what that
means. If it means that the program has an internal error, there's
usually no point in masking it by not doing the memcmp. If it is a
vlid state for the pointer, he's got to decide whether that means a
match, a non-match, or something different.

 

[Keith Thompson]

But isn't a wrapper function a not-unreasonable way to do that?

I wouldn't recommend DwtCBD's particular level of clever-clever
obfuscation, but *at the very least* it changes undefined behaviour to
defined behaviour.

It's impossible to tell without more information.

Changing undefined behavior to defined behavior is not useful if the
defined behavior is logically incorrect.

A wrapper around memcpy() that checks for null pointer arguments and
does something reasonable *might* be a good idea. But if the presence
of null pointers indicates a logical flaw in the code, then there is no
reasonable action to take (except perhaps to terminate the program with
an error message). In that case, the correct solution would be to fix
the program so it doesn't encounter that situation in the first place.

Without more information from Mark, I don't think any further guessing
is particularly constructive.

 

[Joe keane]

I'm not sure that memcmp() will not fail if p_master->bridge is NULL (i.e.
dereferencing NULL pointer), would it be more clear and correct to have:

In general, you don't pass NULL to a function that takes 'pointer to
so-and-so object', unless you have good evidence that it's allowed, and
it does what you want.

Otherwise, it will
a) crash,
or
b) do not what you want.

The 'strcmp' example is good. What does a NULL argument mean? Is it
the same thing as the string ""? Or are NULL and "" two different
things? Does NULL compare equal to "NULL"?