geofffranks / json-api

A perl module to simplify interaction with JSON APIs
Other
6 stars 6 forks source link

_encode mishandles $@ generating error_string #3

Open tlhackque opened 7 years ago

tlhackque commented 7 years ago

I don't have a reproducer for generating bad JSON, but code review revealed a bug in _encode.

As I haven't created a reproducer in your module, I don't plan to generate a PR. But you probably should produce a regression test and patch this.

In _encode, you attempt to remove the calling location from $@ with:

$self->{error_string} =~ s/\s+at\s+\S+\s+line\s+\d+\.?\s*//;

This doesn't take account of the file position that is sometimes included by die and croak.

Here's an example outside of JSON::API (type <CR> twice):

 perl -e'$x=<>.<>;eval{die "foo"}; print "$@\n"; $@ =~ s/\s+at\s+\S+\s+line\s+\d+\.?\s*//; print $@,"\n"'
foo at -e line 1, <> line 2.

foo, <> line 2.

Note that , <> line can be any file handle, and line can also be `chunk'.

You probably intended:

$self->{error_string} =~ s/\s+at\s+\S+\s+line\s+\d+(?:,\s+\S+\s+(?:line|chunk)?\s+\d+)?\..*\z//s; 

 perl -e'$x=<>.<STDIN>;eval{die "foo"}; print "$@\n"; $@ =~ s/\s+at\s+\S+\s+line\s+\d+(?:,\s+\S+\s+(?:line|chunk)\s+\d+)?\..*\z//s; print $@,"\n"'
foo at -e line 1, <STDIN> line 2.

foo

Note that JSON uses Carp::croak() rather than die(). E.g. you need to handle multiple lines:

perl -MCarp -e'$x=<>.<STDIN>;eval{croak "foo"}; print "$@\n"; $@ =~ s/\s+at\s+\S+\s+line\s+\d+(?:,\s+\S+\s+(?:line|chunk)\s+\d+)?\..*\z//s; print $@,"\n"'
foo at -e line 1, <STDIN> line 1.
        eval {...} called at -e line 1

foo

Carp was "recently" patched to include the file position, so you may need to update to see this.

tlhackque commented 7 years ago

The same bug existis in _decode.

We should be able to avoid the whole mess by localizing a%CarpInternal entry.

That way, any error would be reported from the perspective of JSON::API's caller - and it's the user's job to decide if (s)he wants location information.

This is actually the better approach; JSON::API should not be hiding error information.

But it turns out that JSON::XS is calling die() rather than croak(). I've contacted the owner, as this is different from JSON::PP - and wrong.

Here's a reproducer in JSON::API:

perl  -MJSON::API -e'$x=<STDIN>;$x=JSON::API->new( "http://localhost:999" ); $x->put("/", "}{"); print $x->errstr,"\n"'

hash- or arrayref expected (not a simple scalar, use allow_nonref to allow this), <STDIN> line 1.

The fix in JSON::API is easy - and tested with JSON::PP as the backend.

I'll do a PR once the JSON::XS owner agrees to fix his code.