1

I'm getting a warning while doing a Static Analysis (SA) on my code. I have simplified it below (with the first warning)-

typedef struct testStruct_ {
int *ptr;
} testStruct;

testStruct a;
testStruct *a_ptr;
a_ptr = &a;
a_ptr->ptr = NULL; #WARNING: Directly dereferencing pointer a_ptr.

The code goes on to do several operations with a_ptr. I've posted an example for the sake of completion-

rc = fn_a (filename, a_ptr);
rc = fn_b (a_ptr);
rc = fn_c (a_ptr->ptr);

fn_a is defined as-

fn_a (const char *filename, testStruct *a_ptr)
{
    a_ptr->ptr = fn_a_2(filename);
    if (!a_ptr->ptr) {
        ERR("Loading (%s) failed", filename);
        return (FALSE);
    }
    return (TRUE);
}

At one point later, I get another warning:

if (a_ptr && a_ptr->ptr) {
    freeFn(a_ptr->ptr);
}
#WARNING: Dereference before NULL check - NULL checking a_ptr suggests that it may be NULL, but it has already been dereferenced on all paths leading up to the check.

It seems like the line a_ptr->ptr = NULL is deemed incorrect/dangerous. Why is this error being shown and is there a way to correct it?

dbush
  • 205,898
  • 23
  • 218
  • 273
sbhatla
  • 1,040
  • 2
  • 22
  • 34
  • 3
    You need to show more code along with what static analysis tool you are using for us to be able to give an answer to you. – fuz Oct 13 '15 at 20:56
  • I'm using Coverity Connect 7.5. From my analysis, I felt the problem lay in this snippet, and hence I stripped down the rest of the code. Could you explain where else you feel the problem may lie, and I could post that. – sbhatla Oct 13 '15 at 20:59
  • The error, basically, is in the line `a_ptr->ptr = NULL`, I believe. And that's where it's showing the warning. – sbhatla Oct 13 '15 at 21:00
  • 5
    If you put the code above in a c file, run the analysis, does it report the warning? Yes -> horrible false positive No -> You removed an important piece of information. – UmNyobe Oct 13 '15 at 21:03
  • 2
    My guess is it is telling you "you are doing the NULL check now, but you have already dereferenced it before. May be you need to do this null check much before – ramana_k Oct 13 '15 at 21:03
  • I'm not doing the NULL check. The SA tool does a NULL check later at some point, and realizes that it can possibly be NULL. However, at the point it checks, it deems that it has already been deferenced before, and hence issues the second warning. The first warning `Directly dereferencing pointer a_ptr` is issued as shown in the code above, for the line `a_ptr->ptr = NULL;` Also, in my question, I have written the exact words of the warning. – sbhatla Oct 13 '15 at 21:06
  • updating with the point where second error occurs. – sbhatla Oct 13 '15 at 21:10
  • 1
    @sbhatla Why do you test `a_ptr` for `NULL` after you dereference it? – fuz Oct 13 '15 at 21:13
  • *The code goes on to do several operations with a_ptr* -- I wonder what these operations are? – PaulMcKenzie Oct 13 '15 at 21:13
  • The if is correct but the tool probably doesn't recognize it. Try "if ((a_ptr != NULL) && (a_ptr->ptr != NULL))". Or, split the if into two as the tool may not understand the "short circuit" of "&&" in this context. Also, what does gcc -Wall say? – Craig Estey Oct 13 '15 at 21:20
  • 1
    I'd really love to know why I'm being downvoted. The question is concise, clear, to-the-point and logical. Only irrelevant information has been removed. I'll add that right now, but I feel it's pointless, since it will only clutter up the question. – sbhatla Oct 13 '15 at 21:21
  • Is `freeFn` a macro that contains a null check? – Barmar Oct 13 '15 at 21:24
  • 1
    @sbhatla All relevant information is missing from the question. "I felt the problem lay in this snippet" is useless. Your feelings don't help us guess the missing information. – melpomene Oct 13 '15 at 21:25
  • @sbhatla you're probably being downvoted [not me] because you're not posting enough code to make the solution obvious. Post the 50 or so lines, otherwise we're just guessing. – Craig Estey Oct 13 '15 at 21:25
  • 1
    You should make a minimal and **compilable** example. The tool might be referring to lines you left out. – Kninnug Oct 13 '15 at 21:28
  • Right now your question is saying "I got some warnings for my code. I've posted different code below, and I'm not telling you the exact warning I got. What's the problem?" – melpomene Oct 13 '15 at 21:30
  • @melpomene I have very clearly mentioned the exact warnings. The code is functionally the same, stripped of complicated function calls, variable names, and typedefs. Some of the code that falls in between the two warnings is buried deep under layers and layers of function calls, which I can't simplify. Most importantly, that is not where any warning is listed, so it's probably not important. Where the warnings are listed, I have mentioned them in the question with the code. – sbhatla Oct 13 '15 at 21:41
  • Also, updated with a simplified intermediate code snippet. – sbhatla Oct 13 '15 at 21:41
  • @sbhatla I'm not interested in "simplified" code, unless it produces the exact same warnings. As for "mentioned the exact warnings", are you telling me Coverity doesn't even tell you the filename / line number of potential issues? – melpomene Oct 13 '15 at 21:43
  • @melpomene If you feel that will help in any way, here is warning 1 - `deref_ptr: Directly dereferencing pointer vm_def_ptr.` And here is warning 2 - `CID 21726 : Dereference before null check (REVERSE_INULL) check_after_deref: Null-checking vm_def_ptr suggests that it may be null, but it has already been dereferenced on all paths leading to the check.` As mentioned earlier, the line for these warnings is where it's listed in the question. It's GUI based, so the warning appears right next to the line. – sbhatla Oct 13 '15 at 22:01

1 Answers1

7

Coverity is giving you a warning because you are in fact doing a NULL check:

if (a_ptr && a_ptr->ptr) {

Here, a_ptr is evaluated in a boolean context. It evaluates to true if a_ptr is not NULL.

This warning thrown by Coverity if you dereference a pointer and then later on do a NULL check on it. This means one of two things:

  • The pointer could in fact be NULL and any dereference prior to that NULL check could result in a NULL pointer dereference, so you need to either do the NULL check sooner or don't deereference at that point.
  • The NULL check is unnecessary because the pointer can't be NULL, so the NULL check should be removed.

In this particular case, you're explicitly setting a_ptr to the address of a variable, so it can't possibly be NULL at that point. If you don't set it again before the above if statement, that means that the NULL check is unnecessary and should be removed.

dbush
  • 205,898
  • 23
  • 218
  • 273
  • 1
    Thanks! The exact line that helped in your answer was this - "In this particular case, you're explicitly setting a_ptr to the address of a variable, so it can't possibly be NULL at that point. If you don't set it again before the above if statement, that means that the NULL check is unnecessary and should be removed." – sbhatla Oct 13 '15 at 21:45