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

[bindings] Include errno in errors #3403

Merged
merged 6 commits into from
Jul 22, 2022
Merged

[bindings] Include errno in errors #3403

merged 6 commits into from
Jul 22, 2022

Conversation

lrstewart
Copy link
Contributor

Resolved issues:

resolves #3400
resolves #3390

Description of changes:

I added errno to all of our s2n-tls Errors, and used that information to turn ErrorType::IOErrors into more useful std::io::Errors.

I could have only added errno to a new Error::IO variant, but in the past the value of errno has sometimes been useful when debugging other error types. Not usually though: only ErrorType::IOError actually promises that errno will be useful. If folks think it might be more confusing than useful, I can remove it.

Call-outs:

I also made some minor documentation updates and fixed a spacing error that annoyed me while I was trying to verify that relying on errno with IOErrors was at least reasonable.

Testing:

I wrote some unit tests to verify the conversion. I also intentionally caused tests to fail and checked that various errors looked reasonable when printed.

IO error:

Error: Error { code: 67108864, name: "S2N_ERR_IO", message: "underlying I/O operation failed, check system errno", kind: IOError, debug: "Error encountered in lib/stuffer/s2n_stuffer_text.c:70", errno: "Socket operation on non-socket" }

Other errors ("errno" field is present, but not meaningful):

Error: Error { code: 335544320, name: "S2N_ERR_ENCRYPT", message: "error encrypting data", kind: ProtocolError, debug: "Error encountered in lib/stuffer/s2n_stuffer_text.c:70", errno: "Operation now in progress" }
Error: Error { code: 34, name: "Internal s2n error", message: "Internal s2n error", kind: NoError, debug: "Error encountered in lib/stuffer/s2n_stuffer_text.c:70", errno: "Resource temporarily unavailable" }
Error: Error { name: "InvalidInput", message: "A parameter was incorrect" }

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jul 21, 2022
@lrstewart lrstewart marked this pull request as ready for review July 21, 2022 08:24
@lrstewart lrstewart requested review from camshaft and toidiu July 21, 2022 08:24
@@ -39,7 +40,7 @@ impl From<libc::c_int> for ErrorType {
#[derive(PartialEq)]
pub enum Error {
InvalidInput,
Code(s2n_status_code::Type),
Code(s2n_status_code::Type, Errno),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should have Errno as part of our public API. It might be better to do a new type around both s2n_status_code::Type and Errno, just so we're protected from breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion, decided that no details of Error should be public.

So we'll make one breaking change to avoid future breaking changes. Error is now an opaque struct wrapping the original enum, hiding the implementation details. I also gave InvalidInput a kind() (UsageError) so that it can be treated like other errors, and added a source() to more explicitly preserve the original distinction between the two enum variants.

@lrstewart lrstewart requested a review from camshaft July 21, 2022 22:29
Comment on lines 150 to 154
// Previously we referenced InvalidInput via Error::InvalidInput.
// Keep this naming.
// TODO: Update this + all references to all upper case.
#[allow(non_upper_case_globals)]
pub const InvalidInput: Error = Self(Context::InvalidInput);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we are pre 1.0 why not just change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to, but in a separate PR. It's a lot of find and replace that would confuse the diff.

@lrstewart lrstewart requested review from camshaft and toidiu July 22, 2022 03:38
Comment on lines 126 to 131
/// Reports the remaining nanoseconds before the connection may be gracefully shutdown.
///
/// If [`poll_shutdown`] is called before this method reports "0", then an error will occur.
///
/// This method is expected to succeed, but could fail if the underlying C call encounters errors.
/// If it fails, a graceful two-way shutdown of the connection will not be possible.
/// This method is expected to succeed, but could fail if the
/// [underlying C call](`s2n_connection_get_delay`) encounters errors.
/// Failure indicates that calls to [`Self::poll_shutdown`] will also fail and
/// that a graceful two-way shutdown of the connection will not be possible.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this makes more sense. does it also make sense to add/keep

If the application calls [poll_shutdown] before this method reports "0", then an error will occur.

Copy link
Contributor Author

@lrstewart lrstewart Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, because that's not true anymore :) I added a check to poll_shutdown a while back so that it returns Pending instead. The error was because that's how the underlying C shutdown method behaves, but now we don't call that method if it would fail due to blinding. I just forgot to update the documentation.

@lrstewart lrstewart enabled auto-merge (squash) July 22, 2022 20:50
@lrstewart lrstewart merged commit e442191 into aws:main Jul 22, 2022
@lrstewart lrstewart deleted the fix branch July 22, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is remaining_blinding_delay actually fallible? Rust s2n-tls Error loses underlying errno for I/O errors
3 participants