CODEWITHSUNDEEP
c++c
palik
palik

Reputation: 2873

Pointer to local variable is stored outside the scope of this variable

For the following code snippet, which is a part of libgearman

gearman_job_st *gearman_worker_grab_job(gearman_worker_st *worker_shell,
                                        gearman_job_st *job,
                                        gearman_return_t *ret_ptr)
{
    if (worker_shell and worker_shell->impl())
    {

        ...

        gearman_return_t unused;
        if (ret_ptr == NULL)
        {
            ret_ptr= &unused;
        }
        ...
    }

    assert(*ret_ptr != GEARMAN_MAX_RETURN);
    return NULL;
}

PVS-Studio reported:

Viva64-EM
full
671
/nfs/home/xxx/src/gearmand/libgearman/worker.cc
error
V506
Pointer to local variable 'unused' is stored outside the scope of this variable. Such a pointer will become invalid.
false
2
    {
      ret_ptr= &unused;
    }
------------

Regarding the question Pointer to local variable outside the scope of its declaration, if I understand that correctly, malloc and free should be used for refactoring. My question is if there is an other appropriate refactoring alternative. For instance using of std::unique_ptr:

ret_ptr = std::make_unique<gearman_return_t>();

Upvotes: 2

Views: 1615

Answers (1)

dbush
dbush

Reputation: 225827

The ret_ptr parameter to the function in question is expected to point to a variable in the calling function. This pointer is then dereferenced for both reading and writing this external variable.

The if (ret_ptr == NULL) block checks whether the caller actually passed in the address of some variable. If not, this pointer is then made to point to the local variable unused so that the pointer can still be safely dereferenced later in the code. But since ret_ptr now points to a local, changes made by dereferencing it are not seen outside the function. This is fine, since the caller passed in NULL for ret_ptr. Similarly, since ret_ptr is a parameter, any changes to it are not visible outside of the function.

Nothing needs to be refactored here. The code works as intended with regard to ret_ptr. This is a false positive from PVS-Studio.

EDIT:

This is NOT a false positive. The unused variable is defined at a lower scope than ret_ptr, namely the scope of the first if block in the function. After the if block, ret_ptr is then dereferenced. If it was pointing to ununsed, that variable is now out of scope and dereferencing ret_ptr invokes undefined behavior.

To fix this, unused must be declared and assigned to above the if block:

gearman_job_st *gearman_worker_grab_job(gearman_worker_st *worker_shell,
                                        gearman_job_st *job,
                                        gearman_return_t *ret_ptr)
{
    gearman_return_t unused;
    if (ret_ptr == NULL)
    {
        ret_ptr= &unused;
    }

    if (worker_shell and worker_shell->impl())
    {
        ...
    }

    assert(*ret_ptr != GEARMAN_MAX_RETURN);
    return NULL;
}

Upvotes: 9

Related Questions