possible NULL && dereferencing NULL pointer

Discussion in 'C Programming' started by Mark, Jan 31, 2012.

  1. Mark

    Mark Guest

    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
    Mark, Jan 31, 2012
    #1
    1. Advertising

  2. Mark

    Jorgen Grahn Guest

    On Tue, 2012-01-31, Devil with the China Blue Dress wrote:
    > In article <jg7srm$ka8$>,
    > "Mark" <> wrote:
    >
    >> 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:

    >
    > 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

    --
    // Jorgen Grahn <grahn@ Oo o. . .
    \X/ snipabacken.se> O o .
    Jorgen Grahn, Jan 31, 2012
    #2
    1. Advertising

  3. Devil with the China Blue Dress <> writes:
    > In article <jg7srm$ka8$>,
    > "Mark" <> wrote:
    >
    >> 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:

    >
    > 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.

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    Will write code for food.
    "We must do something. This is something. Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"
    Keith Thompson, Jan 31, 2012
    #3
  4. Mark

    gwowen Guest

    On Jan 31, 8:28 am, Keith Thompson <> wrote:

    > 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.
    gwowen, Jan 31, 2012
    #4
  5. Devil with the China Blue Dress <> writes:
    > In article <>, Keith Thompson <>
    > wrote:
    >> Devil with the China Blue Dress <> writes:

    [snip]
    >> > 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.

    >
    > 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.

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    Will write code for food.
    "We must do something. This is something. Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"
    Keith Thompson, Jan 31, 2012
    #5
  6. "Mark" <> writes:

    > 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?

    --
    Ben.
    Ben Bacarisse, Jan 31, 2012
    #6
  7. On Jan 31, 10:31 am, Keith Thompson <> wrote:
    >
    > 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.
    Malcolm McLean, Jan 31, 2012
    #7
  8. gwowen <> writes:
    > On Jan 31, 8:28 am, Keith Thompson <> wrote:
    >> 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.


    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.

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    Will write code for food.
    "We must do something. This is something. Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"
    Keith Thompson, Jan 31, 2012
    #8
  9. Mark

    Joe keane Guest

    In article <jg7srm$ka8$>,
    Mark <> wrote:
    >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"?
    Joe keane, Feb 1, 2012
    #9
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. BigMan
    Replies:
    51
    Views:
    7,228
    Rolf Magnus
    Feb 17, 2005
  2. Angel Tsankov

    NULL pointer dereferencing?

    Angel Tsankov, Mar 2, 2006, in forum: C++
    Replies:
    5
    Views:
    700
  3. Lutz Richter

    Dereferencing a null-pointer allowed?

    Lutz Richter, May 8, 2007, in forum: C++
    Replies:
    7
    Views:
    421
    Sarath
    May 10, 2007
  4. Replies:
    15
    Views:
    833
    Kenneth Brody
    Apr 11, 2008
  5. Shao Miller

    C Standard Regarding Null Pointer Dereferencing

    Shao Miller, Jul 21, 2010, in forum: C Programming
    Replies:
    280
    Views:
    4,315
Loading...

Share This Page