Better to use an OR or an AND when checking for NULLs in C if statements?

Question

Came across a line in OpenSSL that made me do a double take...

if (!*pos)
  return NULL;
if (!*pos || ((*pos)->flags == FLAGS))
  return blah;

Is there a (performance/security/concurrecy) difference in doing that instead of:

if (!*pos) 
  return NULL;
if (*pos && ((*pos)->flags == FLAGS))
  return blah;

Thanks, Chenz

Answer 1

Why not:

if (!*pos) 
  return NULL;
if ((*pos)->flags == FLAGS)
  return blah;

The original code (comments removed) is:

if (!pos)
    return NULL;
if (!*pos)
    return BIO_new(BIO_s_null());
if (!*pos || ((*pos)->flags == ASN1_STRING_FLAG_CONT))
    return BIO_new(BIO_s_mem());
return BIO_new_mem_buf((*pos)->data, (*pos)->length);

If the tests were for some dubious security reason, shouldn't the third test be:

if ( pos && *pos && ((*pos)->flags == ASN1_STRING_FLAG_CONT))

which of course gets ridiculous the further into the function you get. Also, a cursory inspection reveals that this style is not used elsewhere in the file, so I think we can write this off to a minor programmers boo-boo.

Answer 2

Ignoring the first two lines of each which make the test redundant, the two forms don't do the same thing.

if (!*pos || ((*pos)->flags == FLAGS))
   // reached if *pos is NULL or the flags are set
   return blah;

if (*pos && ((*pos)->flags == FLAGS))
   // reached if *pos is not NULL and the flags are set 
   return blah;

The || form would cause the function to return blah if it was reached with *pos is NULL.

(since the code follows a statement which makes the function return NULL if *pos is NULL the test is probably redundant, assuming pos points at normal memory rather than a volatile)

Answer 3

Seems like the first line obsoletes the first condition of the second, but it's still a good practice to leave checks for not-null before dereferencing, in case earlier code is ever removed, or if code that modifies pos (sets it to NULL) is ever placed between these sections. (one significant disadvantage of return in the middle of a function here...)

Other than that, there should be no difference with any optimizing compiler, but the code may convey the idea better: return blah either if *pos is NULL, or if flags are FLAGS; despite earlier condition that singles out pos==NULL, in this place of execution logic may allow it to be NULL as well and perform the return as described instead of skipping this place.

Imagine:

 if (!*pos)
   return NULL;

  /* code added later here */
  if(foo) 
    pos = baz;   // added latter; baz can be NULL
  /* more code... */

 if (!*pos || ((*pos)->flags == FLAGS))
   return blah;

This will still behave correctly, control returned if pos==NULL, only different return.

Any change to the later condition will modify the behavior:

 if (*pos && ((*pos)->flags == FLAGS))
   return blah;
 crash();

Answer 4

Seeing such a code, efficiency is not the right question to ask. I don't like either of the versions. I'd go for

if (!*pos)
  return NULL;
else if((*pos)->flags == FLAGS)
  return blah;

Clean, unambiguous, efficient.