Coverity -- Explicit null dereference

Question

I have a scenario like this:

int main() {
  int *p;
  int *q;
  bool cond1, cond2;

  // Does some processing and sets the cond1 and cond2

  if (cond1) {
     p = // Assign valid address
     q = NULL;
  } else {
     p = NULL;
     q = // Assign valid address
  }

  // Does something else but cond1 and cond2 remains untouched  

  if (cond2) {
    ***// Using 'q' data members.***
  }
}

There are only two condidtions in my code, cond1 and cond2. First if executes for cond1 and else executes for cond2.. Only one of them could be true at a time. I see coverity defect with bold/italic code. Coverity complains below message:

CID 25469 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
9. var_deref_op: Dereferencing null pointer q.

I do not understand why coverity complains here. In this scenario, by the time, I come in 'cond2', I already have 'q' set. Right? What is it that I did not understand?

Solutions I propose:

.. Would it be ok if I write !cond1 just like this:

if (!cond1) {
  // Using 'q' data members.
}

.. Would it be ok if I add extra checks:

if (cond2 && q != NULL) {
  // Using 'q' data members.
}

.. Is it false positive?

Anything else? Thank you in advance.

So what if `cond1 && cond2 == true`? At least from the code you show, and apparently for coverty too, it is not clear why that can't happen. — Baum mit Augen, Apr 30 '18 at 09:37
Asside - I would suggest initialising your pointers to `nullptr` and then just not touching them if not required in the cond1 test — UKMonkey, Apr 30 '18 at 09:45
It's impossible to tell from that code whether `cond2` being true implies that `q` is non-null. — molbdnilo, Apr 30 '18 at 09:48
@BaummitAugen, Forgot to mention that there are only two conditions, cond1 and cond2. If block is for cond1 and else block would be for cond2. — Hemant Bhargava, Apr 30 '18 at 10:13
@Jarod42, Edited the question. Forgot to mention that there are only two conditions available. cond1 and cond2. cond1 for if block and cond2 for else block. — Hemant Bhargava, Apr 30 '18 at 10:15
@molbdnilo, I have answered your question after editing and in comments too. — Hemant Bhargava, Apr 30 '18 at 10:16
As you've written your code, you're assuming `cond2 == !cond1`. If `cond2` and `cond1` are both `true`, the `if (cond2)` test can result in dereferencing a null pointer. In short: Coverity is complaining for good reason, unless the code (that you haven't shown) ensures `cond2 == !cond1`. — Peter, Apr 30 '18 at 10:17
@HemantBhargava - you may know that, but the code you have shown us does not ensure that. And Coverity will tend to interpret your code rather than reading your mind. — Peter, Apr 30 '18 at 10:25
@Peter.. :) :) Agreed, I already edited the question and wrote a comment that only one of them would be true at a time. — Hemant Bhargava, Apr 30 '18 at 10:27
@HemantBhargava - that's fine, but unless the comments in your code are annotations that Coverity understands, Coverity is right to complain. It's not a false positive. It's a sign that your code structure ASSUMES a relationship between `cond1` and `cond2`, rather than ensuring one. — Peter, Apr 30 '18 at 10:30

score 0 · Answer 1 · answered Apr 30 '18 at 10:17

Logically, based on what you've said, and assuming that code1 and code2 are mutually exclusive, this is a false positive. However, the warning has value because it is pointing [sic] out that the safety of q is not trivially obvious from your function's logic.

I would, at the very least, put an assert(q) inside if (code2), but:

assertions usually only apply at debug-time (as I was recently reminded with a runtime crash in a release build that made no sense — after all, I have assertion safety everywhere, right?)
it may still not be enough to satisfy Coverity.

Ideally you'd stick with that one if/else and put all your logic in there. Common logic to perform in the middle? Separate it into a different function that you can call.

The added benefit of this is that you probably no longer require both p and q and so the entire function becomes far more lean.

If code1 and code2 may both be true, though, then you do have a substantial bug here.

Coverity -- Explicit null dereference

1 Answers1