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

Implementation of the error buffer in Easy2 is unsound #589

Open
kadiwa4 opened this issue Nov 22, 2024 · 1 comment
Open

Implementation of the error buffer in Easy2 is unsound #589

kadiwa4 opened this issue Nov 22, 2024 · 1 comment

Comments

@kadiwa4
Copy link

kadiwa4 commented Nov 22, 2024

curl::easy::Easy2::default_configure registers an error buffer like this:

self.setopt_ptr(
    curl_sys::CURLOPT_ERRORBUFFER,
    self.inner.error_buf.borrow().as_ptr() as *const _,
)
.expect("failed to set error buffer");

CURLOPT_ERRORBUFFER requires mutable access over the buffer, and it is important that the buffer pointer is still valid later on when further library calls are made. However, error_buf is borrowed immutably and only for a short time (until the end of the statement).

(A mutable borrow might look like this:

self.inner.error_buf.borrow_mut().as_mut_ptr() as *const u8 as *const _

That does not solve the lifetime problem, though.)

@kadiwa4
Copy link
Author

kadiwa4 commented Dec 23, 2024

I see a couple of ways how this could be solved:

  1. Using a raw pointer to the error buffer with no extra safety features. This is functionally equivalent to the status quo. No idea if this is sound; that would depend on libcurl. The documentation page on error buffers is not very detailed. This option is kinda bad because unsafe would be spread over the entire file instead of being contained in an abstraction.
    struct Inner<H> {
        // ...
        error_buf: *mut u8,
        // ...
    }
  2. Registering a new error buffer for every call to libcurl and then reregistering the previous buffer afterwards.
  3. Creating a custom cell type (HandleCell) similar to a RefCell that ensures that you never have a RawHandle and an ErrorBuf at the same time. This would prevent reentrant usage of curl functions. That might be a breaking change if reentrant usage is currently allowed.
    mod cell {
        // notably not Clone or Copy
        struct CurlHandle<'a>(*mut curl_sys::CURL, &'a Cell<HandleCellState>);
        impl CurlHandle<'_> {
            pub fn setopt_long(
                &mut self,
                opt: curl_sys::CURLoption,
                val: c_long
            ) -> Result<(), Error> { /* ... */ }
            /* + all other functions which call a libcurl fn with a handle ... */ 
        }
        impl Drop for CurlHandle<'_> { /* reset state of cell */ }
    
        struct ErrorBufRef<'a>(&'a [u8], &'a Cell<HandleCellState>);
        impl Drop for ErrorBufRef<'_> { /* reset state of cell */ }
    
        pub struct HandleCell {
            state: Cell<HandleCellState>,
            handle: *mut curl_sys::CURL,
            error_buf: Vec<u8>,
        }
    
        /// What field is currently borrowed?
        enum HandleCell {
            None,
            Handle,
            ErrorBuf,
        }
    
        impl HandleCell {
            pub fn get_handle(&self) -> CurlHandle<'_> { /* ... */ }
            pub fn get_error_buf(&self) -> ErrorBufRef<'_> { /* ... */ }
        }
    }
  4. Same as 3., except that reentrant usage is made possible. If a reentrant (non-outermost) call to libcurl ends up failing, it would not be possible to read the error buffer and the standard error message would be used instead.

I prefer option 1. because I'd assume that it is always okay to read the error buffer whenever our machine is not executing code from libcurl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant