Re: [HACKERS] Dereferenced pointer in tablesample.c

2015-06-30 Thread Petr Jelinek

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
diff --git a/src/backend/access/tablesample/tablesample.c b/src/backend/access/tablesample/tablesample.c
index 44a2434..d263fe2 100644
--- a/src/backend/access/tablesample/tablesample.c
+++ b/src/backend/access/tablesample/tablesample.c
@@ -110,12 +110,6 @@ tablesample_init(SampleScanState *scanstate, TableSampleClause *tablesample)
 		Expr	   *argexpr = (Expr *) lfirst(arg);
 		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);
 		i++;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Dereferenced pointer in tablesample.c

2015-06-30 Thread Tom Lane
Petr Jelinek p...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers