From 1d052a2d6abb5e3132372664a5ea69dfc070569e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kornel=20Lesin=CC=81ski?= Date: Wed, 18 Jul 2018 17:33:09 +0100 Subject: [PATCH] Avoid leaking C FFI strings from invalid text chunks Fixes #28 --- src/ffi.rs | 8 +++--- src/lib.rs | 10 +++---- src/rustimpl.rs | 69 ++++++++++++++++++------------------------------- 3 files changed, 34 insertions(+), 53 deletions(-) diff --git a/src/ffi.rs b/src/ffi.rs index db3c188..2c680f4 100644 --- a/src/ffi.rs +++ b/src/ffi.rs @@ -486,9 +486,9 @@ pub unsafe extern "C" fn lodepng_clear_text(info: &mut Info) { #[no_mangle] pub unsafe extern "C" fn lodepng_add_text(info: &mut Info, key: *const c_char, str: *const c_char) -> Error { - let k = CStr::from_ptr(key); - let s = CStr::from_ptr(str); - lode_error!(rustimpl::lodepng_add_text(info, k, s)) + let k = lode_try!(CStr::from_ptr(key).to_str().map_err(|_| Error(89))); + let s = lode_try!(CStr::from_ptr(str).to_str().map_err(|_| Error(89))); + lode_error!(info.add_text(k, s)) } #[no_mangle] @@ -502,7 +502,7 @@ pub unsafe extern "C" fn lodepng_add_itext(info: &mut Info, key: *const c_char, let l = CStr::from_ptr(langtag); let t = CStr::from_ptr(transkey); let s = CStr::from_ptr(str); - lode_error!(rustimpl::lodepng_add_itext(info, k, l, t, s)) + lode_error!(info.push_itext(k.to_bytes(), l.to_bytes(), t.to_bytes(), s.to_bytes())) } #[no_mangle] diff --git a/src/lib.rs b/src/lib.rs index f0bca51..d346e74 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -319,7 +319,7 @@ impl Info { /// push back both texts at once pub fn add_text(&mut self, key: &str, str: &str) -> Result<(), Error> { - self.push_text(string_copy_slice(key.as_bytes()), string_copy_slice(str.as_bytes())) + self.push_text(key.as_bytes(), str.as_bytes()) } /// use this to clear the itexts again after you filled them in @@ -346,10 +346,10 @@ impl Info { /// push back the 4 texts of 1 chunk at once pub fn add_itext(&mut self, key: &str, langtag: &str, transkey: &str, text: &str) -> Result<(), Error> { self.push_itext( - string_copy_slice(key.as_bytes()), - string_copy_slice(langtag.as_bytes()), - string_copy_slice(transkey.as_bytes()), - string_copy_slice(text.as_bytes()), + key.as_bytes(), + langtag.as_bytes(), + transkey.as_bytes(), + text.as_bytes(), ) } diff --git a/src/rustimpl.rs b/src/rustimpl.rs index 60d6030..a712832 100644 --- a/src/rustimpl.rs +++ b/src/rustimpl.rs @@ -18,6 +18,7 @@ use super::*; use ffi::ColorProfile; use ffi::State; use huffman::HuffmanTree; +use ChunkPosition; pub use ucvec::*; pub use rgb::RGBA8 as RGBA; @@ -482,11 +483,7 @@ pub(crate) unsafe fn string_cleanup(out: &mut *mut c_char) { *out = ptr::null_mut(); } -pub(crate) fn string_copy(inp: &CStr) -> *mut c_char { - string_copy_slice(inp.to_bytes()) -} - -pub(crate) fn string_copy_slice(inp: &[u8]) -> *mut c_char { +pub(crate) fn string_copy(inp: &[u8]) -> *mut c_char { let insize = inp.len(); unsafe { let out = lodepng_malloc(insize + 1) as *mut c_char; @@ -529,43 +526,34 @@ pub(crate) fn text_copy(dest: &mut Info, source: &Info) -> Result<(), Error> { dest.text_strings = ptr::null_mut(); dest.text_num = 0; for (k, v) in source.text_keys_cstr() { - dest.push_text(string_copy(k), string_copy(v))?; + dest.push_text(k.to_bytes(), v.to_bytes())?; } Ok(()) } -unsafe fn realloc_push(array: &mut *mut T, len: usize, new: T) -> Result<(), Error> { - *array = lodepng_realloc((*array) as *mut _, (len + 1) * mem::size_of::()) as *mut _; - *array.offset(len as isize) = new; +unsafe fn realloc_push_string(array: &mut *mut *mut i8, len: usize, new: &[u8]) -> Result<(), Error> { + *array = lodepng_realloc((*array) as *mut _, (len + 1) * mem::size_of::<*mut i8>()) as *mut _; + *array.offset(len as isize) = string_copy(new); Ok(()) } - -use ChunkPosition; - impl Info { - pub(crate) fn push_itext(&mut self, key: *mut c_char, langtag: *mut c_char, transkey: *mut c_char, str: *mut c_char) -> Result<(), Error> { - assert!(!key.is_null()); - assert!(!langtag.is_null()); - assert!(!transkey.is_null()); - assert!(!str.is_null()); + pub(crate) fn push_itext(&mut self, key: &[u8], langtag: &[u8], transkey: &[u8], str: &[u8]) -> Result<(), Error> { unsafe { - realloc_push(&mut self.itext_keys, self.itext_num, key)?; - realloc_push(&mut self.itext_langtags, self.itext_num, langtag)?; - realloc_push(&mut self.itext_transkeys, self.itext_num, transkey)?; - realloc_push(&mut self.itext_strings, self.itext_num, str)?; + realloc_push_string(&mut self.itext_keys, self.itext_num, key)?; + realloc_push_string(&mut self.itext_langtags, self.itext_num, langtag)?; + realloc_push_string(&mut self.itext_transkeys, self.itext_num, transkey)?; + realloc_push_string(&mut self.itext_strings, self.itext_num, str)?; self.itext_num += 1; } Ok(()) } - pub(crate) fn push_text(&mut self, k: *mut c_char, v: *mut c_char) -> Result<(), Error> { - assert!(!k.is_null()); - assert!(!v.is_null()); + pub(crate) fn push_text(&mut self, k: &[u8], v: &[u8]) -> Result<(), Error> { unsafe { - realloc_push(&mut self.text_keys, self.text_num, k)?; - realloc_push(&mut self.text_strings, self.text_num, v)?; + realloc_push_string(&mut self.text_keys, self.text_num, k)?; + realloc_push_string(&mut self.text_strings, self.text_num, v)?; self.text_num += 1; } Ok(()) @@ -595,10 +583,6 @@ impl Info { } } -pub fn lodepng_add_text(info: &mut Info, key: &CStr, str: &CStr) -> Result<(), Error> { - info.push_text(string_copy(key), string_copy(str)) -} - pub(crate) fn itext_copy(dest: &mut Info, source: &Info) -> Result<(), Error> { dest.itext_keys = ptr::null_mut(); dest.itext_langtags = ptr::null_mut(); @@ -606,18 +590,14 @@ pub(crate) fn itext_copy(dest: &mut Info, source: &Info) -> Result<(), Error> { dest.itext_strings = ptr::null_mut(); dest.itext_num = 0; for (k,l,t,s) in source.itext_keys() { - dest.push_itext(string_copy_slice(k.as_bytes()), - string_copy_slice(l.as_bytes()), - string_copy_slice(t.as_bytes()), - string_copy_slice(s.as_bytes()))?; + dest.push_itext(k.as_bytes(), + l.as_bytes(), + t.as_bytes(), + s.as_bytes())?; } Ok(()) } -pub fn lodepng_add_itext(info: &mut Info, key: &CStr, langtag: &CStr, transkey: &CStr, str: &CStr) -> Result<(), Error> { - info.push_itext(string_copy(key), string_copy(langtag), string_copy(transkey), string_copy(str)) -} - fn add_color_bits(out: &mut [u8], index: usize, bits: u32, mut inp: u32) { let m = match bits { 1 => 7, @@ -1206,7 +1186,7 @@ fn read_chunk_text(info: &mut Info, data: &[u8]) -> Result<(), Error> { } /*even though it's not allowed by the standard, no error is thrown if there's no null termination char, if the text is empty*/ - info.push_text(string_copy_slice(keyword), string_copy_slice(str)) + info.push_text(keyword, str) } /*compressed text chunk (zTXt)*/ @@ -1232,7 +1212,7 @@ fn read_chunk_ztxt(info: &mut Info, zlibsettings: &DecompressSettings, data: &[u } let inl = &data[string2_begin..]; let decoded = zlib_decompress(inl, zlibsettings)?; - info.push_text(string_copy_slice(key), string_copy_slice(decoded.slice()))?; + info.push_text(key, decoded.slice())?; Ok(()) } @@ -1264,13 +1244,14 @@ fn read_chunk_itxt(info: &mut Info, zlibsettings: &DecompressSettings, data: &[u let (langtag, data) = split_at_nul(&data[2..]); let (transkey, data) = split_at_nul(data); + let decoded; let rest = if compressed_flag != 0 { - let decoded = zlib_decompress(data, zlibsettings)?; - string_copy_slice(decoded.slice()) + decoded = zlib_decompress(data, zlibsettings)?; + decoded.slice() } else { - string_copy_slice(data) + data }; - info.push_itext(string_copy_slice(key), string_copy_slice(langtag), string_copy_slice(transkey), rest)?; + info.push_itext(key, langtag, transkey, rest)?; Ok(()) }