Potential pointer dereference in plperl.c (caused by transforms patch)

Started by Michael Paquierover 10 years ago4 messages

michael.paquier@gmail.com

over 10 years ago

1 attachment(s)

Hi all,

Coverity is pointing out that as argtypes = NULL in
plperl_call_perl_func@plperl.c, we will have a pointer dereference if
desc->arg_arraytype[i] is not a valid OID, see here:
+       Oid                *argtypes = NULL;
[...]
+       if (fcinfo->flinfo->fn_oid)
+               get_func_signature(fcinfo->flinfo->fn_oid, &argtypes, &nargs);
[...]
                        if (OidIsValid(desc->arg_arraytype[i]))
                                sv =
plperl_ref_from_pg_array(fcinfo->arg[i], desc->arg_arraytype[i]);
+                       else if ((funcid =
get_transform_fromsql(argtypes[i],
current_call_data->prodesc->lang_oid,
current_call_data->prodesc->trftypes)))
+                               sv = (SV *)
DatumGetPointer(OidFunctionCall1(funcid, fcinfo->arg[i]));
AFAIK, fcinfo->flinfo->fn_oid can be InvalidOid in this code path, so
shouldn't we protect a bit the code with something like the patch
attached?
Regards,
-- 
Michael

Attachments:

20150504_plperl_dereference.patchtext/x-diff; charset=US-ASCII; name=20150504_plperl_dereference.patchDownload

diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 840df2e..6a1d2bb 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -2100,8 +2100,11 @@ plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
 	PUSHMARK(SP);
 	EXTEND(sp, desc->nargs);
 
-	if (fcinfo->flinfo->fn_oid)
+	if (OidIsValid(fcinfo->flinfo->fn_oid))
+	{
 		get_func_signature(fcinfo->flinfo->fn_oid, &argtypes, &nargs);
+		Assert(nargs == desc->nargs);
+	}
 
 	for (i = 0; i < desc->nargs; i++)
 	{
@@ -2120,7 +2123,8 @@ plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
 
 			if (OidIsValid(desc->arg_arraytype[i]))
 				sv = plperl_ref_from_pg_array(fcinfo->arg[i], desc->arg_arraytype[i]);
-			else if ((funcid = get_transform_fromsql(argtypes[i], current_call_data->prodesc->lang_oid, current_call_data->prodesc->trftypes)))
+			else if (OidIsValid(fcinfo->flinfo->fn_oid) &&
+				(funcid = get_transform_fromsql(argtypes[i], current_call_data->prodesc->lang_oid, current_call_data->prodesc->trftypes)))
 				sv = (SV *) DatumGetPointer(OidFunctionCall1(funcid, fcinfo->arg[i]));
 			else
 			{

Noah Misch

noah@leadboat.com

about 10 years ago

In reply to: Michael Paquier (#1)

Re: Potential pointer dereference in plperl.c (caused by transforms patch)

On Mon, May 04, 2015 at 02:02:18PM +0900, Michael Paquier wrote:

Coverity is pointing out that as argtypes = NULL in
plperl_call_perl_func@plperl.c, we will have a pointer dereference if
desc->arg_arraytype[i] is not a valid OID, see here:
+       Oid                *argtypes = NULL;
[...]
+       if (fcinfo->flinfo->fn_oid)
+               get_func_signature(fcinfo->flinfo->fn_oid, &argtypes, &nargs);
[...]
if (OidIsValid(desc->arg_arraytype[i]))
sv =
plperl_ref_from_pg_array(fcinfo->arg[i], desc->arg_arraytype[i]);
+                       else if ((funcid =
get_transform_fromsql(argtypes[i],
current_call_data->prodesc->lang_oid,
current_call_data->prodesc->trftypes)))
+                               sv = (SV *)
DatumGetPointer(OidFunctionCall1(funcid, fcinfo->arg[i]));
AFAIK, fcinfo->flinfo->fn_oid can be InvalidOid in this code path, so
shouldn't we protect a bit the code with something like the patch
attached?

fcinfo->flinfo->fn_oid==InvalidOid implies an inline block, and those have no
arguments. If it placates Coverity, I lean toward an assert-only change:

--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -2112,6 +2112,8 @@ plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
 	EXTEND(sp, desc->nargs);

+	/* Get signature for true functions; inline blocks have no args. */
 	if (fcinfo->flinfo->fn_oid)
 		get_func_signature(fcinfo->flinfo->fn_oid, &argtypes, &nargs);
+	Assert(nargs == desc->nargs);

for (i = 0; i < desc->nargs; i++)

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

Michael Paquier

michael.paquier@gmail.com

about 10 years ago

In reply to: Noah Misch (#2)

Re: Potential pointer dereference in plperl.c (caused by transforms patch)

On Sat, Nov 28, 2015 at 5:29 AM, Noah Misch wrote:

fcinfo->flinfo->fn_oid==InvalidOid implies an inline block, and those have no
arguments. If it placates Coverity, I lean toward an assert-only change:

Oh, thanks. I missed this point.

--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -2112,6 +2112,8 @@ plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
EXTEND(sp, desc->nargs);

+       /* Get signature for true functions; inline blocks have no args. */
if (fcinfo->flinfo->fn_oid)
get_func_signature(fcinfo->flinfo->fn_oid, &argtypes, &nargs);
+       Assert(nargs == desc->nargs);

for (i = 0; i < desc->nargs; i++)

Yeah that looks fine.
--
Michael

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

Alvaro Herrera

alvherre@2ndquadrant.com

about 10 years ago

In reply to: Noah Misch (#2)

Re: Re: Potential pointer dereference in plperl.c (caused by transforms patch)

Noah Misch wrote:

fcinfo->flinfo->fn_oid==InvalidOid implies an inline block, and those have no
arguments. If it placates Coverity, I lean toward an assert-only change:
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c

This was committed as d4b686af0b.

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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