Is it unsafe to take the address of a variable inside a loop where it is defined?

Viewed 121

I came across a comment in cppcheck source code here, stating that it is unsafe to move the definition of i in the example into the inner loop. Why is that?

void CheckOther::variableScopeError(const Token *tok, const std::string &varname)
 {
     reportError(tok,
                 Severity::style,
                 "variableScope",
                 "$symbol:" + varname + "\n"
                 "The scope of the variable '$symbol' can be reduced.\n"
                 "The scope of the variable '$symbol' can be reduced. Warning: Be careful "
                 "when fixing this message, especially when there are inner loops. Here is an "
                 "example where cppcheck will write that the scope for 'i' can be reduced:\n"
                 "void f(int x)\n"
                 "{\n"
                 "    int i = 0;\n"
                 "    if (x) {\n"
                 "        // it's safe to move 'int i = 0;' here\n"
                 "        for (int n = 0; n < 10; ++n) {\n"
                 "            // it is possible but not safe to move 'int i = 0;' here\n"
                 "            do_something(&i);\n"
                 "        }\n"
                 "    }\n"
                 "}\n"
                 "When you see this message it is always safe to reduce the variable scope 1 level.", CWE398, Certainty::normal);
 }
3 Answers

Why is that?

The following code outputs i=10:

#include <cstdio>

void save_life(int i) {
    printf("i=%d\n", i);
}

void do_something(int *i) {
    *i += 1;
    if (*i == 10) {
        save_life(*i);
    }
}

 void f(int x)
 {
    int i = 0;
    if (x) {
        for (int n = 0; n < 10; ++n) {
            do_something(&i);
        }
    }
 }

 int main() {
     f(1);
 }

However, the following code doesn't output anything:

#include <cstdio>

void save_life(int i) {
    printf("i=%d\n", i);
}

void do_something(int *i) {
    *i += 1;
    if (*i == 10) {
        save_life(*i);
    }
}

 void f(int x)
 {
    if (x) {
        for (int n = 0; n < 10; ++n) {
            int i = 0;
            do_something(&i);
        }
    }
 }

 int main() {
     f(1);
 }

Because do_something internal state may depend on the pointed-to-value, it's not "safe" to move the declaration in the loop. The function may depend on the state from previous iteration.

The question is focused on the second comment in this hypothetical code:

void f(int x)
    int i = 0;
    if (x) {
        // it's safe to move 'int i = 0;' here
        for (int n = 0; n < 10; ++n) {
             int i=0;
             // it is possible but not safe to move 'int i = 0;' here
             do_something(&i);
        }
    }
}

There is no general issue taking the address of a variable declared in a loop so long as that address (so taken) is not dereferenced after the loop.

Here's a code fragment similar to the question with variable moved inside the loop as per that comment.

int *p=nullptr;
for(int n=0;n<10;++n){
    int i=0; //Where the code in question says it's possible but not safe!
    p=&i; 
    //can use value of p here.
}
//address of i assigned to p now invalid.

This is the same general notion of variable scope that means pointers to local variables can't be used after the function that defined them returns(*). In this case in anything but the iteration the address was taken.

int *q=nullptr;
for(int n=0;n<10;++n){
    int i=0;
    if(n==0){
        q=&i;
    }else{
        //cannot use q here. 
        //That would using the address from the first iteration in a later iteration.
    }
}

So it's not clear what the original author's concern is. What is clear in the code as presented is i will be 0 when do_something(&i) is called every time and in the code as shown nothing is done with any value do_something() assigns to the variable through its pointer.

Sometimes it is necessary to provide an address to a general purpose function that the specific caller doesn't then need to use.

But the code provided isn't sufficient to explain the comment.

(*) For the same reasons that the implementation may have released or reused the memory for other purposes as it leaves the relevant scope.

Take the message The scope of the variable 'i' can be reduced. at face value. Do not add more meaning than is stated. A reduction is allowed, but no assertion is made as to how much the scope can be reduced. To play it safe, reduce as little as possible, then run the tool again to see if further reductions are possible.

If you accept this slow, non-optimized approach, the rest of the details in the comment can be safely ignored.


I think this question originates from taking the following statement out of context.

it is possible but not safe to move 'int i = 0;' here

Specifically, what does "safe" mean? When programmers talk about code being "safe", they most commonly mean some variation on "will not crash" or "no undefined behavior". This would explain why the question's title mentions "taking the address of", as dangling pointers are a common source of undefined behavior. However, there are other meanings of "safe" (as demonstrated in this question's opening section).

This statement was not made in a general context. The context is a code analysis tool (Cppcheck). Code analysis involves suggesting changes to code without deviating from intended functionality. In this context, "safe" means "not changing intended functionality". The danger is not a crash, but changing the meaning of the code. Moving a variable definition inside a loop often will change behavior, because after the move, the variable is re-initialized with each iteration instead of just once. This is not always a problem—perhaps the value is already reset at the start of each iteration. However, it can be a problem, so it is "not safe" in the context of code analysis, unless proven otherwise.

That is the concern here. Not taking the address of something, but moving a variable definition inside a loop. The point of the do_something line is that the value of i must be preserved from one iteration to the next. (The value of i might be used then modified by do_something().) Taking the address of i is merely a way to highlight the possibility that the value of i might change. The same could have been accomplished if i was passed by reference, but that is more subtle than passing by address. The & stands out and says "yo, value potentially changing here!"

The other significant parts of this example are that the loop is an inner loop, and that i is used only inside the loop. That is, there are multiple levels to which scope could be reduced and still be syntactically correct (i.e. "possible"). When this analysis tool reports "The scope of the variable i can be reduced", only one level of reduction has been approved. Reducing the scope more than that might change functionality (i.e. be "not safe"). Might. If scope could be reduced multiple levels and none of those levels come from loops, then it is safe to reduce multiple levels at once. This is an allowed optimization. However, when there is a loop, the safe approach is to reduce the scope to just outside the loop, then run the analysis again to find out if scope can be reduced further.

I would guess that someone had used Cppcheck, gotten the message that the scope of a variable could be reduced, and decided to reduce the scope as much as possible. I'm guessing that this broke the program, so this "someone" reported it as a bug in Cppcheck. The comment warning people against this practice could have been the result. Its meaning should be clear to its target audience, but I can see that there could be less clarity to someone coming from a different perspective.

Related