Skip to content

Commit

Permalink
Avoid dropping non-initialized values leading to reading uninitialize…
Browse files Browse the repository at this point in the history 
…d bytes

Still doesn't seem to fix all segfaults though.
  • Loading branch information
@oyvindln
oyvindln committed Feb 4, 2018
1 parent f907ff7 commit 33585a6
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 31 deletions.
12 changes: 8 additions & 4 deletions miniz_oxide/src/deflate/core.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ fn read_u16_le(slice: &[u8], pos: usize) -> u16{
pub struct CompressorOxide {
lz: LZOxide,
params: ParamsOxide,
huff: HuffmanOxide,
huff: Box<HuffmanOxide>,
dict: DictOxide,
}

Expand All @@ -341,7 +341,9 @@ impl CompressorOxide {
CompressorOxide {
lz: LZOxide::new(),
params: ParamsOxide::new(flags),
huff: HuffmanOxide::new(),
/// Put HuffmanOxide on the stack with default trick to avoid
/// excessive stack copies.
huff: Box::default(),
dict: DictOxide::new(flags),
}
}
Expand Down Expand Up @@ -678,15 +680,17 @@ impl RLE {
}
}

impl HuffmanOxide {
fn new() -> Self {
impl Default for HuffmanOxide {
fn default() -> Self {
HuffmanOxide {
count: [[0; MAX_HUFF_SYMBOLS]; MAX_HUFF_TABLES],
codes: [[0; MAX_HUFF_SYMBOLS]; MAX_HUFF_TABLES],
code_sizes: [[0; MAX_HUFF_SYMBOLS]; MAX_HUFF_TABLES],
}
}
}

impl HuffmanOxide {
fn radix_sort_symbols<'a>(
symbols0: &'a mut [SymFreq],
symbols1: &'a mut [SymFreq],
Expand Down
52 changes: 39 additions & 13 deletions src/lib_oxide.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,16 +137,24 @@ pub unsafe extern "C" fn def_free_func(_opaque: *mut c_void, address: *mut c_voi
}

/// Trait used for states that can be carried by BoxedState.
pub trait StateType {}
impl StateType for tdefl_compressor {}
impl StateType for inflate_state {}
pub trait StateType {
fn drop_state(&mut self);
}
impl StateType for tdefl_compressor {
fn drop_state(&mut self) {
self.drop_inner();
}
}
impl StateType for inflate_state {
fn drop_state(&mut self) {}
}

/// Wrapper for a heap-allocated compressor/decompressor that frees the stucture on drop.
pub struct BoxedState<ST: StateType> {
pub inner: *mut ST,
pub alloc: mz_alloc_func,
pub free: mz_free_func,
pub opaque: *mut c_void,
struct BoxedState<ST: StateType> {
inner: *mut ST,
alloc: mz_alloc_func,
free: mz_free_func,
opaque: *mut c_void,
}

impl<ST: StateType> Drop for BoxedState<ST> {
Expand Down Expand Up @@ -176,6 +184,10 @@ impl<ST: StateType> BoxedState<ST> {
}

fn alloc_state<T>(&mut self) -> MZResult {
if !self.inner.is_null() {
return Err(MZError::Param);
}

self.inner = unsafe { (self.alloc)(self.opaque, 1, mem::size_of::<ST>()) as *mut ST };
if self.inner.is_null() {
Err(MZError::Mem)
Expand All @@ -186,7 +198,10 @@ impl<ST: StateType> BoxedState<ST> {

pub fn free_state(&mut self) {
if !self.inner.is_null() {
unsafe { (self.free)(self.opaque, self.inner as *mut c_void) }
unsafe {
self.inner.as_mut().map(|i| i.drop_state());
(self.free)(self.opaque, self.inner as *mut c_void)
}
self.inner = ptr::null_mut();
}
}
Expand All @@ -199,7 +214,7 @@ pub struct StreamOxide<'io, ST: StateType> {
pub next_out: Option<&'io mut [u8]>,
pub total_out: c_ulong,

pub state: BoxedState<ST>,
state: BoxedState<ST>,

pub adler: c_ulong,
}
Expand Down Expand Up @@ -329,6 +344,7 @@ pub fn mz_deflate_init2_oxide(
return Err(MZError::Param);
}


stream_oxide.adler = MZ_ADLER32_INIT;
stream_oxide.total_in = 0;
stream_oxide.total_out = 0;
Expand All @@ -340,7 +356,11 @@ pub fn mz_deflate_init2_oxide(
}

match stream_oxide.state.as_mut() {
Some(state) => *state = tdefl_compressor::new(comp_flags),
Some(state) => {
unsafe {
ptr::write(state, tdefl_compressor::new(comp_flags));
}
},
None => unreachable!(),
}

Expand Down Expand Up @@ -372,11 +392,13 @@ pub fn mz_deflate_oxide(
let original_total_in = stream_oxide.total_in;
let original_total_out = stream_oxide.total_out;

if let Some(compressor) = state.inner.as_mut() {

loop {
let in_bytes;
let out_bytes;
let defl_status = {
let res = compress(&mut state.inner, *next_in, *next_out, TDEFLFlush::from(flush));
let res = compress(compressor, *next_in, *next_out, TDEFLFlush::from(flush));
in_bytes = res.1;
out_bytes = res.2;
res.0
Expand All @@ -386,7 +408,7 @@ pub fn mz_deflate_oxide(
*next_out = &mut mem::replace(next_out, &mut [])[out_bytes..];
stream_oxide.total_in += in_bytes as c_ulong;
stream_oxide.total_out += out_bytes as c_ulong;
stream_oxide.adler = state.adler32() as c_ulong;
stream_oxide.adler = compressor.adler32() as c_ulong;

if defl_status == TDEFLStatus::BadParam || defl_status == TDEFLStatus::PutBufFailed {
return Err(MZError::Stream);
Expand All @@ -411,6 +433,9 @@ pub fn mz_deflate_oxide(
};
}
}
} else {
Err(MZError::Param)
}
}

/// Free the inner compression state.
Expand All @@ -430,6 +455,7 @@ pub fn mz_deflate_reset_oxide(stream_oxide: &mut StreamOxide<tdefl_compressor>)
let state = stream_oxide.state.as_mut().ok_or(MZError::Stream)?;
stream_oxide.total_in = 0;
stream_oxide.total_out = 0;
state.drop_inner();
*state = tdefl_compressor::new(state.flags() as u32);
Ok(MZStatus::Ok)
}
Expand Down
66 changes: 52 additions & 14 deletions src/tdef.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,35 +32,42 @@ pub mod strategy {
#[repr(C)]
#[allow(bad_style)]
pub struct tdefl_compressor {
pub inner: CompressorOxide,
pub inner: Option<CompressorOxide>,
pub callback: Option<CallbackFunc>,
}

impl tdefl_compressor {
pub(crate) fn new(flags: u32) -> Self {
tdefl_compressor {
inner: CompressorOxide::new(flags),
inner: Some(CompressorOxide::new(flags)),
callback: None,
}
}

pub(crate) fn new_with_callback(flags: u32, func: CallbackFunc) -> Self {
tdefl_compressor {
inner: CompressorOxide::new(flags),
inner: Some(CompressorOxide::new(flags)),
callback: Some(func),
}
}

/// Sets the inner state to `None` and thus drops it.
pub fn drop_inner(&mut self) {
self.inner = None;
}

pub fn adler32(&self) -> u32 {
self.inner.adler32()
self.inner.as_ref().map(|i| i.adler32()).unwrap_or(0)
}

pub fn prev_return_status(&self) -> TDEFLStatus {
self.inner.prev_return_status()
// Not sure we should return on inner not existing, but that shouldn't happen
// anyway.
self.inner.as_ref().map(|i| i.prev_return_status()).unwrap_or(TDEFLStatus::BadParam)
}

pub fn flags(&self) -> i32 {
self.inner.flags()
self.inner.as_ref().map(|i| i.flags()).unwrap_or(0)
}
}

Expand All @@ -80,7 +87,8 @@ pub unsafe extern "C" fn tdefl_compress(
out_size.map(|size| *size = 0);
TDEFLStatus::BadParam
},
Some(compressor) => {
Some(compressor_wrap) => {
if let Some(ref mut compressor) = compressor_wrap.inner {
let in_buf_size = in_size.as_ref().map_or(0, |size| **size);
let out_buf_size = out_size.as_ref().map_or(0, |size| **size);

Expand All @@ -94,11 +102,11 @@ pub unsafe extern "C" fn tdefl_compress(
slice::from_raw_parts(in_buf, in_buf_size)
});

let res = match compressor.callback {
let res = match compressor_wrap.callback {
None => {
match (out_buf as *mut u8).as_mut() {
Some(out_buf) => compress(
&mut compressor.inner,
compressor,
in_slice,
slice::from_raw_parts_mut(out_buf, out_buf_size),
flush,
Expand All @@ -116,14 +124,17 @@ pub unsafe extern "C" fn tdefl_compress(
out_size.map(|size| *size = 0);
return TDEFLStatus::BadParam;
}
let res = compress_to_output(&mut compressor.inner, in_slice, func, flush);
let res = compress_to_output(compressor, in_slice, func, flush);
(res.0, res.1, 0)
},
};

in_size.map(|size| *size = res.1);
out_size.map(|size| *size = res.2);
res.0
} else {
TDEFLStatus::BadParam
}
}
}
}
Expand Down Expand Up @@ -159,11 +170,11 @@ pub unsafe extern "C" fn tdefl_init(
pub unsafe extern "C" fn tdefl_get_prev_return_status(
d: Option<&mut tdefl_compressor>,
) -> TDEFLStatus {
d.map_or(TDEFLStatus::Okay, |d| d.inner.prev_return_status())
d.map_or(TDEFLStatus::Okay, |d| d.prev_return_status())
}

pub unsafe extern "C" fn tdefl_get_adler32(d: Option<&mut tdefl_compressor>) -> c_uint {
d.map_or(::MZ_ADLER32_INIT as u32, |d| d.inner.adler32())
d.map_or(::MZ_ADLER32_INIT as u32, |d| d.adler32())
}

pub unsafe extern "C" fn tdefl_compress_mem_to_output(
Expand All @@ -180,13 +191,14 @@ pub unsafe extern "C" fn tdefl_compress_mem_to_output(
mem::size_of::<tdefl_compressor>(),
) as *mut tdefl_compressor;

*compressor = tdefl_compressor::new_with_callback(flags as u32, CallbackFunc {
ptr::write(compressor,tdefl_compressor::new_with_callback(flags as u32, CallbackFunc {
put_buf_func: put_buf_func,
put_buf_user: put_buf_user,
});
}));

let res = tdefl_compress_buffer(compressor.as_mut(), buf, buf_len, TDEFLFlush::Finish) ==
TDEFLStatus::Done;
compressor.as_mut().map(|c| c.drop_inner());
::miniz_def_free_func(ptr::null_mut(), compressor as *mut c_void);
res
} else {
Expand Down Expand Up @@ -321,3 +333,29 @@ pub extern "C" fn tdefl_create_comp_flags_from_zip_params(
create_comp_flags_from_zip_params(level, window_bits, strategy)
}
);

#[cfg(test)]
mod test {
use super::*;
use miniz_oxide::inflate::decompress_to_vec;

#[test]
fn mem_to_heap() {
let data = b"blargharghawrf31086t13qa9pt7gnseatgawe78vtb6p71v";
let mut out_len = 0;
let data_len = data.len();
let out_data = unsafe {
let res = tdefl_compress_mem_to_heap(
data.as_ptr() as *const c_void, data_len, &mut out_len, 0);
assert!(!res.is_null());
res
};
{
let out_slice = unsafe {
slice::from_raw_parts(out_data as *const u8, out_len)
};
let dec = decompress_to_vec(out_slice).unwrap();
assert!(dec.as_slice() == &data[..]);
}
}
}

2 comments on commit 33585a6

@oyvindln
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking a bit more, there is a also a similar issue in tdefl_init, going to see if I can solve that one next.

@oyvindln
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the one in tdefl_init is a bit more tricky as there is no corresponding tdefl_deinit (C miniz doesn't need it, as it doesn't allocate memory internally unlike zlib.)

Please sign in to comment.