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

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

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++)

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

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