Обсуждение: Dereferenced pointer in tablesample.c
Hi all,
(Petr in CC)
Coverity is complaining about the following pointer dereference in tablesample_init@tablesample.c:
+ ExprState *argstate = ExecInitExpr(argexpr, (PlanState *) scanstate);
+
+ if (argstate == NULL)
+ {
+ fcinfo.argnull[i] = true;
+ fcinfo.arg[i] = (Datum) 0;;
+ }
+
+ fcinfo.arg[i] = ExecEvalExpr(argstate, econtext,
+ &fcinfo.argnull[i], NULL);
--
Michael
Michael
Вложения
On 2015-06-30 09:10, Michael Paquier wrote: > Hi all, > (Petr in CC) > > Coverity is complaining about the following pointer dereference in > tablesample_init@tablesample.c: > + ExprState *argstate = ExecInitExpr(argexpr, (PlanState > *) scanstate); > + > + if (argstate == NULL) > + { > + fcinfo.argnull[i] = true; > + fcinfo.arg[i] = (Datum) 0;; > + } > + > + fcinfo.arg[i] = ExecEvalExpr(argstate, econtext, > + > &fcinfo.argnull[i], NULL); > > If the expression argstate is NULL when calling ExecInitExpr(), argstate > is going to be NULL and dereferenced afterwards, see execQual.c for more > details. Hence I think that the patch attached should be applied. Thoughts? > Well, yes the ExecEvalExpr should be in the else block if we'd keep the NULL logic there. However after rereading the code, ISTM the ExecInitExpr will only return NULL if the argexpr is NULL and argexpr is added by ParseTableSample using the transformExpr on every argument which comes from grammar and those are a_exprs which AFAIK will never be NULL. So I actually think that the argstate can never be NULL in practice. Given the above I would just remove the if statement here - it's not present in any other code that does ExecInitExpr/ExecEvalExpr either. It's most likely relic of the code that didn't treat the repeatable separately and just put it into args List. Patch attached. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
Petr Jelinek <petr@2ndquadrant.com> writes: > On 2015-06-30 09:10, Michael Paquier wrote: >> If the expression argstate is NULL when calling ExecInitExpr(), argstate >> is going to be NULL and dereferenced afterwards, see execQual.c for more >> details. Hence I think that the patch attached should be applied. Thoughts? > Well, yes the ExecEvalExpr should be in the else block if we'd keep the > NULL logic there. > However after rereading the code, ISTM the ExecInitExpr will only return > NULL if the argexpr is NULL and argexpr is added by ParseTableSample > using the transformExpr on every argument which comes from grammar and > those are a_exprs which AFAIK will never be NULL. So I actually think > that the argstate can never be NULL in practice. Indeed. ParseTableSample() is badly in need of a rewrite, but I agree that it's not going to produce null expression trees. > Patch attached. Will push this shortly. regards, tom lane