Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_encode mishandles $@ generating error_string #3

Open
tlhackque opened this issue Oct 25, 2016 · 1 comment
Open

_encode mishandles $@ generating error_string #3

tlhackque opened this issue Oct 25, 2016 · 1 comment

Comments

@tlhackque
Copy link
Contributor

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant