Revert changes about libsafe_ownedfd
It actually was an unsafe abstraction over OwnedFd::from_raw_fd.
This change reverts following changes:
Revert "Remove the use of from_raw_fd from fd_server/aidl.rs"
This reverts commit 105153e486d7d2dfded4d0c41b22fb2bc493a5ee.
Revert "Remove yet another use of from_raw_fd"
This reverts commit abdbd756f696a1c0d0035d1a80b3d6e827c635f0.
Revert "Don't use unsafe raw_from_fd"
This reverts commit 2867dd3e838fb4deb71f335a15e0c9dc2d617240.
Revert "Remove one more use of from_raw_fd"
This reverts commit 9ef2732fb3d55ee6b085e4a02cfbb1078b041ef8.
Revert "Use take_fd_ownership instead of the unsafe from_raw_fd"
This reverts commit ba3099ec10003d6bb2c84138bbb422997142466d.
Revert "Add libsafe_ownedfd crate"
This reverts commit 1743878cd20564e42fe41e4fbaefcb964bf83e02.
Bug: 243500154
Test: N/A
Change-Id: I4e12876518a337f1f14dc1301de909b8ace28282
diff --git a/android/virtmgr/Android.bp b/android/virtmgr/Android.bp
index cbb7b38..f0b6881 100644
--- a/android/virtmgr/Android.bp
+++ b/android/virtmgr/Android.bp
@@ -54,7 +54,6 @@
"libregex",
"librpcbinder_rs",
"librustutils",
- "libsafe_ownedfd",
"libsemver",
"libselinux_bindgen",
"libserde",
diff --git a/android/virtmgr/src/aidl.rs b/android/virtmgr/src/aidl.rs
index dab78c3..87fb611 100644
--- a/android/virtmgr/src/aidl.rs
+++ b/android/virtmgr/src/aidl.rs
@@ -73,7 +73,6 @@
use nix::unistd::pipe;
use rpcbinder::RpcServer;
use rustutils::system_properties;
-use safe_ownedfd::take_fd_ownership;
use semver::VersionReq;
use serde::Deserialize;
use std::collections::HashSet;
@@ -85,7 +84,7 @@
use std::iter;
use std::num::{NonZeroU16, NonZeroU32};
use std::ops::Range;
-use std::os::unix::io::{AsRawFd, IntoRawFd};
+use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd};
use std::os::unix::raw::pid_t;
use std::path::{Path, PathBuf};
use std::sync::{Arc, Mutex, Weak, LazyLock};
@@ -1405,7 +1404,7 @@
let stream = VsockStream::connect_with_cid_port(self.instance.cid, port)
.context("Failed to connect")
.or_service_specific_exception(-1)?;
- vsock_stream_to_pfd(stream)
+ Ok(vsock_stream_to_pfd(stream))
}
fn setHostConsoleName(&self, ptsname: &str) -> binder::Result<()> {
@@ -1564,12 +1563,10 @@
}
/// Converts a `VsockStream` to a `ParcelFileDescriptor`.
-fn vsock_stream_to_pfd(stream: VsockStream) -> binder::Result<ParcelFileDescriptor> {
- let owned_fd = take_fd_ownership(stream.into_raw_fd())
- .context("Failed to take ownership of the vsock stream")
- .with_log()
- .or_service_specific_exception(-1)?;
- Ok(ParcelFileDescriptor::new(owned_fd))
+fn vsock_stream_to_pfd(stream: VsockStream) -> ParcelFileDescriptor {
+ // SAFETY: ownership is transferred from stream to f
+ let f = unsafe { File::from_raw_fd(stream.into_raw_fd()) };
+ ParcelFileDescriptor::new(f)
}
/// Parses the platform version requirement string.
diff --git a/android/virtmgr/src/main.rs b/android/virtmgr/src/main.rs
index e4a96ac..67e7282 100644
--- a/android/virtmgr/src/main.rs
+++ b/android/virtmgr/src/main.rs
@@ -25,16 +25,16 @@
use crate::aidl::{GLOBAL_SERVICE, VirtualizationService};
use android_system_virtualizationservice::aidl::android::system::virtualizationservice::IVirtualizationService::BnVirtualizationService;
-use anyhow::{bail, Result};
+use anyhow::{bail, Context, Result};
use binder::{BinderFeatures, ProcessState};
use log::{info, LevelFilter};
use rpcbinder::{FileDescriptorTransportMode, RpcServer};
-use std::os::unix::io::{AsFd, RawFd};
+use std::os::unix::io::{AsFd, FromRawFd, OwnedFd, RawFd};
use std::sync::LazyLock;
use clap::Parser;
+use nix::fcntl::{fcntl, F_GETFD, F_SETFD, FdFlag};
use nix::unistd::{write, Pid, Uid};
use std::os::unix::raw::{pid_t, uid_t};
-use safe_ownedfd::take_fd_ownership;
const LOG_TAG: &str = "virtmgr";
@@ -71,6 +71,32 @@
ready_fd: RawFd,
}
+fn take_fd_ownership(raw_fd: RawFd, owned_fds: &mut Vec<RawFd>) -> Result<OwnedFd, anyhow::Error> {
+ // Basic check that the integer value does correspond to a file descriptor.
+ fcntl(raw_fd, F_GETFD).with_context(|| format!("Invalid file descriptor {raw_fd}"))?;
+
+ // The file descriptor had CLOEXEC disabled to be inherited from the parent.
+ // Re-enable it to make sure it is not accidentally inherited further.
+ fcntl(raw_fd, F_SETFD(FdFlag::FD_CLOEXEC))
+ .with_context(|| format!("Could not set CLOEXEC on file descriptor {raw_fd}"))?;
+
+ // Creating OwnedFd for stdio FDs is not safe.
+ if [libc::STDIN_FILENO, libc::STDOUT_FILENO, libc::STDERR_FILENO].contains(&raw_fd) {
+ bail!("File descriptor {raw_fd} is standard I/O descriptor");
+ }
+
+ // Reject RawFds that already have a corresponding OwnedFd.
+ if owned_fds.contains(&raw_fd) {
+ bail!("File descriptor {raw_fd} already owned");
+ }
+ owned_fds.push(raw_fd);
+
+ // SAFETY: Initializing OwnedFd for a RawFd provided in cmdline arguments.
+ // We checked that the integer value corresponds to a valid FD and that this
+ // is the first argument to claim its ownership.
+ Ok(unsafe { OwnedFd::from_raw_fd(raw_fd) })
+}
+
fn check_vm_support() -> Result<()> {
if hypervisor_props::is_any_vm_supported()? {
Ok(())
@@ -94,9 +120,11 @@
let args = Args::parse();
- let rpc_server_fd =
- take_fd_ownership(args.rpc_server_fd).expect("Failed to take ownership of rpc_server_fd");
- let ready_fd = take_fd_ownership(args.ready_fd).expect("Failed to take ownership of ready_fd");
+ let mut owned_fds = vec![];
+ let rpc_server_fd = take_fd_ownership(args.rpc_server_fd, &mut owned_fds)
+ .expect("Failed to take ownership of rpc_server_fd");
+ let ready_fd = take_fd_ownership(args.ready_fd, &mut owned_fds)
+ .expect("Failed to take ownership of ready_fd");
// Start thread pool for kernel Binder connection to VirtualizationServiceInternal.
ProcessState::start_thread_pool();