Обсуждение: jsonb_plperl bug

Hi,

I have found a bug in jsonb_plperl extension. A possible fix is proposed below.

jsonb_perl is the contrib module, which defines TRANSFORM functions for jsonb data type and PL/Perl procedural language.

The bug can be reproduced as follows:

CREATE EXTENSION plperl;
CREATE EXTENSION jsonb_plperl;

CREATE OR REPLACE FUNCTION text2jsonb (text) RETURNS jsonb
    LANGUAGE plperl TRANSFORM FOR TYPE jsonb AS
$$
    my $x = shift;
    my $ret = {a=>$x};
    return $ret;
$$;
SELECT text2jsonb(NULL);
SELECT text2jsonb('11');
SELECT text2jsonb(NULL);

The last SELECT produces a strange error.

ERROR:  cannot transform this Perl type to jsonb

A brief research has shown that the problem is in an incomplete logic inside the transform function. The reason can be illustrated by the flollowing Perl one-liner:


perl -MDevel::Peek  -e 'sub x { my $x = shift; Dump $x; warn "----\n\n"; }; x(undef); x("a"); x(undef); '

It outputs:
SV = NULL(0x0) at 0x73a1b8
  REFCNT = 1
  FLAGS = (PADMY)
----

SV = PV(0x71da50) at 0x73a1b8
  REFCNT = 1
  FLAGS = (PADMY,POK,pPOK)
  PV = 0x7409a0 "a"\0
  CUR = 1
  LEN = 16
----

SV = PV(0x71da50) at 0x73a1b8
  REFCNT = 1
  FLAGS = (PADMY)
  PV = 0x7409a0 "a"\0
  CUR = 1
  LEN = 16
----

This shows that internal representation of the same undef  in perl is different in first and third function calls. 
It is the way Perl reuses the the lexical variable, probably, for optimization reasons.

Current jsonb_plperl implementation works good for the first (most evident) case, but does not work at all for the third, which results in the abovementioned error.

The attached patch solves this issue and defines corresponding tests.

Regards,
Ivan

=?UTF-8?B?SXZhbiBQYW5jaGVua28=?= <wao@mail.ru> writes:
> I have found a bug in jsonb_plperl extension. A possible fix is proposed below.
> ...
> +                /* SVt_PV without POK flag is also NULL */
> +                if(SvTYPE(in) == SVt_PV)

Ugh.  Doesn't Perl provide some saner way to determine the type of a SV?

The core code seems to think that SvOK() is a sufficient test for an
undef.  Should we be doing that before the switch, perhaps?

(My underlying concern here is mostly about whether we have other
similar bugs.  There are a lot of places checking SvTYPE.)

            regards, tom lane

 Tom Lane <tgl@sss.pgh.pa.us>:

Ivan Panchenko <wao@mail.ru> writes:
> I have found a bug in jsonb_plperl extension. A possible fix is proposed below.
> ...
> + /* SVt_PV without POK flag is also NULL */
> + if(SvTYPE(in) == SVt_PV)

Ugh. Doesn't Perl provide some saner way to determine the type of a SV?

The core code seems to think that SvOK() is a sufficient test for an
undef. Should we be doing that before the switch, perhaps?

Thank you, Tom. Yes, there is a solution with SvOK(), please see the attached patch.

SvOK() check before the switch seems too early, because in such case we would  loose hashes and arrays which are not SvOK. So I put it inside the switch. May be, it's better to remove the switch at all, and rewrite the code with ifs?

(My underlying concern here is mostly about whether we have other
similar bugs. There are a lot of places checking SvTYPE.)

I looked through plperl.c, but found no similar cases of checking SvTYPE.

regards, tom lane

Regards, Ivan

=?UTF-8?B?SXZhbiBQYW5jaGVua28=?= <wao@mail.ru> writes:
> Tom Lane <tgl@sss.pgh.pa.us>:
>> The core code seems to think that SvOK() is a sufficient test for an
>> undef.  Should we be doing that before the switch, perhaps?

> Thank you, Tom. Yes, there is a solution with SvOK(), please see the attached patch.

Yeah, that looks cleaner.  I suppose we could get rid of the switch()
but it would result in a bigger diff for not much reason.

>> (My underlying concern here is mostly about whether we have other
>> similar bugs.  There are a lot of places checking SvTYPE.)

> I looked through plperl.c, but found no similar cases of checking SvTYPE.

Yeah, at least there are no other places explicitly checking for
SVt_NULL.

Pushed with minor fiddling with the test case.  Thanks!

            regards, tom lane