Bug 1329126 - update rust mp4 parser for preventing buffer overflow. r?kinetik draft
authorAlfredo.Yang <ayang@mozilla.com>
Fri, 31 Mar 2017 15:49:43 +0800
changeset 554227 f06b4426136926a8efe0041711419452c6c39d94
parent 552691 272ce6c2572164f5f6a9fba2a980ba9ccf50770c
child 622274 4fe7df83dd0340437ffe0b5aeff57f5c30296371
push id51867
push userbmo:ayang@mozilla.com
push dateFri, 31 Mar 2017 07:52:48 +0000
reviewerskinetik
bugs1329126
milestone55.0a1
Bug 1329126 - update rust mp4 parser for preventing buffer overflow. r?kinetik MozReview-Commit-ID: Ih85J4nejLx
media/libstagefright/binding/mp4parse-cargo.patch
media/libstagefright/binding/mp4parse/src/boxes.rs
media/libstagefright/binding/mp4parse/src/lib.rs
media/libstagefright/binding/mp4parse/src/tests.rs
media/libstagefright/binding/mp4parse_capi/src/lib.rs
media/libstagefright/binding/update-rust.sh
--- a/media/libstagefright/binding/mp4parse-cargo.patch
+++ b/media/libstagefright/binding/mp4parse-cargo.patch
@@ -40,16 +40,16 @@ index aeeebc65..5c0836a 100644
 +build = false
  
  [dependencies]
  byteorder = "1.0.0"
  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"]
 -
  # Somewhat heavy-handed, but we want at least -Z force-overflow-checks=on.
  [profile.release]
  debug-assertions = true
--- a/media/libstagefright/binding/mp4parse/src/boxes.rs
+++ b/media/libstagefright/binding/mp4parse/src/boxes.rs
@@ -44,17 +44,17 @@ macro_rules! box_database {
 pub struct FourCC {
     pub value: String
 }
 
 impl From<u32> for FourCC {
     fn from(number: u32) -> FourCC {
         let mut box_chars = Vec::new();
         for x in 0..4 {
-            let c = (number >> x * 8 & 0x000000FF) as u8;
+            let c = (number >> (x * 8) & 0x000000FF) as u8;
             box_chars.push(c);
         }
         box_chars.reverse();
 
         let box_string = match String::from_utf8(box_chars) {
             Ok(t) => t,
             _ => String::from("null"), // error to retrieve fourcc
         };
--- a/media/libstagefright/binding/mp4parse/src/lib.rs
+++ b/media/libstagefright/binding/mp4parse/src/lib.rs
@@ -21,17 +21,17 @@ use num_traits::Num;
 mod boxes;
 use boxes::{BoxType, FourCC};
 
 // Unit tests.
 #[cfg(test)]
 mod tests;
 
 // Arbitrary buffer size limit used for raw read_bufs on a box.
-const BUF_SIZE_LIMIT: u64 = 1024 * 1024;
+const BUF_SIZE_LIMIT: usize = 1024 * 1024;
 
 static DEBUG_MODE: std::sync::atomic::AtomicBool = std::sync::atomic::ATOMIC_BOOL_INIT;
 
 pub fn set_debug_mode(mode: bool) {
     DEBUG_MODE.store(mode, std::sync::atomic::Ordering::SeqCst);
 }
 
 #[inline(always)]
@@ -1505,19 +1505,16 @@ fn read_es_descriptor(data: &[u8], esds:
 
     Ok(())
 }
 
 fn read_esds<T: Read>(src: &mut BMFFBox<T>) -> Result<ES_Descriptor> {
     let (_, _) = read_fullbox_extra(src)?;
 
     let esds_size = src.head.size - src.head.offset - 4;
-    if esds_size > BUF_SIZE_LIMIT {
-        return Err(Error::InvalidData("esds box exceeds BUF_SIZE_LIMIT"));
-    }
     let esds_array = read_buf(src, esds_size as usize)?;
 
     let mut es_data = ES_Descriptor::default();
     find_descriptor(&esds_array, &mut es_data)?;
 
     es_data.codec_esds = esds_array;
 
     Ok(es_data)
@@ -1695,19 +1692,16 @@ fn read_video_sample_entry<T: Read>(src:
             BoxType::AVCConfigurationBox => {
                 if (name != BoxType::AVCSampleEntry &&
                     name != BoxType::AVC3SampleEntry &&
                     name != BoxType::ProtectedVisualSampleEntry) ||
                     codec_specific.is_some() {
                         return Err(Error::InvalidData("malformed video sample entry"));
                     }
                 let avcc_size = b.head.size - b.head.offset;
-                if avcc_size > BUF_SIZE_LIMIT {
-                    return Err(Error::InvalidData("avcC box exceeds BUF_SIZE_LIMIT"));
-                }
                 let avcc = read_buf(&mut b.content, avcc_size as usize)?;
                 log!("{:?} (avcc)", avcc);
                 // TODO(kinetik): Parse avcC box?  For now we just stash the data.
                 codec_specific = Some(VideoCodecSpecific::AVCConfig(avcc));
             }
             BoxType::VPCodecConfigurationBox => { // vpcC
                 if (name != BoxType::VP8SampleEntry &&
                     name != BoxType::VP9SampleEntry &&
@@ -1988,16 +1982,19 @@ fn skip<T: Read>(src: &mut T, mut bytes:
         }
         bytes -= len;
     }
     Ok(())
 }
 
 /// Read size bytes into a Vector or return error.
 fn read_buf<T: ReadBytesExt>(src: &mut T, size: usize) -> Result<Vec<u8>> {
+    if size > BUF_SIZE_LIMIT {
+        return Err(Error::InvalidData("read_buf size exceeds BUF_SIZE_LIMIT"));
+    }
     let mut buf = vec![0; size];
     let r = src.read(&mut buf)?;
     if r != size {
         return Err(Error::InvalidData("failed buffer read"));
     }
     Ok(buf)
 }
 
--- a/media/libstagefright/binding/mp4parse/src/tests.rs
+++ b/media/libstagefright/binding/mp4parse/src/tests.rs
@@ -636,17 +636,17 @@ fn avcc_limit() {
          .append_repeated(0, 4)
          .B32(0xffffffff)
          .append_bytes(b"avcC")
          .append_repeated(0, 100)
     });
     let mut iter = super::BoxIter::new(&mut stream);
     let mut stream = iter.next_box().unwrap().unwrap();
     match super::read_video_sample_entry(&mut stream) {
-        Err(Error::InvalidData(s)) => assert_eq!(s, "avcC box exceeds BUF_SIZE_LIMIT"),
+        Err(Error::InvalidData(s)) => assert_eq!(s, "read_buf size exceeds BUF_SIZE_LIMIT"),
         Ok(_) => panic!("expected an error result"),
         _ => panic!("expected a different error result"),
     }
 }
 
 #[test]
 fn esds_limit() {
     let mut stream = make_box(BoxSize::Auto, b"mp4a", |s| {
@@ -661,17 +661,17 @@ fn esds_limit() {
          .B32(48000 << 16)
          .B32(0xffffffff)
          .append_bytes(b"esds")
          .append_repeated(0, 100)
     });
     let mut iter = super::BoxIter::new(&mut stream);
     let mut stream = iter.next_box().unwrap().unwrap();
     match super::read_audio_sample_entry(&mut stream) {
-        Err(Error::InvalidData(s)) => assert_eq!(s, "esds box exceeds BUF_SIZE_LIMIT"),
+        Err(Error::InvalidData(s)) => assert_eq!(s, "read_buf size exceeds BUF_SIZE_LIMIT"),
         Ok(_) => panic!("expected an error result"),
         _ => panic!("expected a different error result"),
     }
 }
 
 #[test]
 fn esds_limit_2() {
     let mut stream = make_box(BoxSize::Auto, b"mp4a", |s| {
--- a/media/libstagefright/binding/mp4parse_capi/src/lib.rs
+++ b/media/libstagefright/binding/mp4parse_capi/src/lib.rs
@@ -14,17 +14,17 @@
 //!    match input.read(&mut buf) {
 //!        Ok(n) => n as isize,
 //!        Err(_) => -1,
 //!    }
 //! }
 //!
 //! 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 {
 //!     let parser = mp4parse_capi::mp4parse_new(&io);
 //!     let rv = mp4parse_capi::mp4parse_read(parser);
 //!     assert_eq!(rv, mp4parse_capi::mp4parse_error::MP4PARSE_OK);
 //!     mp4parse_capi::mp4parse_free(parser);
 //! }
@@ -256,48 +256,43 @@ impl mp4parse_parser {
     fn sample_table_mut(&mut self) -> &mut HashMap<u32, Vec<mp4parse_indice>> {
         &mut self.0.sample_table
     }
 }
 
 #[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<extern fn(buffer: *mut u8, size: usize, userdata: *mut std::os::raw::c_void) -> isize>,
     pub userdata: *mut std::os::raw::c_void,
 }
 
 impl Read for mp4parse_io {
     fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
         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 {
             Err(std::io::Error::new(std::io::ErrorKind::Other, "I/O error in mp4parse_io Read impl"))
         }
     }
 }
 
 // C API wrapper functions.
 
 /// Allocate an `mp4parse_parser*` to read from the supplied `mp4parse_io`.
 #[no_mangle]
 pub unsafe extern fn mp4parse_new(io: *const mp4parse_io) -> *mut mp4parse_parser {
     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 {
         context: MediaContext::new(),
         io: (*io).clone(),
         poisoned: false,
         opus_header: HashMap::new(),
         pssh_data: Vec::new(),
@@ -563,41 +558,35 @@ pub unsafe extern fn mp4parse_get_track_
             let mut v = Vec::new();
             match serialize_opus_header(opus, &mut v) {
                 Err(_) => {
                     return MP4PARSE_ERROR_INVALID;
                 }
                 Ok(_) => {
                     let header = (*parser).opus_header_mut();
                     header.insert(track_index, v);
-                    match header.get(&track_index) {
-                        None => {}
-                        Some(v) => {
-                            if v.len() > std::u32::MAX as usize {
-                                return MP4PARSE_ERROR_INVALID;
-                            }
-                            (*info).codec_specific_config.length = v.len() as u32;
-                            (*info).codec_specific_config.data = v.as_ptr();
+                    if let Some(v) = header.get(&track_index) {
+                        if v.len() > std::u32::MAX as usize {
+                            return MP4PARSE_ERROR_INVALID;
                         }
+                        (*info).codec_specific_config.length = v.len() as u32;
+                        (*info).codec_specific_config.data = v.as_ptr();
                     }
                 }
             }
         }
         AudioCodecSpecific::MP3 => (),
     }
 
-    match audio.protection_info.iter().find(|sinf| sinf.tenc.is_some()) {
-        Some(p) => {
-            if let Some(ref tenc) = p.tenc {
-                (*info).protected_data.is_encrypted = tenc.is_encrypted;
-                (*info).protected_data.iv_size = tenc.iv_size;
-                (*info).protected_data.kid.set_data(&(tenc.kid));
-            }
-        },
-        _ => {},
+    if let Some(p) = audio.protection_info.iter().find(|sinf| sinf.tenc.is_some()) {
+        if let Some(ref tenc) = p.tenc {
+            (*info).protected_data.is_encrypted = tenc.is_encrypted;
+            (*info).protected_data.iv_size = tenc.iv_size;
+            (*info).protected_data.kid.set_data(&(tenc.kid));
+        }
     }
 
     MP4PARSE_OK
 }
 
 /// Fill the supplied `mp4parse_track_video_info` with metadata for `track`.
 #[no_mangle]
 pub unsafe extern fn mp4parse_get_track_video_info(parser: *mut mp4parse_parser, track_index: u32, info: *mut mp4parse_track_video_info) -> mp4parse_error {
@@ -643,32 +632,26 @@ pub unsafe extern fn mp4parse_get_track_
             _ => 0,
         };
     } else {
         return MP4PARSE_ERROR_INVALID;
     }
     (*info).image_width = video.width;
     (*info).image_height = video.height;
 
-    match video.codec_specific {
-        VideoCodecSpecific::AVCConfig(ref avc) => {
-            (*info).extra_data.set_data(avc);
-        },
-        _ => {},
+    if let VideoCodecSpecific::AVCConfig(ref avc) = video.codec_specific {
+        (*info).extra_data.set_data(avc);
     }
 
-    match video.protection_info.iter().find(|sinf| sinf.tenc.is_some()) {
-        Some(p) => {
-            if let Some(ref tenc) = p.tenc {
-                (*info).protected_data.is_encrypted = tenc.is_encrypted;
-                (*info).protected_data.iv_size = tenc.iv_size;
-                (*info).protected_data.kid.set_data(&(tenc.kid));
-            }
-        },
-        _ => {},
+    if let Some(p) = video.protection_info.iter().find(|sinf| sinf.tenc.is_some()) {
+        if let Some(ref tenc) = p.tenc {
+            (*info).protected_data.is_encrypted = tenc.is_encrypted;
+            (*info).protected_data.iv_size = tenc.iv_size;
+            (*info).protected_data.kid.set_data(&(tenc.kid));
+        }
     }
 
     MP4PARSE_OK
 }
 
 #[no_mangle]
 pub unsafe extern fn mp4parse_get_indice_table(parser: *mut mp4parse_parser, track_id: u32, indices: *mut mp4parse_byte_data) -> mp4parse_error {
     if parser.is_null() || (*parser).poisoned() {
@@ -681,22 +664,19 @@ pub unsafe extern fn mp4parse_get_indice
     let context = (*parser).context();
     let tracks = &context.tracks;
     let track = match tracks.iter().find(|track| track.track_id == Some(track_id)) {
         Some(t) => t,
         _ => return MP4PARSE_ERROR_INVALID,
     };
 
     let index_table = (*parser).sample_table_mut();
-    match index_table.get(&track_id) {
-        Some(v) => {
-            (*indices).set_indices(v);
-            return MP4PARSE_OK;
-        },
-        _ => {},
+    if let Some(v) = index_table.get(&track_id) {
+        (*indices).set_indices(v);
+        return MP4PARSE_OK;
     }
 
     let media_time = match (&track.media_time, &track.timescale) {
         (&Some(t), &Some(s)) => {
             track_time_to_us(t, s).map(|v| v as i64)
         },
         _ => None,
     };
@@ -713,23 +693,20 @@ pub unsafe extern fn mp4parse_get_indice
     // before first frame is displayed.
     let offset_time = match (empty_duration, media_time) {
         (Some(e), Some(m)) => e - m,
         (Some(e), None) => e,
         (None, Some(m)) => m,
         _ => 0,
     };
 
-    match create_sample_table(track, offset_time) {
-        Some(v) => {
-            (*indices).set_indices(&v);
-            index_table.insert(track_id, v);
-            return MP4PARSE_OK;
-        },
-        _ => {},
+    if let Some(v) = create_sample_table(track, offset_time) {
+        (*indices).set_indices(&v);
+        index_table.insert(track_id, v);
+        return MP4PARSE_OK;
     }
 
     MP4PARSE_ERROR_INVALID
 }
 
 // Convert a 'ctts' compact table to full table by iterator,
 // (sample_with_the_same_offset_count, offset) => (offset), (offset), (offset) ...
 //
@@ -929,23 +906,20 @@ fn create_sample_table(track: &Track, tr
                     start_decode: 0,
                     sync: !has_sync_table,
                 }
             );
         }
     }
 
     // Mark the sync sample in sample_table according to 'stss'.
-    match track.stss {
-        Some(ref v) => {
-            for iter in &v.samples {
-                sample_table[(iter - 1) as usize].sync = true;
-            }
-        },
-        _ => {}
+    if let Some(ref v) = track.stss {
+        for iter in &v.samples {
+            sample_table[(iter - 1) as usize].sync = true;
+        }
     }
 
     let ctts_iter = match track.ctts {
         Some(ref v) => Some(v.samples.as_slice().iter()),
         _ => None,
     };
 
     let mut ctts_offset_iter = TimeOffsetIterator {
@@ -1109,22 +1083,23 @@ pub unsafe extern fn mp4parse_get_pssh_i
     *info = Default::default();
 
     let context = (*parser).context_mut();
     let pssh_data = (*parser).pssh_data_mut();
     let info: &mut mp4parse_pssh_info = &mut *info;
 
     pssh_data.clear();
     for pssh in &context.psshs {
+        let content_len = pssh.box_content.len();
+        if content_len > std::u32::MAX as usize {
+            return MP4PARSE_ERROR_INVALID;
+        }
         let mut data_len = Vec::new();
-        match data_len.write_u32::<byteorder::NativeEndian>(pssh.box_content.len() as u32) {
-            Err(_) => {
-                return MP4PARSE_ERROR_IO;
-            },
-            _ => (),
+        if data_len.write_u32::<byteorder::NativeEndian>(content_len as u32).is_err() {
+            return MP4PARSE_ERROR_IO;
         }
         pssh_data.extend_from_slice(pssh.system_id.as_slice());
         pssh_data.extend_from_slice(data_len.as_slice());
         pssh_data.extend_from_slice(pssh.box_content.as_slice());
     }
 
     info.data.set_data(pssh_data);
 
@@ -1151,17 +1126,17 @@ extern fn valid_read(buf: *mut u8, size:
         Err(_) => -1,
     }
 }
 
 #[test]
 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 {
         let parser = mp4parse_new(&io);
         assert!(!parser.is_null());
         mp4parse_free(parser);
     }
 }
@@ -1190,29 +1165,29 @@ fn arg_validation() {
     unsafe {
         // Passing a null mp4parse_io is an error.
         let parser = mp4parse_new(std::ptr::null());
         assert!(parser.is_null());
 
         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);
         assert!(parser.is_null());
 
         // Passing a null mp4parse_parser is an error.
         assert_eq!(MP4PARSE_ERROR_BADARG, mp4parse_read(std::ptr::null_mut()));
 
@@ -1241,17 +1216,17 @@ fn arg_validation() {
     }
 }
 
 #[test]
 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);
         assert!(!parser.is_null());
 
         // Our mp4parse_io read should simply fail with an error.
         assert_eq!(MP4PARSE_ERROR_IO, mp4parse_read(parser));
 
@@ -1290,17 +1265,17 @@ fn arg_validation_with_parser() {
     }
 }
 
 #[test]
 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);
         assert!(!parser.is_null());
 
         // Our mp4parse_io read should simply fail with an error.
         assert_eq!(MP4PARSE_ERROR_IO, mp4parse_read(parser));
 
@@ -1309,17 +1284,17 @@ fn get_track_count_poisoned_parser() {
         assert_eq!(rv, MP4PARSE_ERROR_BADARG);
     }
 }
 
 #[test]
 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());
 
         assert_eq!(MP4PARSE_OK, mp4parse_read(parser));
 
         let mut count: u32 = 0;
         assert_eq!(MP4PARSE_OK, mp4parse_get_track_count(parser, &mut count));
--- a/media/libstagefright/binding/update-rust.sh
+++ b/media/libstagefright/binding/update-rust.sh
@@ -1,13 +1,13 @@
 #!/bin/sh -e
 # Script to update mp4parse-rust sources to latest upstream
 
 # Default version.
-VER=b78dc3e4e80ce4132e7880ae068d0672cbfeaa48
+VER=2f595d947adc981360df4ce34bc157202ca566ae
 
 # Accept version or commit from the command line.
 if test -n "$1"; then
   VER=$1
 fi
 
 echo "Fetching sources..."
 rm -rf _upstream