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

[apps/genpkey] exit status should not be 0 on output errors #12305

Conversation

@romen
Copy link
Member

@romen romen commented Jun 28, 2020

TL;DR

If the key is to be serialized or printed as text and the framework returns an error, the app should signal the failure to the user using a non-zero exit status.

History

From 1.1.1-stable (this snippet did not change in master, only shifted by a few lines):

openssl/apps/genpkey.c

Lines 180 to 199 in 7bdf1ee

if (rv <= 0) {
BIO_puts(bio_err, "Error writing key\n");
ERR_print_errors(bio_err);
}
if (text) {
if (do_param)
rv = EVP_PKEY_print_params(out, pkey, 0, NULL);
else
rv = EVP_PKEY_print_private(out, pkey, 0, NULL);
if (rv <= 0) {
BIO_puts(bio_err, "Error printing key\n");
ERR_print_errors(bio_err);
}
}
ret = 0;
end:

The code above has been unchanged (apart from formatting) for the last 14 years.

In case of an error writing the key to a file or in textual form to the terminal, the exit status of the command is still reset to 0.

Branches

This should be fixed both in master and in 1.1.1

If the key is to be serialized or printed as text and the framework
returns an error, the app should signal the failure to the user using
a non-zero exit status.
@romen
Copy link
Member Author

@romen romen commented Jun 28, 2020

@mattcaswell this command has been "misbehaving" this way for a very long time and across releases.

Yet I don't think these 2 error paths have been triggered often, as excluding the current status of master with the serializer implementation still being finished, my guts say errors that could have resulted in either of these 2 steps failing would have been caught within a goto end block, before reaching this part of genpkey_main.

Anyway, one could argue that fixing this bug is changing an established behavior of genpkey:

  • do we require a discussion and vote to change this in 1.1.1?
  • do we require a discussion and vote to change this in master?
@romen romen mentioned this pull request Jun 28, 2020
13 of 19 tasks complete
@levitte
Copy link
Member

@levitte levitte commented Jun 28, 2020

I would rather move ret = 0 to line 192 and replace your goto end with ret++

@romen
Copy link
Member Author

@romen romen commented Jun 28, 2020

I would rather move ret = 0 to line 192 and replace your goto end with ret++

I like it! I actually fixed it like that first, but later changed to use goto just for conformity with our usual handling and to shorten the discussion!

Fixup pushed!

@romen
Copy link
Member Author

@romen romen commented Jun 28, 2020

Travis failure is due to #12306 (the failure was already there, both in 1.1.1 and master, this PR just bring it to the surface!)

@richsalz
Copy link
Contributor

@richsalz richsalz commented Jun 28, 2020

ret++ Why not ret = 1?

@romen
Copy link
Member Author

@romen romen commented Jun 28, 2020

ret++ Why not ret = 1?

@richsalz either works for me, I guess both 1 and 2 are valid "failure" exit statuses, but I agree that it does not add much more information than is already reported on the screen to know which of the two is failing.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Jun 28, 2020

I only brought it up because there are very few instances where an exit code is meaningful. (Verify, maybe?)

@paulidale paulidale dismissed their stale review Jun 29, 2020

Test failures seem relevant.

@romen
Copy link
Member Author

@romen romen commented Jun 29, 2020

@paulidale Yes, the failures are due to #12306

I'll soon open PRs to fix it in both master and 1.1.1, and I will update this PR and the 1.1.1 one to rebase on top of those and to further test out these problematic curves!

@romen
Copy link
Member Author

@romen romen commented Jun 29, 2020

@paulidale

Travis failures are expected!

master chain of PRs

PRs #12305, #12313 and #12307 depend on each other.

Check #12307 (based on top of the other 2) for (hopefully) green Travis.

1.1.1 chain of PRs

PRs #12305, #12312 and #12308 depend on each other.

Check #12308 (based on top of the other 2) for (hopefully) green Travis.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Jul 4, 2020

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@romen
Copy link
Member Author

@romen romen commented Jul 4, 2020

CI failures are expected, and fixed in #12307 / #12308 which have been approved.

They will be merged together

romen added a commit to romen/openssl that referenced this pull request Jul 4, 2020
If the key is to be serialized or printed as text and the framework
returns an error, the app should signal the failure to the user using
a non-zero exit status.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#12305)

(cherry picked from commit 5858b18)
romen added a commit to romen/openssl that referenced this pull request Jul 4, 2020
If the key is to be serialized or printed as text and the framework
returns an error, the app should signal the failure to the user using
a non-zero exit status.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#12305)
openssl-machine pushed a commit that referenced this pull request Jul 7, 2020
If the key is to be serialized or printed as text and the framework
returns an error, the app should signal the failure to the user using
a non-zero exit status.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #12305)
openssl-machine pushed a commit that referenced this pull request Jul 7, 2020
If the key is to be serialized or printed as text and the framework
returns an error, the app should signal the failure to the user using
a non-zero exit status.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #12305)

(cherry picked from commit 466d30c)
@romen
Copy link
Member Author

@romen romen commented Jul 7, 2020

Merged to master as:

  • 466d30c [apps/genpkey] exit status should not be 0 on output errors

Merged to 1.1.1 as:

  • 1940c09 [apps/genpkey] exit status should not be 0 on output errors

Thanks!

@romen romen closed this Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants