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

Particular test asserts path_len == 0, but only with run-tests #1875

Closed
pallas opened this issue Mar 26, 2019 · 4 comments
Closed

Particular test asserts path_len == 0, but only with run-tests #1875

pallas opened this issue Mar 26, 2019 · 4 comments

Comments

@pallas
Copy link
Contributor

pallas commented Mar 26, 2019

Describe the bug
With run-tests, the following test asserts.

[1,5,3]
[1]
*** Expected [1], but got [0] for test at line number 3: path(.[] | select(.,3))
jq: src/execute.c:295: stack_restore: Assertion `path_len == 0' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff76bc88b in raise () from /lib64/libc.so.6
#0  0x00007ffff76bc88b in raise () from /lib64/libc.so.6
#1  0x00007ffff76a57a5 in abort () from /lib64/libc.so.6
#2  0x00007ffff76a567f in __assert_fail_base.cold.0 () from /lib64/libc.so.6
#3  0x00007ffff76b40b2 in __assert_fail () from /lib64/libc.so.6
#4  0x0000555555562f53 in stack_restore (jq=jq@entry=0x55555565d790) at src/execute.c:295
#5  0x0000555555562ff8 in jq_reset (jq=jq@entry=0x55555565d790) at src/execute.c:305
#6  0x000055555556f2cb in jq_teardown (jq=<optimized out>) at src/execute.c:1059
#7  0x00005555555756c2 in run_jq_tests (lib_dirs=..., verbose=verbose@entry=0, testdata=testdata@entry=0x55555565d560) at src/jq_test.c:181
#8  0x0000555555575ec1 in jq_testsuite (libdirs=..., verbose=0, argc=1, argv=0x7fffffffe1c8) at src/jq_test.c:22
#9  0x000055555555e647 in main (argc=3, argv=0x7fffffffe1b8) at src/main.c:528

It does not fail outside of run-tests, e.g. with echo '[1,2,3]' | jq 'path(.[] | select(.,3))'

To Reproduce
See above.

Expected behavior
No assert.

Environment (please complete the following information):

  • OS and Version: Gentoo/Linux
  • jq version: jq-1.6 & jq-1.5

Additional context
Found by American Fuzzy Lop, based on the following testsuite test.

[1,5,3]
[1]
@nicowilliams
Copy link
Contributor

Fascinating! Thanks for the report!

@nicowilliams
Copy link
Contributor

nicowilliams commented Mar 26, 2019

Ah, yes, the issue is that the test did not consume enough outputs from the jq state machine. There's a bug there, but it only affects tests, at least in this case.

$ jq --run-tests
path(.[] | select(.,3))
Testing 'path(.[] | select(.,3))' at line number 1
[1,5,3]
[0]
[0]
[1]
[1]
[2]
[2]

1 of 1 tests passed (0 malformed)

The issue has to do with the argument to select() being a generator, then abandoning the jq VM before it's done producing all its outputs.

I would have expected that this jq program would exercise the bug, path(.[] | select(.,halt)), but it did not.

@nicowilliams
Copy link
Contributor

The jq processor in the co-routines branch is susceptible:

$ ./jq -cn 'coexp([0]|path(.[] | select(.,3))) | fhread'
[0]
jq: src/execute.c:350: stack_restore: Assertion `path_len == 0' failed.
Abort(coredump)

And you can see it's the , operator causing this:

$ ./jq -cn 'coexp([0]|path(.[] | select(.))) | fhread'
[0]
$ 

The simplest thing to do may be to remove the assertion and replace it with resetting jq->path_len, like so:

diff --git a/src/execute.c b/src/execute.c
index f019fcc..7c3f162 100644
--- a/src/execute.c
+++ b/src/execute.c
@@ -347,7 +347,7 @@ uint16_t* stack_restore(jq_state *jq){
     assert(path_len >= 0);
     jq->path = jv_array_slice(jq->path, 0, path_len);
   } else {
-    assert(path_len == 0);
+    fork->path_len = 0;
   }
   jv_free(jq->value_at_path);
   jq->value_at_path = fork->value_at_path;

That patch removes the immediate problem, and I think it's sound (but we should reason about that).

@nicowilliams nicowilliams self-assigned this Mar 26, 2019
@nicowilliams
Copy link
Contributor

BTW, AFL is awesome. I do really need to learn how to use it. Thanks @pallas!

rbolsius pushed a commit to rbolsius/jq that referenced this issue Sep 12, 2019
Expressions of the form `path(EXPR) | select(GENERATOR)`, where `EXPR`
is a path expression and `GENERATOR` is a generator conditional
expression (e.g., `has("a"), has("b")`) cause an assertion if the
jq_state VM is torn down too soon.  That assert() was only correct if
assuming that the conditional is not a generator.

If the conditional is generator, then what we see is that when
backtracking a SUBEXP_END is executed without a corresponding
SUBEXP_BEGIN because the entire conditional is bracketed with
SUBEXP_BEGIN and SUBEXP_END, and since it's resumed in the middle, in
between the brackets.

Rather than assert that the jq->path_len being restored has some
particular value, we can simply re-compute it from the restored
jq->path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants