[PATCH] Fix segmentation fault and infinite loop in jsonb_{plperl,plpython}

Aleksander Alekseev <aleksander@tigerdata.com> writes:

I discovered several bugs in jsonb_plperl and jsonb_plpython.

Good catches, will take care of this.

(I'm inclined to fold these into one patch, though.)

regards, tom lane

Aleksander Alekseev <aleksander@tigerdata.com> writes:

The second bug affects only jsonb_plperl. It's possible to construct a
Perl object with circular references which will cause
SV_to_JsonbValue() to go into an infinite loop here:

while (SvROK(in))
in = SvRV(in);

I suggest fixing it by rewriting the while loop into a recursion with
check_stack_depth() call. This will make the behavior consistent with
jsonb_plpython.

Unfortunately, your 0002 is too cute for its own good. I tried it
here, with a not-especially-new gcc compiling at -O2, and found that
the tail recursion in SV_deref() is optimized into a loop. So the
stack doesn't grow and we still have an uninterruptible loop.

I don't immediately see a way to write that function so that the
compiler is certain not to spot the tail recursion. Tricks like
two mutually recursive functions might be seen through at
sufficiently high -O levels.

We could instead add a CHECK_FOR_INTERRUPTS, so that you can at
least break out of the infinite loop. I'm not sure if the case
is worth more effort than that.

regards, tom lane

Hi Tom,

Thanks for taking a look.

The second bug affects only jsonb_plperl. It's possible to construct a
Perl object with circular references which will cause
SV_to_JsonbValue() to go into an infinite loop here:

while (SvROK(in))
in = SvRV(in);

I suggest fixing it by rewriting the while loop into a recursion with
check_stack_depth() call. This will make the behavior consistent with
jsonb_plpython.

Unfortunately, your 0002 is too cute for its own good. I tried it
here, with a not-especially-new gcc compiling at -O2, and found that
the tail recursion in SV_deref() is optimized into a loop. So the
stack doesn't grow and we still have an uninterruptible loop.

That's funny :)

I don't immediately see a way to write that function so that the
compiler is certain not to spot the tail recursion. Tricks like
two mutually recursive functions might be seen through at
sufficiently high -O levels.

We could instead add a CHECK_FOR_INTERRUPTS, so that you can at
least break out of the infinite loop. I'm not sure if the case
is worth more effort than that.

Alternatively I could implement a check for the loops, if you feel
like this is worth the effort.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@tigerdata.com> writes:

We could instead add a CHECK_FOR_INTERRUPTS, so that you can at
least break out of the infinite loop. I'm not sure if the case
is worth more effort than that.

Alternatively I could implement a check for the loops, if you feel
like this is worth the effort.

I thought about that, but I'm not sure how to build a bulletproof
check at reasonable (ie, near zero) cost. We could detect the example
case where an object refers directly to itself, by noticing that "in"
doesn't change in one iteration. But I'm pretty sure it's possible to
build reference loops involving two or more Perl objects, and those
would fool such a check.

regards, tom lane

Hi,

Alternatively I could implement a check for the loops, if you feel
like this is worth the effort.

I thought about that, but I'm not sure how to build a bulletproof
check at reasonable (ie, near zero) cost. We could detect the example
case where an object refers directly to itself, by noticing that "in"
doesn't change in one iteration. But I'm pretty sure it's possible to
build reference loops involving two or more Perl objects, and those
would fool such a check.

I was thinking about depth-first search where we store our current
path in a set. If the visited node is already in the set then the
graph has loops.

This is not exactly cheap but the complexity is proportional to the
cost of the serialization so I think we should be fine. The good thing
is that the user will get a sensible error message instead of an
infinite loop.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@tigerdata.com> writes:

I thought about that, but I'm not sure how to build a bulletproof
check at reasonable (ie, near zero) cost. We could detect the example
case where an object refers directly to itself, by noticing that "in"
doesn't change in one iteration. But I'm pretty sure it's possible to
build reference loops involving two or more Perl objects, and those
would fool such a check.

I was thinking about depth-first search where we store our current
path in a set. If the visited node is already in the set then the
graph has loops.

This is not exactly cheap but the complexity is proportional to the
cost of the serialization so I think we should be fine.

No, it'd be O(N^2) for an N-deep reference chain. Admittedly,
realistic use-cases would never have more than a couple of layers of
indirection. But this whole exercise is to guard against adversarial
inputs, I think. I don't really want to add cycles and complexity to
make our behavior a bit more friendly in cases that nobody is going
to get into unless they are trying to break the database.

regards, tom lane

Hi Tom,

Thanks for all your great feedback.

No, it'd be O(N^2) for an N-deep reference chain. Admittedly,
realistic use-cases would never have more than a couple of layers of
indirection. But this whole exercise is to guard against adversarial
inputs, I think. I don't really want to add cycles and complexity to
make our behavior a bit more friendly in cases that nobody is going
to get into unless they are trying to break the database.

OK, that's fair enough. Here is the corrected patch.

```
=# SELECT perl_circular();
^CCancel request sent
ERROR: canceling statement due to user request
CONTEXT: PL/Perl function "perl_circular"
```

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@tigerdata.com> writes:

OK, that's fair enough. Here is the corrected patch.

Pushed with some comments added.

regards, tom lane

Hi Tom,

Pushed with some comments added.

Thanks for applying the patch.

I looked around and found the same bug in hstore_plperl:

```
=# CREATE EXTENSION IF NOT EXISTS hstore_plperl CASCADE;
CREATE EXTENSION

=# CREATE OR REPLACE FUNCTION hstore_circular() RETURNS hstore
LANGUAGE plperl TRANSFORM FOR TYPE hstore AS $$
my $x;
$x = \$x;
return $x;
$$;
CREATE FUNCTION

=# SELECT hstore_circular();
^CCancel request sent
^CCancel request sent
^CCancel request sent
```

From what I can tell this is the last issue like this. Here is the patch.

Would you like to merge it as well or will it be better to start a new
thread for visibility?

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@tigerdata.com> writes:

I looked around and found the same bug in hstore_plperl:

D'oh, I should have spotted that myself. Pushed, thanks for noticing.

regards, tom lane