Thread: empty array case in plperl_ref_from_pg_array not handled correctly
Hi, Per the new valgrind animal we get: http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink&dt=2016-03-08%2004%3A22%3A00 2016-03-08 05:56:05.566 UTC [56de6971.723:5] LOG: statement: select plperl_sum_array('{}'); ==1827== Invalid write of size 4 ==1827== at 0x14E35DD1: plperl_ref_from_pg_array (plperl.c:1459) ==1827== by 0x14E3608C: plperl_call_perl_func (plperl.c:2135) ==1827== by 0x14E3C24F: plperl_func_handler (plperl.c:2357) ==1827== by 0x14E3C24F: plperl_call_handler (plperl.c:1778) ==1827== by 0x5E0531: ExecMakeFunctionResultNoSets (execQual.c:2041) ==1827== by 0x5E641C: ExecTargetList (execQual.c:5367) ==1827== by 0x5E641C: ExecProject (execQual.c:5582) ==1827== by 0x5FC1C1: ExecResult (nodeResult.c:155) ==1827== by 0x5DF577: ExecProcNode (execProcnode.c:392) ==1827== by 0x5DB675: ExecutePlan (execMain.c:1566) ==1827== by 0x5DB675: standard_ExecutorRun (execMain.c:338) ==1827== by 0x6F4DD7: PortalRunSelect (pquery.c:942) ==1827== by 0x6F631D: PortalRun (pquery.c:786) ==1827== by 0x6F2FFA: exec_simple_query (postgres.c:1094) ==1827== by 0x6F2FFA: PostgresMain (postgres.c:4021) ==1827== by 0x46D33F: BackendRun (postmaster.c:4258) ==1827== by 0x46D33F: BackendStartup (postmaster.c:3932) ==1827== by 0x46D33F: ServerLoop (postmaster.c:1690) ==1827== Address 0x15803220 is 304 bytes inside a block of size 8,192 alloc'd ==1827== at 0x4C28BB5: malloc (vg_replace_malloc.c:299) ==1827== by 0x808A37: AllocSetAlloc (aset.c:864) ==1827== by 0x80A3B3: palloc (mcxt.c:907) ==1827== by 0x7E0802: get_func_signature (lsyscache.c:1483) ==1827== by 0x14E367D6: plperl_call_perl_func (plperl.c:2116) ==1827== by 0x14E3C24F: plperl_func_handler (plperl.c:2357) ==1827== by 0x14E3C24F: plperl_call_handler (plperl.c:1778) ==1827== by 0x5E0531: ExecMakeFunctionResultNoSets (execQual.c:2041) ==1827== by 0x5E641C: ExecTargetList (execQual.c:5367) ==1827== by 0x5E641C: ExecProject (execQual.c:5582) ==1827== by 0x5FC1C1: ExecResult (nodeResult.c:155) ==1827== by 0x5DF577: ExecProcNode (execProcnode.c:392) ==1827== by 0x5DB675: ExecutePlan (execMain.c:1566) ==1827== by 0x5DB675: standard_ExecutorRun (execMain.c:338) ==1827== by 0x6F4DD7: PortalRunSelect (pquery.c:942) ==1827== looking at the code static SV * plperl_ref_from_pg_array(Datum arg, Oid typid) { .../* Get total number of elements in each dimension */info->nelems = palloc(sizeof(int) * info->ndims);info->nelems[0] =nitems; that's not suprising. If ndims is zero nelemes will be a zero-length array. AddingAssert(info->ndims > 0); makes it die reliably, without gdb. ISTM the assumption that an array always has a dimension is a bit more widespread... E.g. split_array() looks like it'd not work nicely with a zero dimensional array... Greetings, Andres Freund
On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund <andres@anarazel.de> wrote:
Hi,
Per the new valgrind animal we get:
http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink&dt=2016-03-08%2004%3A22%3A00
2016-03-08 05:56:05.566 UTC [56de6971.723:5] LOG: statement: select plperl_sum_array('{}');
==1827== Invalid write of size 4
==1827== at 0x14E35DD1: plperl_ref_from_pg_array (plperl.c:1459)
[ I think you may have meant to CC me not Alexey K. I'm probably the person responsible :D. ]
Indeed, I think the simplest fix is to make plperl_ref_from_pg_array() return an "empty" array in that case. The attached fixes the valgrind warning for me. (cassert enabled build on master).
ISTM the assumption that an array always has a dimension is a bit more
widespread... E.g. split_array() looks like it'd not work nicely with a
zero dimensional array...
Hrm, plperl_ref_from_pg_array() is the only caller of split_array(), so I added an Assert() to make its contract more clear.
Recursive calls to split_array() should be fine as nested 0 dim arrays get collapsed down to single 0 dim array (I hand tested it nonetheless):
# select ARRAY[ARRAY[ARRAY[]]]::int[];
array
───────
{}
(1 row)
Looking around, everything that makes an array seems to pass through plperl_ref_from_pg_array(), so I think once that is fixed all is good. I also looked for dimensions and ARR_NDIM() just to make sure (didn't find anything fishy).
Thanks,
Attachment
On 08 Mar 2016, at 10:11, Alex Hunsaker <badalex@gmail.com> wrote:On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund <andres@anarazel.de> wrote:Hi,
Per the new valgrind animal we get:
http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink&dt=2016-03-08%2004%3A22%3A00
2016-03-08 05:56:05.566 UTC [56de6971.723:5] LOG: statement: select plperl_sum_array('{}');
==1827== Invalid write of size 4
==1827== at 0x14E35DD1: plperl_ref_from_pg_array (plperl.c:1459)[ I think you may have meant to CC me not Alexey K. I'm probably the person responsible :D. ]Indeed, I think the simplest fix is to make plperl_ref_from_pg_array() return an "empty" array in that case. The attached fixes the valgrind warning for me. (cassert enabled build on master).
Looks good to me, thank you. Judging from the size in the error message it’s likely the
info->nelems[0] = items
line that caused this issue. The patch fixes it at first glance, although I have yet to make my valgrind setup on OS X working to check this for real :-)
--
Oleksii
On 2016-03-08 02:11:03 -0700, Alex Hunsaker wrote: > On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund <andres@anarazel.de> wrote: > > > Hi, > > > > Per the new valgrind animal we get: > > > > > > http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink&dt=2016-03-08%2004%3A22%3A00 > > 2016-03-08 05:56:05.566 UTC [56de6971.723:5] LOG: statement: select > > plperl_sum_array('{}'); > > ==1827== Invalid write of size 4 > > ==1827== at 0x14E35DD1: plperl_ref_from_pg_array (plperl.c:1459) > > > > > [ I think you may have meant to CC me not Alexey K. I'm probably the person > responsible :D. ] I just took the first person mentioned in the commit message http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=87bb2ade2ce646083f39d5ab3e3307490211ad04 sorry for leaving you out ;) > Recursive calls to split_array() should be fine as nested 0 dim arrays get > collapsed down to single 0 dim array (I hand tested it nonetheless): > # select ARRAY[ARRAY[ARRAY[]]]::int[]; > array > ─────── > {} > (1 row) > > Looking around, everything that makes an array seems to pass through > plperl_ref_from_pg_array(), so I think once that is fixed all is good. I > also looked for dimensions and ARR_NDIM() just to make sure (didn't find > anything fishy). Thanks for looking. backpatched all the way. Greetings, Andres Freund