Merge pull request #203 from mozilla/mp4parse_new-refactor

Refactor mp4parse_new to return Mp4parseStatus

Author: baumanj

mp4parse_capi/examples/dump.rs

@@ -28,8 +28,8 @@ fn dump_file(filename: &str) {
     };
 
     unsafe {
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let rv = mp4parse_new(&io, &mut parser);
 
         match rv {
             Mp4parseStatus::Ok => (),

mp4parse_capi/examples/test.cc

@@ -35,24 +35,23 @@ intptr_t io_read(uint8_t *buffer, uintptr_t size, void *userdata)
 
 void test_arg_validation()
 {
-  Mp4parseParser *parser = mp4parse_new(nullptr, nullptr);
-  assert(parser == nullptr);
+  Mp4parseParser *parser = nullptr;
+  Mp4parseStatus rv = mp4parse_new(nullptr, nullptr);
+  assert(rv == MP4PARSE_STATUS_BAD_ARG);
 
-  Mp4parseStatus rv = MP4PARSE_STATUS_INVALID;
-  parser = mp4parse_new(nullptr, &rv);
+  rv = mp4parse_new(nullptr, &parser);
   assert(rv == MP4PARSE_STATUS_BAD_ARG);
   assert(parser == nullptr);
 
   Mp4parseIo io = { nullptr, nullptr };
-  rv = MP4PARSE_STATUS_INVALID;
-  parser = mp4parse_new(&io, &rv);
+  rv = mp4parse_new(&io, &parser);
   assert(rv == MP4PARSE_STATUS_BAD_ARG);
   assert(parser == nullptr);
 
   int dummy_value = 42;
   io = { nullptr, &dummy_value };
-  rv = MP4PARSE_STATUS_INVALID;
-  parser = mp4parse_new(&io, &rv);
+  parser = nullptr;
+  rv = mp4parse_new(&io, &parser);
   assert(rv == MP4PARSE_STATUS_BAD_ARG);
   assert(parser == nullptr);
 
@@ -75,8 +74,8 @@ void test_arg_validation_with_parser()
 {
   int dummy_value = 42;
   Mp4parseIo io = { error_read, &dummy_value };
-  Mp4parseStatus rv = MP4PARSE_STATUS_INVALID;
-  Mp4parseParser *parser = mp4parse_new(&io, &rv);
+  Mp4parseParser *parser = nullptr;
+  Mp4parseStatus rv = mp4parse_new(&io, &parser);
   assert(rv == MP4PARSE_STATUS_IO);
   assert(parser == nullptr);
 
@@ -97,8 +96,8 @@ void test_arg_validation_with_data(const std::string& filename)
   FILE* f = fopen(filename.c_str(), "rb");
   assert(f != nullptr);
   Mp4parseIo io = { io_read, f };
-  Mp4parseStatus rv = MP4PARSE_STATUS_INVALID;
-  Mp4parseParser *parser = mp4parse_new(&io, &rv);
+  Mp4parseParser *parser = nullptr;
+  Mp4parseStatus rv = mp4parse_new(&io, &parser);
   assert(rv == MP4PARSE_STATUS_OK);
   assert(parser != nullptr);
 
@@ -183,8 +182,8 @@ int32_t read_file(const char* filename)
 
   fprintf(stderr, "Parsing file '%s'.\n", filename);
   Mp4parseIo io = { io_read, f };
-  Mp4parseStatus rv = MP4PARSE_STATUS_INVALID;
-  Mp4parseParser *parser = mp4parse_new(&io, &rv);
+  Mp4parseParser *parser = nullptr;
+  Mp4parseStatus rv = mp4parse_new(&io, &parser);
 
   if (rv != MP4PARSE_STATUS_OK) {
     assert(parser == nullptr);

mp4parse_capi/src/lib.rs

@@ -22,9 +22,9 @@
 //!     read: Some(buf_read),
 //!     userdata: &mut file as *mut _ as *mut std::os::raw::c_void
 //! };
-//! let mut rv = mp4parse_capi::Mp4parseStatus::Invalid;
+//! let mut parser = std::ptr::null_mut();
 //! unsafe {
-//!     let parser = mp4parse_capi::mp4parse_new(&io, &mut rv);
+//!     let rv = mp4parse_capi::mp4parse_new(&io, &mut parser);
 //!     assert_eq!(rv, mp4parse_capi::Mp4parseStatus::Ok);
 //!     assert!(!parser.is_null());
 //!     mp4parse_capi::mp4parse_free(parser);
@@ -417,76 +417,66 @@ impl Read for Mp4parseIo {
 ///
 /// # Safety
 ///
-/// This function is unsafe because it dereferences the io pointer given to it.
-/// The caller should ensure that the `Mp4ParseIo` struct passed in is a valid
-/// pointer. The caller should also ensure the members of io are valid: the
-/// `read` function should be sanely implemented, and the `userdata` pointer
-/// should be valid. The `status_out` is also dereferenced and must point to
-/// a valid Mp4parseStatus.
-///
-/// On a successful the variable pointed to by `status_out` will be set to
-/// `Mp4parseStatus::Ok` and the return value will be a non-NULL pointer to
-/// an `Mp4parseParser`. If there is any sort of error, the return value will
-/// be NULL and the type of error will be set in the variable pointed to by
-/// `status_out`, ensuring the consumer never has access to potentially invalid
-/// parsed data.
+/// This function is unsafe because it dereferences the `io` and `parser_out`
+/// pointers given to it. The caller should ensure that the `Mp4ParseIo`
+/// struct passed in is a valid pointer. The caller should also ensure the
+/// members of io are valid: the `read` function should be sanely implemented,
+/// and the `userdata` pointer should be valid. The `parser_out` should be a
+/// valid pointer to a location containing a null pointer. Upon successful
+/// return (`Mp4parseStatus::Ok`), that location will contain the address of
+/// an `Mp4parseParser` allocated by this function.
 ///
 /// To avoid leaking memory, any successful return of this function must be
 /// paired with a call to `mp4parse_free`. In the event of error, no memory
 /// will be allocated and `mp4parse_free` must *not* be called.
 #[no_mangle]
 pub unsafe extern "C" fn mp4parse_new(
     io: *const Mp4parseIo,
-    status_out: *mut Mp4parseStatus,
-) -> *mut Mp4parseParser {
-    mp4parse_new_common(io, status_out)
+    parser_out: *mut *mut Mp4parseParser,
+) -> Mp4parseStatus {
+    mp4parse_new_common(io, parser_out)
 }
 
 /// Allocate an `Mp4parseAvifParser*` to read from the supplied `Mp4parseIo`.
 ///
-/// See mp4parse_new; this function is identical except that it returns a
-/// pointer to an `Mp4parseAvifParser`, which (when non-null) must be paired
-/// with a call to mp4parse_avif_free.
+/// See mp4parse_new; this function is identical except that it allocates an
+/// `Mp4parseAvifParser`, which (when successful) must be paired with a call
+/// to mp4parse_avif_free.
 ///
 /// # Safety
 ///
 /// Same as mp4parse_new.
 #[no_mangle]
 pub unsafe extern "C" fn mp4parse_avif_new(
     io: *const Mp4parseIo,
-    status_out: *mut Mp4parseStatus,
-) -> *mut Mp4parseAvifParser {
-    mp4parse_new_common(io, status_out)
+    parser_out: *mut *mut Mp4parseAvifParser,
+) -> Mp4parseStatus {
+    mp4parse_new_common(io, parser_out)
 }
 
 unsafe fn mp4parse_new_common<P: ContextParser>(
     io: *const Mp4parseIo,
-    status_out: *mut Mp4parseStatus,
-) -> *mut P {
+    parser_out: *mut *mut P,
+) -> Mp4parseStatus {
     // Validate arguments from C.
-    if io.is_null() || (*io).userdata.is_null() || (*io).read.is_none() || status_out.is_null() {
-        set_if_non_null(status_out, Mp4parseStatus::BadArg);
-        std::ptr::null_mut()
+    if io.is_null()
+        || (*io).userdata.is_null()
+        || (*io).read.is_none()
+        || parser_out.is_null()
+        || !(*parser_out).is_null()
+    {
+        Mp4parseStatus::BadArg
     } else {
         match mp4parse_new_common_safe(&mut (*io).clone()) {
             Ok(parser) => {
-                set_if_non_null(status_out, Mp4parseStatus::Ok);
-                parser
-            }
-            Err(status) => {
-                set_if_non_null(status_out, status);
-                std::ptr::null_mut()
+                *parser_out = parser;
+                Mp4parseStatus::Ok
             }
+            Err(status) => status,
         }
     }
 }
 
-unsafe fn set_if_non_null<T>(ptr: *mut T, val: T) {
-    if !ptr.is_null() {
-        *ptr = val;
-    }
-}
-
 fn mp4parse_new_common_safe<T: Read, P: ContextParser>(
     io: &mut T,
 ) -> Result<*mut P, Mp4parseStatus> {
@@ -1618,12 +1608,12 @@ fn get_track_count_null_parser() {
 #[test]
 fn arg_validation() {
     unsafe {
-        let parser = mp4parse_new(std::ptr::null(), std::ptr::null_mut());
-        assert!(parser.is_null());
+        let rv = mp4parse_new(std::ptr::null(), std::ptr::null_mut());
+        assert_eq!(rv, Mp4parseStatus::BadArg);
 
         // Passing a null Mp4parseIo is an error.
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(std::ptr::null(), &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let rv = mp4parse_new(std::ptr::null(), &mut parser);
         assert_eq!(rv, Mp4parseStatus::BadArg);
         assert!(parser.is_null());
 
@@ -1634,8 +1624,8 @@ fn arg_validation() {
             read: None,
             userdata: null_mut,
         };
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::BadArg);
         assert!(parser.is_null());
 
@@ -1644,8 +1634,8 @@ fn arg_validation() {
             read: None,
             userdata: &mut dummy_value as *mut _ as *mut std::os::raw::c_void,
         };
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::BadArg);
         assert!(parser.is_null());
 
@@ -1680,6 +1670,18 @@ fn arg_validation() {
     }
 }
 
+#[test]
+fn parser_input_must_be_null() {
+    let mut dummy_value = 42;
+    let io = Mp4parseIo {
+        read: Some(error_read),
+        userdata: &mut dummy_value as *mut _ as *mut std::os::raw::c_void,
+    };
+    let mut parser = 0xDEADBEEF as *mut _;
+    let rv = unsafe { mp4parse_new(&io, &mut parser) };
+    assert_eq!(rv, Mp4parseStatus::BadArg);
+}
+
 #[test]
 fn arg_validation_with_parser() {
     unsafe {
@@ -1688,8 +1690,8 @@ fn arg_validation_with_parser() {
             read: Some(error_read),
             userdata: &mut dummy_value as *mut _ as *mut std::os::raw::c_void,
         };
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::Io);
         assert!(parser.is_null());
 
@@ -1738,35 +1740,15 @@ fn arg_validation_with_parser() {
     }
 }
 
-#[test]
-fn get_track_count_poisoned_parser() {
-    unsafe {
-        let mut dummy_value = 42;
-        let io = Mp4parseIo {
-            read: Some(error_read),
-            userdata: &mut dummy_value as *mut _ as *mut std::os::raw::c_void,
-        };
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
-        assert_eq!(rv, Mp4parseStatus::Io);
-        assert!(parser.is_null());
-
-        let mut count: u32 = 0;
-        let rv = mp4parse_get_track_count(parser, &mut count);
-        assert_eq!(rv, Mp4parseStatus::BadArg);
-    }
-}
-
 #[cfg(test)]
 fn parse_minimal_mp4() -> *mut Mp4parseParser {
     let mut file = std::fs::File::open("../mp4parse/tests/minimal.mp4").unwrap();
     let io = Mp4parseIo {
         read: Some(valid_read),
         userdata: &mut file as *mut _ as *mut std::os::raw::c_void,
     };
-    let mut rv = Mp4parseStatus::Invalid;
-    let parser;
-    unsafe { parser = mp4parse_new(&io, &mut rv) }
+    let mut parser = std::ptr::null_mut();
+    let rv = unsafe { mp4parse_new(&io, &mut parser) };
     assert_eq!(Mp4parseStatus::Ok, rv);
     parser
 }

mp4parse_capi/tests/test_chunk_out_of_range.rs

@@ -20,8 +20,8 @@ fn parse_out_of_chunk_range() {
     };
 
     unsafe {
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let mut rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::Ok);
         assert!(!parser.is_null());
 

mp4parse_capi/tests/test_encryption.rs

@@ -21,8 +21,8 @@ fn parse_cenc() {
     };
 
     unsafe {
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let mut rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::Ok);
         assert!(!parser.is_null());
         let mut counts: u32 = 0;
@@ -104,8 +104,8 @@ fn parse_cbcs() {
     };
 
     unsafe {
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let mut rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::Ok);
         assert!(!parser.is_null());
         let mut counts: u32 = 0;
@@ -166,8 +166,8 @@ fn parse_unencrypted() {
     };
 
     unsafe {
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let mut rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::Ok);
         assert!(!parser.is_null());
 

mp4parse_capi/tests/test_fragment.rs

@@ -20,8 +20,8 @@ fn parse_fragment() {
     };
 
     unsafe {
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let mut rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::Ok);
         assert!(!parser.is_null());
 
@@ -73,8 +73,8 @@ fn parse_opus_fragment() {
     };
 
     unsafe {
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let mut rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::Ok);
         assert!(!parser.is_null());
 

mp4parse_capi/tests/test_rotation.rs

@@ -20,8 +20,8 @@ fn parse_rotation() {
     };
 
     unsafe {
-        let mut rv = Mp4parseStatus::Invalid;
-        let parser = mp4parse_new(&io, &mut rv);
+        let mut parser = std::ptr::null_mut();
+        let mut rv = mp4parse_new(&io, &mut parser);
         assert_eq!(rv, Mp4parseStatus::Ok);
         assert!(!parser.is_null());