Skip to content
New issue

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

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

Already on GitHub? # to your account

Remove undocumented and untested --stream-errors option #2748

Closed
wants to merge 1 commit into from

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Jul 22, 2023

This PR drops --stream-errors option. This option is undocumented, and untested. Also buggy; printf '[' | jq --stream-errors . aborts with assertion error.

@itchyny itchyny added this to the 1.7 release milestone Jul 22, 2023
@nicowilliams
Copy link
Contributor

nicowilliams commented Jul 22, 2023

EDIT: printf '[' | ./jq -c --stream --stream-errors . does not abort (it outputs ["Unfinished JSON term at EOF at line 1, column 1",[0]]). So another fix would be to make --stream-errors imply --stream.

It should be an easy fix:

(gdb) p jv_show(result,0)
["Unfinished JSON term at EOF at line 2, column 0",<invalid>]$1 = void
(gdb)

The problem is that we need to check for .[1] of result being invalid, and then do something about that: dump just .[0] of result, or drop the .[1] of `result.

Now, the question is: is --stream-errors useful? I think it should be!

And this doesn't abort:

: ; printf '[' | ./jq --stream-errors '.[0]'
"Unfinished JSON term at EOF at line 1, column 1"

@nicowilliams
Copy link
Contributor

And:

: ; (./jq -cn '[{a:{b:["c"]}}]'; printf '['; ./jq -cn '["a",{b:"c"}]') | ./jq -c --stream --stream-error
s .
[[0,"a","b",0],"c"]
[[0,"a","b",0]]
[[0,"a","b"]]
[[0,"a"]]
[[0]]
[[0,0],"a"]
[[0,1,"b"],"c"]
[[0,1,"b"]]
[[0,1]]
["Unfinished JSON term at EOF at line 3, column 0",[0]]

@nicowilliams
Copy link
Contributor

One thing we should do is write a fuzzer for the streaming parser. Also, we should document what is and what is not fuzzed so that users who need better security know what features to use / not use.

@itchyny itchyny marked this pull request as draft July 22, 2023 23:12
@itchyny
Copy link
Contributor Author

itchyny commented Jul 22, 2023

Okay, we need to fix the crash, and update the help (after #2747) and the manual.

@nicowilliams
Copy link
Contributor

Okay, we need to fix the crash, and update the help (after #2747) and the manual.

I'll try to get to it tomorrow. Maybe or maybe later tonight.

nicowilliams added a commit to nicowilliams/jq that referenced this pull request Jul 23, 2023
nicowilliams added a commit to nicowilliams/jq that referenced this pull request Jul 23, 2023
nicowilliams added a commit to nicowilliams/jq that referenced this pull request Jul 23, 2023
nicowilliams added a commit to nicowilliams/jq that referenced this pull request Jul 23, 2023
nicowilliams added a commit to nicowilliams/jq that referenced this pull request Jul 23, 2023
nicowilliams added a commit to nicowilliams/jq that referenced this pull request Jul 23, 2023
nicowilliams added a commit to nicowilliams/jq that referenced this pull request Jul 23, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants