jqlang / jq

Command-line JSON processor
https://jqlang.github.io/jq/
Other
29.59k stars 1.54k forks source link

Remove multiple calls to `free` when successively calling `jq_reset`. #3134

Closed Sameesunkaria closed 1 month ago

Sameesunkaria commented 1 month ago

jq_reset calls jv_free on the exit_code and the error_message stored on the jq state. However, it doesn't replace the actual instance of those members. This means that subsequent calls to jq_reset will call jv_free again on those members, which in turn may call free on the same pointer multiple times. Freeing the same pointer multiple times is undefined behavior and can cause heap corruption, which is how I spotted this issue.

In practice, this issue only occurs when using a program that may halt_error, because that is when the exit_code and error_message are set to values other than jv_invalid. Subsequent attempts to call jq_start (which calls jq_reset internally) after hitting a halt_error can cause you to run into this issue.

The changes simply reset the exit_code and the error_message to jv_invalid (the initial value set in jq_init) after they are freed.

wader commented 1 month ago

Hey, thanks. Possible to add a minimal reuse/reset regression test case to https://github.com/jqlang/jq/blob/master/src/jq_test.c that would be detected by valgrind?

Sameesunkaria commented 1 month ago

Added a separate function for testing the jq_state after calls to jq_start. In theory these tests could be baked into the existing run_jq_tests but currently it only ever calls jq_start once for each program. To correctly validate the changes, we must run it against a "dirty" jq_state that was reset by jq_start.

Please let me know if you had something else in mind. :)

itchyny commented 1 month ago

Please drop the change to gitignore file. Files generated not by the project should not be in per-project gitignore file.

Sameesunkaria commented 1 month ago

Removed the commit with the changes to gitignore