Thread: [PATCH] Explicit null dereferenced (src/backend/access/heap/heaptoast.c)
Hi,
Per Coverity.
When "Prepare for toasting", it is necessary to turn off the flag TOAST_NEEDS_DELETE_OLD,
if there is no need to delete external values from the old tuple, otherwise,
there are dereference NULL at toast_helper.c (on toast_tuple_cleanup function).
regards,
Ranier Vilela
Attachment
Re: [PATCH] Explicit null dereferenced (src/backend/access/heap/heaptoast.c)
From
gkokolatos@pm.me
Date:
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Friday, 28 August 2020 03:22, Ranier Vilela <ranier.vf@gmail.com> wrote: > Hi, > > Per Coverity. > > When "Prepare for toasting", it is necessary to turn off the flag TOAST_NEEDS_DELETE_OLD, > if there is no need to delete external values from the old tuple, otherwise, > there are dereference NULL at toast_helper.c (on toast_tuple_cleanup function). > Excuse my ignorance, isn't this a false positive? Regardless right after prepare for toasting, a call to toast_tuple_init is made which will explicitly and unconditionallyset ttc_flags to zero so the flag bit set in the patch will be erased anyways. This patch may make coverityhappy but does not really change anything in the behaviour of the code. Furthermore, in the same function, toast_tuple_init, the flag is set to TOAST_NEEDS_DELETE_OLD after the old value is actuallyinspected and found to not be null, be stored on disk and to be different than the new value. To my understanding,this seems to be correct. Can you please explain to me what I am missing? //Georgios > regards, > Ranier Vilela
Re: [PATCH] Explicit null dereferenced (src/backend/access/heap/heaptoast.c)
From
Ranier Vilela
Date:
Em sex., 28 de ago. de 2020 às 04:45, <gkokolatos@pm.me> escreveu:
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 28 August 2020 03:22, Ranier Vilela <ranier.vf@gmail.com> wrote:
> Hi,
>
> Per Coverity.
>
> When "Prepare for toasting", it is necessary to turn off the flag TOAST_NEEDS_DELETE_OLD,
> if there is no need to delete external values from the old tuple, otherwise,
> there are dereference NULL at toast_helper.c (on toast_tuple_cleanup function).
>
Excuse my ignorance, isn't this a false positive?
Yes, you're right.
Coverity fails with &.
if (oldtup == NULL)
147 {
3. assign_zero: Assigning: ttc.ttc_oldvalues = NULL.
148 ttc.ttc_oldvalues = NULL;
149 ttc.ttc_oldisnull = NULL;
4. Falling through to end of if statement.
150 }
151 else
152 {
153 ttc.ttc_oldvalues = toast_oldvalues;
154 ttc.ttc_oldisnull = toast_oldisnull;
155 }
156 ttc.ttc_attr = toast_attr;
157 toast_tuple_init(&ttc); // Coverity ignores the call completely here.
toast_tuple_init, solves the bug, because reset ttc->flags.
Regardless right after prepare for toasting, a call to toast_tuple_init is made which will explicitly and unconditionally set ttc_flags to zero so the flag bit set in the patch will be erased anyways. This patch may make coverity happy but does not really change anything in the behaviour of the code.
That's right, the patch doesn't change anything.
Furthermore, in the same function, toast_tuple_init, the flag is set to TOAST_NEEDS_DELETE_OLD after the old value is actually inspected and found to not be null, be stored on disk and to be different than the new value. To my understanding, this seems to be correct.
Very correct.
Thanks for taking a look here.
You could take a look at the attached patch,
would it be an improvement?
would it be an improvement?
toast_tuple_init, it seems to me that it can be improved.
ttc->ttc_oldvalues is constant, and it could be unlooping?
regards,
Ranier Vilela