From 94923406ae4af8139a6ee8fcb3c265c26ae69835 Mon Sep 17 00:00:00 2001 From: Zach Reizner Date: Thu, 15 Nov 2018 14:17:34 -0800 Subject: [PATCH] qcow_utils: do not close given fds in `convert_to_*` functions The `convert_to_*` functions take ownership of the passed FDs even though they should not according to the function's contract. This change clones the passed FDs so that the caller can retain ownership of its FDs. This change also wraps most of the implementations in catch_unwind so that panics do not unwind past FFI boundaries, which is undefined behavior. BUG=chromium:905799 TEST=in crosh: `vmc export ` Change-Id: I2f65ebff51243675d0854574d8fd02cec1b237a4 Reviewed-on: https://chromium-review.googlesource.com/1338501 Commit-Ready: Zach Reizner Tested-by: Zach Reizner Reviewed-by: Daniel Verkamp --- qcow_utils/src/qcow_utils.rs | 44 ++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/qcow_utils/src/qcow_utils.rs b/qcow_utils/src/qcow_utils.rs index ea84b62854..33f20ae123 100644 --- a/qcow_utils/src/qcow_utils.rs +++ b/qcow_utils/src/qcow_utils.rs @@ -7,11 +7,13 @@ extern crate libc; extern crate qcow; -use libc::{EINVAL, EIO}; +use libc::{EBADFD, EINVAL, EIO}; use std::ffi::CStr; use std::fs::{File, OpenOptions}; +use std::mem::forget; use std::os::raw::{c_char, c_int}; use std::os::unix::io::FromRawFd; +use std::panic::catch_unwind; use qcow::{ImageType, QcowFile}; @@ -47,23 +49,47 @@ pub unsafe extern "C" fn create_qcow_with_size(path: *const c_char, virtual_size #[no_mangle] pub unsafe extern "C" fn convert_to_qcow2(src_fd: c_int, dst_fd: c_int) -> c_int { // The caller is responsible for passing valid file descriptors. + // The caller retains ownership of the file descriptors. let src_file = File::from_raw_fd(src_fd); - let dst_file = File::from_raw_fd(dst_fd); + let src_file_owned = src_file.try_clone(); + forget(src_file); - match qcow::convert(src_file, dst_file, ImageType::Qcow2) { - Ok(_) => 0, - Err(_) => -EIO, + let dst_file = File::from_raw_fd(dst_fd); + let dst_file_owned = dst_file.try_clone(); + forget(dst_file); + + match (src_file_owned, dst_file_owned) { + (Ok(src_file), Ok(dst_file)) => { + catch_unwind( + || match qcow::convert(src_file, dst_file, ImageType::Qcow2) { + Ok(_) => 0, + Err(_) => -EIO, + }, + ).unwrap_or(-EIO) + } + _ => -EBADFD, } } #[no_mangle] pub unsafe extern "C" fn convert_to_raw(src_fd: c_int, dst_fd: c_int) -> c_int { // The caller is responsible for passing valid file descriptors. + // The caller retains ownership of the file descriptors. let src_file = File::from_raw_fd(src_fd); - let dst_file = File::from_raw_fd(dst_fd); + let src_file_owned = src_file.try_clone(); + forget(src_file); - match qcow::convert(src_file, dst_file, ImageType::Raw) { - Ok(_) => 0, - Err(_) => -EIO, + let dst_file = File::from_raw_fd(dst_fd); + let dst_file_owned = dst_file.try_clone(); + forget(dst_file); + + match (src_file_owned, dst_file_owned) { + (Ok(src_file), Ok(dst_file)) => { + catch_unwind(|| match qcow::convert(src_file, dst_file, ImageType::Raw) { + Ok(_) => 0, + Err(_) => -EIO, + }).unwrap_or(-EIO) + } + _ => -EBADFD, } }