Re: [PATCH] Explicit null dereferenced (src/backend/access/heap/heaptoast.c)

Em sex., 28 de ago. de 2020 às 04:45, <gkokolatos(at)pm(dot)me> escreveu:

>
>
>
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Friday, 28 August 2020 03:22, Ranier Vilela <ranier(dot)vf(at)gmail(dot)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?
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