Re: [HACKERS] empty array case in plperl_ref_from_pg_array not handled correctly

2016-03-08 Thread Andres Freund
On 2016-03-08 02:11:03 -0700, Alex Hunsaker wrote:
> On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund  wrote:
> 
> > Hi,
> >
> > Per the new valgrind animal we get:
> >
> >
> > http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink=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


-- 
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] empty array case in plperl_ref_from_pg_array not handled correctly

2016-03-08 Thread Oleksii Kliukin

> On 08 Mar 2016, at 10:11, Alex Hunsaker  wrote:
> 
> 
> 
> On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund  > wrote:
> Hi,
> 
> Per the new valgrind animal we get:
> 
> http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink=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 :-)

Kind regards,
--
Oleksii



Re: [HACKERS] empty array case in plperl_ref_from_pg_array not handled correctly

2016-03-08 Thread Alex Hunsaker
On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund  wrote:

> Hi,
>
> Per the new valgrind animal we get:
>
>
> http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink=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,


plperl_array_valgrind.patch
Description: Binary data

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


[HACKERS] empty array case in plperl_ref_from_pg_array not handled correctly

2016-03-07 Thread Andres Freund
Hi,

Per the new valgrind animal we get:

http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink=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. Adding
Assert(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


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