diff options
author | Andrew Walbran <qwandor@google.com> | 2023-07-21 13:28:31 +0100 |
---|---|---|
committer | Andrew Walbran <qwandor@google.com> | 2023-07-24 10:51:43 +0100 |
commit | a64e3c15d3b38293f85a4c1f819d4544a07a9ad6 (patch) | |
tree | a55a36e9aafce6e9fa3fb9feb5857d44b80df86a | |
parent | 2bdd5d4540ae668978893d28d628cd34228377dd (diff) | |
download | DnsResolver-a64e3c15d3b38293f85a4c1f819d4544a07a9ad6.tar.gz |
Add safety comments.
These will soon be required by a lint. I've also marked some functions
as unsafe that were incorrectly marked as safe.
Bug: 290018030
Change-Id: I866c1016020daafef462869a9f15ae7f0acc81dc
-rw-r--r-- | doh/boot_time.rs | 16 | ||||
-rw-r--r-- | doh/connection/mod.rs | 2 | ||||
-rw-r--r-- | doh/ffi.rs | 54 | ||||
-rw-r--r-- | doh/tests/doh_frontend/src/dns_https_frontend.rs | 3 | ||||
-rw-r--r-- | doh/tests/doh_frontend/src/ffi.rs | 28 |
5 files changed, 68 insertions, 35 deletions
diff --git a/doh/boot_time.rs b/doh/boot_time.rs index 453235ba..666a44de 100644 --- a/doh/boot_time.rs +++ b/doh/boot_time.rs @@ -57,8 +57,7 @@ impl BootTime { /// Gets a `BootTime` representing the current moment in time. pub fn now() -> BootTime { let mut t = libc::timespec { tv_sec: 0, tv_nsec: 0 }; - // # Safety - // clock_gettime's only action will be to possibly write to the pointer provided, + // SAFETY: clock_gettime's only action will be to possibly write to the pointer provided, // and no borrows exist from that object other than the &mut used to construct the pointer // itself. if unsafe { libc::clock_gettime(libc::CLOCK_BOOTTIME, &mut t as *mut libc::timespec) } != 0 @@ -93,9 +92,8 @@ struct TimerFd(RawFd); impl Drop for TimerFd { fn drop(&mut self) { - // # Safety - // The fd is owned by the TimerFd struct, and no memory access occurs as a result of this - // call. + // SAFETY: The fd is owned by the TimerFd struct, and no memory access occurs as a result of + // this call. unsafe { libc::close(self.0); } @@ -110,9 +108,8 @@ impl AsRawFd for TimerFd { impl TimerFd { fn create() -> io::Result<Self> { - // # Unsafe - // This libc call will either give us back a file descriptor or fail, it does not act on - // memory or resources. + // SAFETY: This libc call will either give us back a file descriptor or fail, it does not + // act on memory or resources. let raw = unsafe { libc::timerfd_create(libc::CLOCK_BOOTTIME, libc::TFD_NONBLOCK | libc::TFD_CLOEXEC) }; @@ -131,8 +128,7 @@ impl TimerFd { tv_nsec: duration.subsec_nanos().try_into().unwrap(), }, }; - // # Unsafe - // We own `timer` and there are no borrows to it other than the pointer we pass to + // SAFETY: We own `timer` and there are no borrows to it other than the pointer we pass to // timerfd_settime. timerfd_settime is explicitly documented to handle a null output // parameter for its fourth argument by not filling out the output. The fd passed in at // self.0 is owned by the `TimerFd` struct, so we aren't breaking anyone else's invariants. diff --git a/doh/connection/mod.rs b/doh/connection/mod.rs index f0b27d79..5898228c 100644 --- a/doh/connection/mod.rs +++ b/doh/connection/mod.rs @@ -61,7 +61,7 @@ fn new_scid() -> [u8; quiche::MAX_CONN_ID_LEN] { fn mark_socket(socket: &std::net::UdpSocket, socket_mark: u32) -> io::Result<()> { use std::os::unix::io::AsRawFd; let fd = socket.as_raw_fd(); - // libc::setsockopt is a wrapper function calling into bionic setsockopt. + // SAFETY: libc::setsockopt is a wrapper function calling into bionic setsockopt. // The only pointer being passed in is &socket_mark, which is valid by virtue of being a // reference, and the foreign function doesn't take ownership or a reference to that memory // after completion. @@ -35,8 +35,12 @@ use tokio::sync::oneshot; use tokio::task; use url::Url; -pub type ValidationCallback = - extern "C" fn(net_id: uint32_t, success: bool, ip_addr: *const c_char, host: *const c_char); +pub type ValidationCallback = unsafe extern "C" fn( + net_id: uint32_t, + success: bool, + ip_addr: *const c_char, + host: *const c_char, +); pub type TagSocketCallback = extern "C" fn(sock: RawFd); #[repr(C)] @@ -61,7 +65,9 @@ fn wrap_validation_callback(validation_fn: ValidationCallback) -> ValidationRepo } }; let netd_id = info.net_id; - task::spawn_blocking(move || { + // SAFETY: The string pointers are obtained from `CString`, so they must be valid C + // strings. + task::spawn_blocking(move || unsafe { validation_fn(netd_id, success, ip_addr.as_ptr(), domain.as_ptr()) }) .await @@ -167,12 +173,16 @@ pub extern "C" fn doh_dispatcher_new( } /// Deletes a DoH engine created by doh_dispatcher_new(). +/// /// # Safety +/// /// `doh` must be a non-null pointer previously created by `doh_dispatcher_new()` /// and not yet deleted by `doh_dispatcher_delete()`. #[no_mangle] pub unsafe extern "C" fn doh_dispatcher_delete(doh: *mut DohDispatcher) { - Box::from_raw(doh).lock().exit_handler() + // SAFETY: The caller guarantees that `doh` was created by `doh_dispatcher_new` (which does so + // using `Box::into_raw`), and that it hasn't yet been deleted by this function. + unsafe { Box::from_raw(doh) }.lock().exit_handler() } /// Probes and stores the DoH server with the given configurations. @@ -194,12 +204,15 @@ pub unsafe extern "C" fn doh_net_new( network_type: uint32_t, private_dns_mode: uint32_t, ) -> int32_t { - let (url, domain, ip_addr, cert_path) = match ( - std::ffi::CStr::from_ptr(url).to_str(), - std::ffi::CStr::from_ptr(domain).to_str(), - std::ffi::CStr::from_ptr(ip_addr).to_str(), - std::ffi::CStr::from_ptr(cert_path).to_str(), - ) { + // SAFETY: The caller guarantees that these are all valid nul-terminated C strings. + let (url, domain, ip_addr, cert_path) = match unsafe { + ( + std::ffi::CStr::from_ptr(url).to_str(), + std::ffi::CStr::from_ptr(domain).to_str(), + std::ffi::CStr::from_ptr(ip_addr).to_str(), + std::ffi::CStr::from_ptr(cert_path).to_str(), + ) + } { (Ok(url), Ok(domain), Ok(ip_addr), Ok(cert_path)) => { if domain.is_empty() { (url, None, ip_addr.to_string(), None) @@ -268,7 +281,9 @@ pub unsafe extern "C" fn doh_query( response_len: size_t, timeout_ms: uint64_t, ) -> ssize_t { - let q = slice::from_raw_parts_mut(dns_query, dns_query_len); + // SAFETY: The caller guarantees that `dns_query` is a valid pointer to a buffer of at least + // `dns_query_len` items. + let q = unsafe { slice::from_raw_parts_mut(dns_query, dns_query_len) }; let (resp_tx, resp_rx) = oneshot::channel(); let t = Duration::from_millis(timeout_ms); @@ -298,7 +313,10 @@ pub unsafe extern "C" fn doh_query( if answer.len() > response_len || answer.len() > isize::MAX as usize { return DOH_RESULT_INTERNAL_ERROR; } - let response = slice::from_raw_parts_mut(response, answer.len()); + // SAFETY: The caller guarantees that response points to a valid buffer at + // least `response_len` long, and we just checked that `answer.len()` is no + // longer than `response_len`. + let response = unsafe { slice::from_raw_parts_mut(response, answer.len()) }; response.copy_from_slice(&answer); answer.len() as ssize_t } @@ -341,25 +359,27 @@ mod tests { const LOOPBACK_ADDR: &str = "127.0.0.1:443"; const LOCALHOST_URL: &str = "https://mylocal.com/dns-query"; - extern "C" fn success_cb( + unsafe extern "C" fn success_cb( net_id: uint32_t, success: bool, ip_addr: *const c_char, host: *const c_char, ) { assert!(success); + // SAFETY: The caller guarantees that ip_addr and host are valid nul-terminated C strings. unsafe { assert_validation_info(net_id, ip_addr, host); } } - extern "C" fn fail_cb( + unsafe extern "C" fn fail_cb( net_id: uint32_t, success: bool, ip_addr: *const c_char, host: *const c_char, ) { assert!(!success); + // SAFETY: The caller guarantees that ip_addr and host are valid nul-terminated C strings. unsafe { assert_validation_info(net_id, ip_addr, host); } @@ -373,10 +393,12 @@ mod tests { host: *const c_char, ) { assert_eq!(net_id, TEST_NET_ID); - let ip_addr = std::ffi::CStr::from_ptr(ip_addr).to_str().unwrap(); + // SAFETY: The caller guarantees that `ip_addr` is a valid nul-terminated C string. + let ip_addr = unsafe { std::ffi::CStr::from_ptr(ip_addr) }.to_str().unwrap(); let expected_addr: SocketAddr = LOOPBACK_ADDR.parse().unwrap(); assert_eq!(ip_addr, expected_addr.ip().to_string()); - let host = std::ffi::CStr::from_ptr(host).to_str().unwrap(); + // SAFETY: The caller guarantees that `host` is a valid nul-terminated C string. + let host = unsafe { std::ffi::CStr::from_ptr(host) }.to_str().unwrap(); assert_eq!(host, ""); } diff --git a/doh/tests/doh_frontend/src/dns_https_frontend.rs b/doh/tests/doh_frontend/src/dns_https_frontend.rs index b7d11b7d..d3c538d4 100644 --- a/doh/tests/doh_frontend/src/dns_https_frontend.rs +++ b/doh/tests/doh_frontend/src/dns_https_frontend.rs @@ -477,6 +477,9 @@ fn into_tokio_udp_socket(socket: std::net::UdpSocket) -> Result<UdpSocket> { fn build_pipe() -> Result<(File, File)> { let mut fds = [0, 0]; + // SAFETY: The pointer we pass to `pipe` must be valid because it comes from a reference. The + // file descriptors it returns must be valid and open, so they are safe to pass to + // `File::from_raw_fd`. unsafe { if libc::pipe(fds.as_mut_ptr()) == 0 { return Ok((File::from_raw_fd(fds[0]), File::from_raw_fd(fds[1]))); diff --git a/doh/tests/doh_frontend/src/ffi.rs b/doh/tests/doh_frontend/src/ffi.rs index 3235d5da..37a5fba0 100644 --- a/doh/tests/doh_frontend/src/ffi.rs +++ b/doh/tests/doh_frontend/src/ffi.rs @@ -38,10 +38,14 @@ pub unsafe extern "C" fn frontend_new( backend_addr: *const c_char, backend_port: *const c_char, ) -> *mut DohFrontend { - let addr = CStr::from_ptr(addr).to_str().unwrap(); - let port = CStr::from_ptr(port).to_str().unwrap(); - let backend_addr = CStr::from_ptr(backend_addr).to_str().unwrap(); - let backend_port = CStr::from_ptr(backend_port).to_str().unwrap(); + // SAFETY: Our caller promises that addr is a valid C string. + let addr = unsafe { CStr::from_ptr(addr) }.to_str().unwrap(); + // SAFETY: Our caller promises that port is a valid C string. + let port = unsafe { CStr::from_ptr(port) }.to_str().unwrap(); + // SAFETY: Our caller promises that backend_addr is a valid C string. + let backend_addr = unsafe { CStr::from_ptr(backend_addr) }.to_str().unwrap(); + // SAFETY: Our caller promises that backend_port is a valid C string. + let backend_port = unsafe { CStr::from_ptr(backend_port) }.to_str().unwrap(); let socket_addr = to_socket_addr(addr, port).or_else(logging_and_return_err); let backend_socket_addr = @@ -73,13 +77,19 @@ pub extern "C" fn frontend_stop(doh: &mut DohFrontend) -> bool { /// If the caller has called `frontend_start` to start `DohFrontend`, it has to call /// call `frontend_stop` to stop the worker thread before deleting the object. /// +/// The DohFrontend is not set to null pointer, caller needs to do it on its own. +/// /// # Safety /// -/// The DohFrontend is not set to null pointer, caller needs to do it on its own. +/// `doh` must be a pointer either null or previously returned by `frontend_new`, and not yet passed +/// to `frontend_delete`. #[no_mangle] pub unsafe extern "C" fn frontend_delete(doh: *mut DohFrontend) { if !doh.is_null() { - drop(Box::from_raw(doh)); + // SAFETY: Our caller promised that `doh` was either null or previously returned by + // `frontend_new`. We just checked that it's not null, so it must have been returned by + // `frontend_new`, which obtained it from `Box::into_raw`. + drop(unsafe { Box::from_raw(doh) }); } } @@ -96,7 +106,8 @@ pub unsafe extern "C" fn frontend_set_certificate( if certificate.is_null() { return false; } - let certificate = CStr::from_ptr(certificate).to_str().unwrap(); + // SAFETY: Our caller promises that certificate is a valid C string. + let certificate = unsafe { CStr::from_ptr(certificate) }.to_str().unwrap(); doh.set_certificate(certificate).or_else(logging_and_return_err).is_ok() } @@ -113,7 +124,8 @@ pub unsafe extern "C" fn frontend_set_private_key( if private_key.is_null() { return false; } - let private_key = CStr::from_ptr(private_key).to_str().unwrap(); + // SAFETY: Our caller promises that private_key is a valid C string. + let private_key = unsafe { CStr::from_ptr(private_key) }.to_str().unwrap(); doh.set_private_key(private_key).or_else(logging_and_return_err).is_ok() } |