Reputation: 2873
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
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