Impl drop for HandleData

src/network.rs

@@ -11,7 +11,7 @@ use reqwest::blocking::{Client, RequestBuilder, Response};
 use reqwest::Method;
 use std::ffi::CString;
 use std::os::raw::c_ulonglong;
-use std::ptr;
+use std::ptr::{self, NonNull};
 
 use crate::proj::{ProjError, _string};
 use libc::c_char;
@@ -28,19 +28,18 @@ const RETRY_CODES: [u16; 4] = [429, 500, 502, 504];
 struct HandleData {
     request: reqwest::blocking::RequestBuilder,
     headers: reqwest::header::HeaderMap,
-    // this raw pointer is returned to libproj but never returned from libproj,
-    // so a copy of the pointer (raw pointers are Copy) is stored here, so it can be
-    // reconstituted and dropped in network_close.
+    // this raw pointer is handed out to libproj but never returned,
+    // so a copy of the pointer (raw pointers are Copy) is stored here.
     // Note to future self: are you 100% sure that the pointer is never read again
     // after network_close returns?
-    hptr: Option<*const c_char>,
+    hptr: Option<NonNull<c_char>>,
 }
 
 impl HandleData {
     fn new(
         request: reqwest::blocking::RequestBuilder,
         headers: reqwest::header::HeaderMap,
-        hptr: Option<*const c_char>,
+        hptr: Option<NonNull<c_char>>,
     ) -> Self {
         Self {
             request,
@@ -50,6 +49,16 @@ impl HandleData {
     }
 }
 
+impl Drop for HandleData {
+    // whenever HandleData is dropped we check whether it has a pointer,
+    // dereferencing it if need be so the resource is freed
+    fn drop(&mut self) {
+        if let Some(header) = self.hptr {
+            let _ = unsafe { CString::from_raw(header.as_ptr() as *mut i8) };
+        }
+    }
+}
+
 /// Return an exponential wait time based on the number of retries
 ///
 /// Example: a value of 8 allows up to 6400 ms of retry delay, for a cumulative total of 25500 ms
@@ -176,7 +185,7 @@ unsafe fn _network_open(
     out_size_read.write(contentlength);
     let headers = res.headers().clone();
     // Copy the downloaded bytes into the buffer so it can be passed around
-    &res.bytes()?
+    res.bytes()?
         .as_ptr()
         .copy_to_nonoverlapping(buffer as *mut u8, contentlength.min(size_to_read));
     // Store req into the handle so new ranges can be queried
@@ -198,14 +207,10 @@ pub(crate) unsafe extern "C" fn network_close(
     handle: *mut PROJ_NETWORK_HANDLE,
     _: *mut c_void,
 ) {
-    // Reconstitute the Handle data so it can be dropped
-    let hd = &*(handle as *const c_void as *mut HandleData);
-    // Reconstitute and drop the header value returned by network_get_header_value,
-    // since PROJ never explicitly returns it to us
-    if let Some(header) = hd.hptr {
-        let _ = CString::from_raw(header as *mut i8);
-    }
-    let _ = *hd;
+    // Because we created the raw pointer from a Box, we have to re-constitute the Box
+    // This is the exact reverse order seen in _network_open
+    let void = handle as *mut c_void as *mut HandleData;
+    let _: Box<HandleData> = Box::from_raw(void);
 }
 
 /// Network callback: get header value
@@ -217,7 +222,7 @@ pub(crate) unsafe extern "C" fn network_get_header_value(
     header_name: *const c_char,
     ud: *mut c_void,
 ) -> *const c_char {
-    let mut hd = &mut *(handle as *const c_void as *mut HandleData);
+    let hd = &mut *(handle as *const c_void as *mut HandleData);
     match _network_get_header_value(pc, handle, header_name, ud) {
         Ok(res) => res,
         Err(_) => {
@@ -226,7 +231,7 @@ pub(crate) unsafe extern "C" fn network_get_header_value(
             // unwrapping an empty str is fine
             let cstr = CString::new(hvalue).unwrap();
             let err = cstr.into_raw();
-            hd.hptr = Some(err);
+            hd.hptr = Some(NonNull::new(err).expect("Failed to create non-Null pointer"));
             err
         }
     }
@@ -240,20 +245,23 @@ unsafe fn _network_get_header_value(
     _: *mut c_void,
 ) -> Result<*const c_char, ProjError> {
     let lookup = _string(header_name)?.to_lowercase();
-    let mut hd = &mut *(handle as *mut c_void as *mut HandleData);
+    let hd = &mut *(handle as *mut c_void as *mut HandleData);
     let hvalue = hd
         .headers
         .get(&lookup)
         .ok_or_else(|| ProjError::HeaderError(lookup.to_string()))?
         .to_str()?;
     let cstr = CString::new(hvalue).unwrap();
     let header = cstr.into_raw();
-    // Raw pointers are Copy: the pointer returned by this function is never returned by libproj,so
-    // in order to avoid a memory leak, the pointer is copied and stored in the HandleData struct,
-    // which is dropped in close_network. As part of that, the pointer in hptr is returned to Rust
-    hd.hptr = Some(header);
+    // Raw pointers are Copy: the pointer returned by this function is never returned by libproj so
+    // in order to avoid a memory leak the pointer is copied and stored in the HandleData struct,
+    // which is dropped when close_network returns. As part of that drop, the pointer in hptr is returned to Rust
+    hd.hptr = Some(
+        NonNull::new(header).expect("Failed to create non-Null pointer when building header value"),
+    );
     Ok(header)
 }
+
 /// Network: read range
 ///
 /// Read size_to_read bytes from handle, starting at `offset`, into `buffer`. During this read,
@@ -311,7 +319,7 @@ fn _network_read_range(
     // - 1 is used because the HTTP convention is to use inclusive start and end offsets
     let end = offset as usize + size_to_read - 1;
     let hvalue = format!("bytes={}-{}", offset, end);
-    let mut hd = unsafe { &mut *(handle as *const c_void as *mut HandleData) };
+    let hd = unsafe { &mut *(handle as *const c_void as *mut HandleData) };
     let initial = hd.request.try_clone().ok_or(ProjError::RequestCloneError)?;
     let with_headers = initial.header("Range", &hvalue).header("Client", CLIENT);
     let mut res = with_headers.send()?;