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

Rename --nul-output to --raw-output0, abort on string containing NUL #2684

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Jul 9, 2023

The option naming --nul-output was confusing, especially when we have a
similar option for input stream in the future (--nul-input vs --null-input).
Based on the observation of other command line tools, we rename the option
to --raw-output0. We also drop the short option -0 to avoid confusion on
introducing the NUL-delimited input option.

Unlike the other command line tools outputting file names with NUL delimiter,
jq deals with JSON, and its strings may contain NUL character. To protect
users from the risk of injection attacks, we abort the program and print an
error message before outputting strings including NUL character. Closes #2683.

@nicowilliams
Copy link
Contributor

We should first decide whether to keep -0.

@itchyny itchyny force-pushed the fix-nul-option-nul-character branch 2 times, most recently from 86699b4 to 4c61f65 Compare July 22, 2023 14:52
@nicowilliams
Copy link
Contributor

Can you remove -0 while you're at it?

@itchyny itchyny marked this pull request as draft July 25, 2023 13:23
@itchyny itchyny force-pushed the fix-nul-option-nul-character branch from 4c61f65 to 26b0dfa Compare July 26, 2023 12:54
@itchyny itchyny changed the title Fix --nul-output option to abort on a string containing NUL Rename --nul-output to --raw-output0, abort on string containing NUL Jul 26, 2023
@itchyny itchyny force-pushed the fix-nul-option-nul-character branch 2 times, most recently from 1729f19 to ff47773 Compare July 26, 2023 12:56
@itchyny
Copy link
Contributor Author

itchyny commented Jul 26, 2023

There were some proposed options but I'm ok with this plan. What do you think?

@itchyny itchyny marked this pull request as ready for review July 26, 2023 13:04
tests/shtest Show resolved Hide resolved
@wader
Copy link
Member

wader commented Jul 26, 2023

NUL meaning zero byte comes from ASCII? the man page for find calls it "ASCII NUL character (character code 0)" when describing -print0

@itchyny
Copy link
Contributor Author

itchyny commented Jul 26, 2023

Ah, I remember the concern is that it's not abort, but just skip. It exits with non-zero only when the last output contains NUL.

@nicowilliams
Copy link
Contributor

LGTM! Merge at your convenience. I think this gets us very close to release ready. I need to change the release key's name. And I need to figure quite how to do the signing in the actions, unless you already know.

@itchyny itchyny force-pushed the fix-nul-option-nul-character branch from ff47773 to c43f1d3 Compare July 26, 2023 23:24
@itchyny
Copy link
Contributor Author

itchyny commented Jul 26, 2023

Ah, I remember the concern is that it's not abort, but just skip. It exits with non-zero only when the last output contains NUL.

Sorry for pushing after reviewing, but I reconsidered that this should be an error. So I break on such case, and assert that it fails with non-zero status (actually 5), no further output is printed. The patch is now somehow smaller and readable IMO.

The option naming --nul-output was confusing, especially when we have a
similar option for input stream in the future (--nul-input vs --null-input).
Based on the observation of other command line tools, we rename the option
to --raw-output0. We also drop the short option -0 to avoid confusion on
introducing the NUL-delimited input option.

Unlike the other command line tools outputting file names with NUL delimiter,
jq deals with JSON, and its strings may contain NUL character. To protect
users from the risk of injection attacks, we abort the program and print an
error message before outputting strings including NUL character. Closes jqlang#2683.
@itchyny itchyny force-pushed the fix-nul-option-nul-character branch from c43f1d3 to 1ea53bc Compare July 26, 2023 23:30
@nicowilliams nicowilliams merged commit a1e791a into jqlang:master Jul 27, 2023
29 checks passed
@nicowilliams
Copy link
Contributor

Thanks!

wader added a commit to wader/fq that referenced this pull request Aug 8, 2023
Remove -0 short arg, now it means the expression "-0".
This is to bo in sync with jq 1.7 jqlang/jq#2684

Correct and clarify that NUL and new lines are outputted after and not between each output.

Still missing is to abort on output containing zero when using --raw-output0
wader added a commit to wader/fq that referenced this pull request Aug 8, 2023
Remove -0 short arg, now it means the expression "-0".
This is to be in sync with jq 1.7 jqlang/jq#2684

Correct and clarify that NUL and new lines are outputted after and not between each output.

Still missing is to abort on output containing zero when using --raw-output0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing -0/--nul-output before releasing 1.7
3 participants