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

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.
diff --git a/doh/ffi.rs b/doh/ffi.rs
index 2276f654..63b98cc8 100644
--- a/doh/ffi.rs
+++ b/doh/ffi.rs
@@ -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()
 }