From 596bedbd2bf901bc46ee174f1baef504ba84ae57 Mon Sep 17 00:00:00 2001 From: Matthew Gregan Date: Thu, 30 Mar 2017 18:10:22 +1300 Subject: [PATCH] Use Option to represent potentially-null function pointer. This addresses https://bugzilla.mozilla.org/show_bug.cgi?id=1351497, which was filed as a rustc bug in https://github.com/rust-lang/rust/issues/40913, but revealed to be undefined behaviour that happened to work with earlier compiler versions. As the comment removed in this patch describes, the only reason this code existed was to work around a limitation of rusty-cheddar. --- mp4parse_capi/Cargo.toml | 2 +- mp4parse_capi/examples/afl-capi.rs | 2 +- mp4parse_capi/src/lib.rs | 27 ++++++++++-------------- mp4parse_capi/tests/test_fragment.rs | 2 +- mp4parse_capi/tests/test_rotation.rs | 2 +- mp4parse_capi/tests/test_sample_table.rs | 6 +++--- 6 files changed, 18 insertions(+), 23 deletions(-) diff --git a/mp4parse_capi/Cargo.toml b/mp4parse_capi/Cargo.toml index a30e045b..599708e2 100644 --- a/mp4parse_capi/Cargo.toml +++ b/mp4parse_capi/Cargo.toml @@ -29,7 +29,7 @@ mp4parse = {version = "0.7.1", path = "../mp4parse"} num-traits = "0.1.37" [build-dependencies] -rusty-cheddar = "0.3.2" +rusty-cheddar = { git = "https://github.com/kinetiknz/rusty-cheddar" } [features] fuzz = ["mp4parse/fuzz"] diff --git a/mp4parse_capi/examples/afl-capi.rs b/mp4parse_capi/examples/afl-capi.rs index 7b4bfc5a..d53ec755 100644 --- a/mp4parse_capi/examples/afl-capi.rs +++ b/mp4parse_capi/examples/afl-capi.rs @@ -22,7 +22,7 @@ fn doit() { let mut input = Vec::new(); std::io::stdin().read_to_end(&mut input).unwrap(); let mut cursor = std::io::Cursor::new(input); - let io = mp4parse_io { read: vec_read, userdata: &mut cursor as *mut _ as *mut std::os::raw::c_void }; + let io = mp4parse_io { read: Some(vec_read), userdata: &mut cursor as *mut _ as *mut std::os::raw::c_void }; unsafe { let context = mp4parse_new(&io); let rv = mp4parse_read(context); diff --git a/mp4parse_capi/src/lib.rs b/mp4parse_capi/src/lib.rs index aead1499..5d33432f 100644 --- a/mp4parse_capi/src/lib.rs +++ b/mp4parse_capi/src/lib.rs @@ -19,7 +19,7 @@ //! //! let mut file = std::fs::File::open("../mp4parse/tests/minimal.mp4").unwrap(); //! let io = mp4parse_capi::mp4parse_io { -//! read: buf_read, +//! read: Some(buf_read), //! userdata: &mut file as *mut _ as *mut std::os::raw::c_void //! }; //! unsafe { @@ -261,7 +261,7 @@ impl mp4parse_parser { #[repr(C)] #[derive(Clone)] pub struct mp4parse_io { - pub read: extern fn(buffer: *mut u8, size: usize, userdata: *mut std::os::raw::c_void) -> isize, + pub read: Option isize>, pub userdata: *mut std::os::raw::c_void, } @@ -270,7 +270,7 @@ impl Read for mp4parse_io { if buf.len() > isize::max_value() as usize { return Err(std::io::Error::new(std::io::ErrorKind::Other, "buf length overflow in mp4parse_io Read impl")); } - let rv = (self.read)(buf.as_mut_ptr(), buf.len(), self.userdata); + let rv = self.read.unwrap()(buf.as_mut_ptr(), buf.len(), self.userdata); if rv >= 0 { Ok(rv as usize) } else { @@ -287,12 +287,7 @@ pub unsafe extern fn mp4parse_new(io: *const mp4parse_io) -> *mut mp4parse_parse if io.is_null() || (*io).userdata.is_null() { return std::ptr::null_mut(); } - // is_null() isn't available on a fn type because it can't be null (in - // Rust) by definition. But since this value is coming from the C API, - // it *could* be null. Ideally, we'd wrap it in an Option to represent - // reality, but this causes rusty-cheddar to emit the wrong type - // (https://github.com/Sean1708/rusty-cheddar/issues/30). - if ((*io).read as *mut std::os::raw::c_void).is_null() { + if (*io).read.is_none() { return std::ptr::null_mut(); } let parser = Box::new(mp4parse_parser(Wrap { @@ -1156,7 +1151,7 @@ extern fn valid_read(buf: *mut u8, size: usize, userdata: *mut std::os::raw::c_v fn new_parser() { let mut dummy_value: u32 = 42; let io = mp4parse_io { - read: panic_read, + read: Some(panic_read), userdata: &mut dummy_value as *mut _ as *mut std::os::raw::c_void, }; unsafe { @@ -1195,19 +1190,19 @@ fn arg_validation() { let null_mut: *mut std::os::raw::c_void = std::ptr::null_mut(); // Passing an mp4parse_io with null members is an error. - let io = mp4parse_io { read: std::mem::transmute(null_mut), + let io = mp4parse_io { read: None, userdata: null_mut }; let parser = mp4parse_new(&io); assert!(parser.is_null()); - let io = mp4parse_io { read: panic_read, + let io = mp4parse_io { read: Some(panic_read), userdata: null_mut }; let parser = mp4parse_new(&io); assert!(parser.is_null()); let mut dummy_value = 42; let io = mp4parse_io { - read: std::mem::transmute(null_mut), + read: None, userdata: &mut dummy_value as *mut _ as *mut std::os::raw::c_void, }; let parser = mp4parse_new(&io); @@ -1246,7 +1241,7 @@ fn arg_validation_with_parser() { unsafe { let mut dummy_value = 42; let io = mp4parse_io { - read: error_read, + read: Some(error_read), userdata: &mut dummy_value as *mut _ as *mut std::os::raw::c_void, }; let parser = mp4parse_new(&io); @@ -1295,7 +1290,7 @@ fn get_track_count_poisoned_parser() { unsafe { let mut dummy_value = 42; let io = mp4parse_io { - read: error_read, + read: Some(error_read), userdata: &mut dummy_value as *mut _ as *mut std::os::raw::c_void, }; let parser = mp4parse_new(&io); @@ -1314,7 +1309,7 @@ fn get_track_count_poisoned_parser() { fn arg_validation_with_data() { unsafe { let mut file = std::fs::File::open("../mp4parse/tests/minimal.mp4").unwrap(); - let io = mp4parse_io { read: valid_read, + let io = mp4parse_io { read: Some(valid_read), userdata: &mut file as *mut _ as *mut std::os::raw::c_void }; let parser = mp4parse_new(&io); assert!(!parser.is_null()); diff --git a/mp4parse_capi/tests/test_fragment.rs b/mp4parse_capi/tests/test_fragment.rs index 811c69f4..b91fb221 100644 --- a/mp4parse_capi/tests/test_fragment.rs +++ b/mp4parse_capi/tests/test_fragment.rs @@ -15,7 +15,7 @@ extern fn buf_read(buf: *mut u8, size: usize, userdata: *mut std::os::raw::c_voi fn parse_fragment() { let mut file = std::fs::File::open("tests/bipbop_audioinit.mp4").expect("Unknown file"); let io = mp4parse_io { - read: buf_read, + read: Some(buf_read), userdata: &mut file as *mut _ as *mut std::os::raw::c_void }; diff --git a/mp4parse_capi/tests/test_rotation.rs b/mp4parse_capi/tests/test_rotation.rs index b3068773..f3e83267 100644 --- a/mp4parse_capi/tests/test_rotation.rs +++ b/mp4parse_capi/tests/test_rotation.rs @@ -15,7 +15,7 @@ extern fn buf_read(buf: *mut u8, size: usize, userdata: *mut std::os::raw::c_voi fn parse_rotation() { let mut file = std::fs::File::open("tests/video_rotation_90.mp4").expect("Unknown file"); let io = mp4parse_io { - read: buf_read, + read: Some(buf_read), userdata: &mut file as *mut _ as *mut std::os::raw::c_void }; diff --git a/mp4parse_capi/tests/test_sample_table.rs b/mp4parse_capi/tests/test_sample_table.rs index baade761..f670a310 100644 --- a/mp4parse_capi/tests/test_sample_table.rs +++ b/mp4parse_capi/tests/test_sample_table.rs @@ -15,7 +15,7 @@ extern fn buf_read(buf: *mut u8, size: usize, userdata: *mut std::os::raw::c_voi fn parse_sample_table() { let mut file = std::fs::File::open("tests/bipbop_nonfragment_header.mp4").expect("Unknown file"); let io = mp4parse_io { - read: buf_read, + read: Some(buf_read), userdata: &mut file as *mut _ as *mut std::os::raw::c_void }; @@ -100,7 +100,7 @@ fn parse_sample_table() { fn parse_sample_table_with_elst() { let mut file = std::fs::File::open("tests/short-cenc.mp4").expect("Unknown file"); let io = mp4parse_io { - read: buf_read, + read: Some(buf_read), userdata: &mut file as *mut _ as *mut std::os::raw::c_void }; @@ -156,7 +156,7 @@ fn parse_sample_table_with_elst() { fn parse_sample_table_with_negative_ctts() { let mut file = std::fs::File::open("tests/white.mp4").expect("Unknown file"); let io = mp4parse_io { - read: buf_read, + read: Some(buf_read), userdata: &mut file as *mut _ as *mut std::os::raw::c_void };