Merge "Add safety comments." into main am: 367ade7e3e am: 619328d78f am: 625c6af88b (61fc635c) · Commits · e / os / android_packages_modules_DnsResolver

doh/boot_time.rs

+6 −10

Original line number	Diff line number	Diff line
		@@ -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.

doh/connection/mod.rs

+1 −1

Original line number	Diff line number	Diff line
		@@ -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.

doh/ffi.rs

+38 −16

Original line number	Diff line number	Diff line
		@@ -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 (
		// 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, "");
		}

doh/tests/doh_frontend/src/dns_https_frontend.rs

+3 −0

Original line number	Diff line number	Diff line
		@@ -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])));

doh/tests/doh_frontend/src/ffi.rs

+20 −8

Original line number	Diff line number	Diff line
		@@ -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()
		}