Possible pointer var TupleDesc rettupdesc used not initialized (src/backend/optimizer/util/clauses.c)
Hi,
Possible pointer TupleDesc rettupdesc used not initialized?
if (!isNull) at line 4346 taking a true branch, the function
check_sql_fn_retval at line 4448 can use rettupdesc uninitialized.
regards,
Ranier Vilela
On May 24, 2021, at 5:37 PM, Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi,
Possible pointer TupleDesc rettupdesc used not initialized?
if (!isNull) at line 4346 taking a true branch, the function check_sql_fn_retval at line 4448 can use rettupdesc uninitialized.
Care to submit a patch?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Em seg., 24 de mai. de 2021 às 22:42, Mark Dilger <
mark.dilger@enterprisedb.com> escreveu:
On May 24, 2021, at 5:37 PM, Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi,
Possible pointer TupleDesc rettupdesc used not initialized?
if (!isNull) at line 4346 taking a true branch, the function
check_sql_fn_retval at line 4448 can use rettupdesc uninitialized.
Care to submit a patch?
Hi Mark, sorry but not.
I examined the code and I can't say what the correct value is for
rettupdesc.
regards,
Ranier Vilela
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
Livre
de vírus. www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>.
<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
On Mon, May 24, 2021 at 7:21 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Em seg., 24 de mai. de 2021 às 22:42, Mark Dilger <
mark.dilger@enterprisedb.com> escreveu:On May 24, 2021, at 5:37 PM, Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi,
Possible pointer TupleDesc rettupdesc used not initialized?
if (!isNull) at line 4346 taking a true branch, the function
check_sql_fn_retval at line 4448 can use rettupdesc uninitialized.
Care to submit a patch?
Hi Mark, sorry but not.
I examined the code and I can't say what the correct value is for
rettupdesc.
Hi,
It seems the following call would fill up value for rettupdesc :
functypclass = get_expr_result_type((Node *) fexpr, NULL, &rettupdesc);
Cheers
Show quoted text
regards,
Ranier Vilela<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> Livre
de vírus. www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>.
<#m_-660087238671669467_DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
Em seg., 24 de mai. de 2021 às 23:35, Zhihong Yu <zyu@yugabyte.com>
escreveu:
On Mon, May 24, 2021 at 7:21 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Em seg., 24 de mai. de 2021 às 22:42, Mark Dilger <
mark.dilger@enterprisedb.com> escreveu:On May 24, 2021, at 5:37 PM, Ranier Vilela <ranier.vf@gmail.com>
wrote:
Hi,
Possible pointer TupleDesc rettupdesc used not initialized?
if (!isNull) at line 4346 taking a true branch, the function
check_sql_fn_retval at line 4448 can use rettupdesc uninitialized.
Care to submit a patch?
Hi Mark, sorry but not.
I examined the code and I can't say what the correct value is for
rettupdesc.Hi,
It seems the following call would fill up value for rettupdesc :functypclass = get_expr_result_type((Node *) fexpr, NULL, &rettupdesc);
In short, do you suggest running half the else?
To do this, you need to fill fexpr correctly.
It will not always be a trivial solution.
regards,
Ranier Vilela
Ranier Vilela <ranier.vf@gmail.com> writes:
Possible pointer TupleDesc rettupdesc used not initialized?
if (!isNull) at line 4346 taking a true branch, the function
check_sql_fn_retval at line 4448 can use rettupdesc uninitialized.
This seems to have been introduced by the SQL-function-body patch.
After some study, I concluded that the reason we haven't noticed
is that the case is nearly unreachable: check_sql_fn_retval never
consults the rettupdesc unless the function result type is composite
and the tlist length is more than one --- and we eliminated the latter
case earlier in inline_function.
There is an exception, namely if the single tlist item fails to
be coercible to the output type, but that's hard to get to given
that it'd have been checked while defining the SQL-body function.
I did manage to reproduce a problem after turning off
check_function_bodies so I could create a broken function.
In any case, inline_function has no business assuming that
check_sql_fn_retval doesn't need a valid value.
The simplest way to fix this seems to be to move the code that
creates "fexpr" and calls get_expr_result_type, so that we always
do that even for SQL-body cases. We could alternatively use some
other way to obtain a result tupdesc in the SQL-body path; but
creating the dummy FuncExpr node is cheap enough that I don't
think it's worth contortions to avoid doing it.
regards, tom lane
Em ter., 25 de mai. de 2021 às 13:09, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
Possible pointer TupleDesc rettupdesc used not initialized?
if (!isNull) at line 4346 taking a true branch, the function
check_sql_fn_retval at line 4448 can use rettupdesc uninitialized.This seems to have been introduced by the SQL-function-body patch.
After some study, I concluded that the reason we haven't noticed
is that the case is nearly unreachable: check_sql_fn_retval never
consults the rettupdesc unless the function result type is composite
and the tlist length is more than one --- and we eliminated the latter
case earlier in inline_function.There is an exception, namely if the single tlist item fails to
be coercible to the output type, but that's hard to get to given
that it'd have been checked while defining the SQL-body function.
I did manage to reproduce a problem after turning off
check_function_bodies so I could create a broken function.In any case, inline_function has no business assuming that
check_sql_fn_retval doesn't need a valid value.The simplest way to fix this seems to be to move the code that
creates "fexpr" and calls get_expr_result_type, so that we always
do that even for SQL-body cases. We could alternatively use some
other way to obtain a result tupdesc in the SQL-body path; but
creating the dummy FuncExpr node is cheap enough that I don't
think it's worth contortions to avoid doing it.
Following the guidelines, I provided a patch.
But I did more than requested, removed redundant variable and reduced the
scope of two.
vcregress check pass fine.
regards,
Ranier Vilela
Attachments:
v1_fix_uninitialized_var_clauses.patchapplication/octet-stream; name=v1_fix_uninitialized_var_clauses.patchDownload
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index e117ab976e..c93d7f52ec 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4272,10 +4272,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
inline_error_callback_arg callback_arg;
ErrorContextCallback sqlerrcontext;
FuncExpr *fexpr;
- SQLFunctionParseInfoPtr pinfo;
TupleDesc rettupdesc;
- ParseState *pstate;
- List *raw_parsetree_list;
List *querytree_list;
Query *querytree;
Node *newexpr;
@@ -4338,6 +4335,26 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
sqlerrcontext.previous = error_context_stack;
error_context_stack = &sqlerrcontext;
+ /*
+ * Create a dummy FuncExpr node containing the already-simplified
+ * arguments to pass to prepare_sql_fn_parse_info.
+ */
+ fexpr = makeNode(FuncExpr);
+ fexpr->funcid = funcid;
+ fexpr->funcresulttype = result_type;
+ fexpr->funcretset = false;
+ fexpr->funcvariadic = funcvariadic;
+ fexpr->funcformat = COERCE_EXPLICIT_CALL; /* doesn't matter */
+ fexpr->funccollid = result_collid; /* doesn't matter */
+ fexpr->inputcollid = input_collid;
+ fexpr->args = args;
+ fexpr->location = -1;
+
+ /* fexpr also provides a convenient way to resolve a composite result */
+ (void) get_expr_result_type((Node *) fexpr,
+ NULL,
+ &rettupdesc);
+
/* If we have prosqlbody, pay attention to that not prosrc */
tmp = SysCacheGetAttr(PROCOID,
func_tuple,
@@ -4346,7 +4363,6 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
if (!isNull)
{
Node *n;
- List *querytree_list;
n = stringToNode(TextDatumGetCString(tmp));
if (IsA(n, List))
@@ -4359,47 +4375,31 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
}
else
{
- /*
- * Set up to handle parameters while parsing the function body. We
- * need a dummy FuncExpr node containing the already-simplified
- * arguments to pass to prepare_sql_fn_parse_info. (In some cases we
- * don't really need that, but for simplicity we always build it.)
- */
- fexpr = makeNode(FuncExpr);
- fexpr->funcid = funcid;
- fexpr->funcresulttype = result_type;
- fexpr->funcretset = false;
- fexpr->funcvariadic = funcvariadic;
- fexpr->funcformat = COERCE_EXPLICIT_CALL; /* doesn't matter */
- fexpr->funccollid = result_collid; /* doesn't matter */
- fexpr->inputcollid = input_collid;
- fexpr->args = args;
- fexpr->location = -1;
+ SQLFunctionParseInfoPtr pinfo;
+ ParseState *pstate;
+ /*
+ * Set up to handle parameters while parsing the function body.
+ */
pinfo = prepare_sql_fn_parse_info(func_tuple,
(Node *) fexpr,
input_collid);
- /* fexpr also provides a convenient way to resolve a composite result */
- (void) get_expr_result_type((Node *) fexpr,
- NULL,
- &rettupdesc);
-
/*
* We just do parsing and parse analysis, not rewriting, because
* rewriting will not affect table-free-SELECT-only queries, which is
* all that we care about. Also, we can punt as soon as we detect
* more than one command in the function body.
*/
- raw_parsetree_list = pg_parse_query(src);
- if (list_length(raw_parsetree_list) != 1)
+ querytree_list = pg_parse_query(src);
+ if (list_length(querytree_list) != 1)
goto fail;
pstate = make_parsestate(NULL);
pstate->p_sourcetext = src;
sql_fn_parser_setup(pstate, pinfo);
- querytree = transformTopLevelStmt(pstate, linitial(raw_parsetree_list));
+ querytree = transformTopLevelStmt(pstate, linitial(querytree_list));
free_parsestate(pstate);
}
Ranier Vilela <ranier.vf@gmail.com> writes:
Following the guidelines, I provided a patch.
Oh, I already pushed a fix, thanks.
regards, tom lane
Em ter., 25 de mai. de 2021 às 14:35, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
Following the guidelines, I provided a patch.
Oh, I already pushed a fix, thanks.
No problem!
Thank you.
regards,
Ranier Vilela