Обсуждение: Possible NULL dereferencing (src/backend/tcop/pquery.c)
Hi,
Per Coverity.
Perhaps it is excessive caution.
Probably assertion check has already caught all possible errors.
But, redundancy may not cost as much and is worth it.
Probably assertion check has already caught all possible errors.
But, redundancy may not cost as much and is worth it.
1.Assertion check
/* Caller messed up if we have neither a ready query nor held data. */
Assert(queryDesc || portal->holdStore);
Assert(queryDesc || portal->holdStore);
But in release, if QueryDesc is NULL and portal->holdStore is NULL too,
when Call PushActiveSnapshot deference NULL check can happen.
2. if (portal->atEnd || count <= 0) is True
No need to recheck count against FETCH_ALL.
Is it worth correcting them?
regards,
Ranier Vilela
Вложения
Ranier Vilela <ranier.vf@gmail.com> writes:
> 1.Assertion check
> /* Caller messed up if we have neither a ready query nor held data. */
> Assert(queryDesc || portal->holdStore);
> But in release, if QueryDesc is NULL and portal->holdStore is NULL too,
> when Call PushActiveSnapshot *deference* NULL check can happen.
> 2. if (portal->atEnd || count <= 0) is True
> No need to recheck count against FETCH_ALL.
> Is it worth correcting them?
No.
The assertion already says that that's a case that cannot happen.
Or to look at it another way: if the case were to occur in a devel
build, you'd get a core dump at the assertion. If the case were
to occur in a production build, you'd get a core dump at the
dereference. Not much difference. Either way, it's a *caller*
bug, because the caller is supposed to make sure this cannot happen.
If we thought that it could possibly happen, we would use an ereport
but not an assertion; having both for the same condition is quite
misguided.
(If Coverity is whining about this for you, there's something wrong
with your Coverity settings. In the project's instance, Coverity
accepts assertions as assertions.)
I'm unimpressed with the other proposed change too; it's making the logic
more complicated and fragile for a completely negligible "performance
gain". Moreover the compiler could probably make the same optimization.
regards, tom lane
Em sex., 26 de jun. de 2020 às 18:24, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
> 1.Assertion check
> /* Caller messed up if we have neither a ready query nor held data. */
> Assert(queryDesc || portal->holdStore);
> But in release, if QueryDesc is NULL and portal->holdStore is NULL too,
> when Call PushActiveSnapshot *deference* NULL check can happen.
> 2. if (portal->atEnd || count <= 0) is True
> No need to recheck count against FETCH_ALL.
> Is it worth correcting them?
No.
The assertion already says that that's a case that cannot happen.
Or to look at it another way: if the case were to occur in a devel
build, you'd get a core dump at the assertion. If the case were
to occur in a production build, you'd get a core dump at the
dereference. Not much difference. Either way, it's a *caller*
bug, because the caller is supposed to make sure this cannot happen.
If we thought that it could possibly happen, we would use an ereport
but not an assertion; having both for the same condition is quite
misguided.
Ok, thats a job of Assertion.
But I still worry that, in some rare cases, portal-> holdStore might be corrupted in some way
and the function is called, causing a segmentation fault.
and the function is called, causing a segmentation fault.
(If Coverity is whining about this for you, there's something wrong
with your Coverity settings. In the project's instance, Coverity
accepts assertions as assertions.)
Probable, because reports this:
CID 10127 (#2 of 2): Dereference after null check (FORWARD_NULL)8. var_deref_op: Dereferencing null pointer queryDesc.
I'm unimpressed with the other proposed change too; it's making the logic
more complicated and fragile for a completely negligible "performance
gain". Moreover the compiler could probably make the same optimization.
Ok.
It's not easy.
best regards,
Ranier Vilela