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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions bindings/rust/s2n-tls/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,10 @@ impl Connection {

/// Reports the remaining nanoseconds before the connection may be safely closed.
///
/// If [`shutdown`] is called before this method reports "0", then an error will occur.
/// 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.
pub fn remaining_blinding_delay(&self) -> Result<Duration, Error> {
let nanos = unsafe { s2n_connection_get_delay(self.connection.as_ptr()).into_result() }?;
Ok(Duration::from_nanos(nanos))
Expand Down Expand Up @@ -357,7 +360,7 @@ impl Connection {
/// Sets the currently executing async callback.
///
/// Multiple callbacks can be configured for a connection and config, but
/// [`negotiate`] can only execute and block on one callback at a time.
/// [`poll_negotiate`] can only execute and block on one callback at a time.
/// The handshake is sequential, not concurrent, and stops execution when
/// it encounters an async callback. It does not continue execution (and
/// therefore can't call any other callbacks) until the blocking async callback
Expand Down
88 changes: 80 additions & 8 deletions bindings/rust/s2n-tls/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

use core::{convert::TryInto, fmt, ptr::NonNull, task::Poll};
use errno::{errno, Errno};
use libc::c_char;
use s2n_tls_sys::*;
use std::{convert::TryFrom, ffi::CStr};
Expand Down Expand Up @@ -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.

}

pub trait Fallible {
Expand Down Expand Up @@ -151,14 +152,14 @@ impl Error {
//# an error: s2n_errno = S2N_ERR_T_OK
*s2n_errno = s2n_error_type::OK as _;

Self::Code(code)
Self::Code(code, errno())
}
}

pub fn name(&self) -> &'static str {
match self {
Self::InvalidInput => "InvalidInput",
Self::Code(code) => unsafe {
Self::Code(code, _) => unsafe {
// Safety: we assume the string has a valid encoding coming from s2n
cstr_to_str(s2n_strerror_name(*code))
},
Expand All @@ -168,7 +169,7 @@ impl Error {
pub fn message(&self) -> &'static str {
match self {
Self::InvalidInput => "A parameter was incorrect",
Self::Code(code) => unsafe {
Self::Code(code, _) => unsafe {
// Safety: we assume the string has a valid encoding coming from s2n
cstr_to_str(s2n_strerror(*code, core::ptr::null()))
},
Expand All @@ -178,7 +179,7 @@ impl Error {
pub fn debug(&self) -> Option<&'static str> {
match self {
Self::InvalidInput => None,
Self::Code(code) => unsafe {
Self::Code(code, _) => unsafe {
let debug_info = s2n_strerror_debug(*code, core::ptr::null());

// The debug string should be set to a constant static string
Expand All @@ -198,7 +199,7 @@ impl Error {
pub fn kind(&self) -> Option<ErrorType> {
match self {
Self::InvalidInput => None,
Self::Code(code) => unsafe { Some(ErrorType::from(s2n_error_get_type(*code))) },
Self::Code(code, _) => unsafe { Some(ErrorType::from(s2n_error_get_type(*code))) },
}
}

Expand All @@ -209,7 +210,7 @@ impl Error {
pub fn alert(&self) -> Option<u8> {
match self {
Self::InvalidInput => None,
Self::Code(_code) => {
Self::Code(_, _) => {
None
// TODO: We should use the new s2n-tls method
// once it's available.
Expand Down Expand Up @@ -246,14 +247,20 @@ impl TryFrom<std::io::Error> for Error {

impl From<Error> for std::io::Error {
fn from(input: Error) -> Self {
if let Error::Code(_, errno) = input {
if Some(ErrorType::IOError) == input.kind() {
let bare = std::io::Error::from_raw_os_error(errno.0);
return std::io::Error::new(bare.kind(), input);
}
}
std::io::Error::new(std::io::ErrorKind::Other, input)
}
}

impl fmt::Debug for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let mut s = f.debug_struct("Error");
if let Self::Code(code) = self {
if let Self::Code(code, _) = self {
s.field("code", code);
}

Expand All @@ -268,6 +275,13 @@ impl fmt::Debug for Error {
s.field("debug", &debug);
}

// "errno" is only known to be meaningful for IOErrors.
// However, it has occasionally proved useful for debugging
// other errors, so include it for all errors.
if let Self::Code(_, errno) = self {
s.field("errno", &errno.to_string());
}

s.finish()
}
}
Expand All @@ -279,3 +293,61 @@ impl fmt::Display for Error {
}

impl std::error::Error for Error {}

#[cfg(test)]
mod tests {
use super::*;
use errno::set_errno;

#[test]
fn to_io_error() -> Result<(), Box<dyn std::error::Error>> {
// This relies on an implementation detail of s2n-tls errors,
// and could make this test brittle. However, the alternative
// is a real handshake producing a real IO error, so just updating
// this test if the definition of an IO error changes might be easier.
let s2n_io_error_code = 1 << 26;
let s2n_io_error = Error::Code(s2n_io_error_code, Errno(0));
assert_eq!(Some(ErrorType::IOError), s2n_io_error.kind());

// IO error
{
let s2n_error = Error::Code(s2n_io_error_code, Errno(libc::EACCES));
let io_error = std::io::Error::from(s2n_error);
assert_eq!(std::io::ErrorKind::PermissionDenied, io_error.kind());
assert!(io_error.into_inner().is_some());
}

// Captured IO error
{
let result: isize = -1;
set_errno(Errno(libc::ECONNRESET));
unsafe {
let s2n_errno_ptr = s2n_errno_location();
*s2n_errno_ptr = s2n_io_error_code;
}

let s2n_error = result.into_result().unwrap_err();
let io_error = std::io::Error::from(s2n_error);
assert_eq!(std::io::ErrorKind::ConnectionReset, io_error.kind());
assert!(io_error.into_inner().is_some());
}

// Not IO error
{
let s2n_error = Error::Code(s2n_io_error_code - 1, Errno(libc::ECONNRESET));
let io_error = std::io::Error::from(s2n_error);
assert_eq!(std::io::ErrorKind::Other, io_error.kind());
assert!(io_error.into_inner().is_some());
}

// Not s2n-tls error
{
let s2n_error = Error::InvalidInput;
let io_error = std::io::Error::from(s2n_error);
assert_eq!(std::io::ErrorKind::Other, io_error.kind());
assert!(io_error.into_inner().is_some());
}

Ok(())
}
}
2 changes: 1 addition & 1 deletion docs/USAGE-GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ if (s2n_negotiate(conn, &blocked) < 0) {
case S2N_ERR_T_CLOSED:
return SUCCESS;
case S2N_ERR_T_IO:
handle_io_err();
handle_io_err(errno);
return FAILURE;
case S2N_ERR_T_PROTO:
handle_proto_err();
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -1328,7 +1328,7 @@ int s2n_connection_send_stuffer(struct s2n_stuffer *stuffer, struct s2n_connecti
do {
errno = 0;
w = conn->send(conn->send_io_context, stuffer->blob.data + stuffer->read_cursor, len);
if (w < 0 && errno == EPIPE) {
if (w < 0 && errno == EPIPE) {
conn->write_fd_broken = 1;
}
S2N_ERROR_IF(w < 0 && errno != EINTR, S2N_ERR_SEND_STUFFER_TO_CONN);
Expand Down