Skip to content

Commit

Permalink
Fix calling destructor on uninitialized dynamic library crash.
Browse files Browse the repository at this point in the history
A test case was also created for this situation to prevent the problem
occuring again.

A similar problem was also fixed for the symbol method.

There was some minor code cleanup.
  • Loading branch information
mstewartgallus committed Aug 2, 2013
1 parent 82b2455 commit 2047026
Showing 1 changed file with 70 additions and 36 deletions.
106 changes: 70 additions & 36 deletions src/libstd/unstable/dynamic_lib.rs
Expand Up @@ -31,7 +31,7 @@ impl Drop for DynamicLibrary {
dl::close(self.handle)
}
} {
Ok(()) => { },
Ok(()) => {},
Err(str) => fail!(str)
}
}
Expand All @@ -41,14 +41,20 @@ impl DynamicLibrary {
/// Lazily open a dynamic library. When passed None it gives a
/// handle to the calling process
pub fn open(filename: Option<&path::Path>) -> Result<DynamicLibrary, ~str> {
do dl::check_for_errors_in {
unsafe {
DynamicLibrary { handle:
match filename {
Some(name) => dl::open_external(name),
None => dl::open_internal()
}
unsafe {
let maybe_library = do dl::check_for_errors_in {
match filename {
Some(name) => dl::open_external(name),
None => dl::open_internal()
}
};

// The dynamic library must not be constructed if there is
// an error opening the library so the destructor does not
// run.
match maybe_library {
Err(err) => Err(err),
Ok(handle) => Ok(DynamicLibrary { handle: handle })
}
}
}
Expand All @@ -58,41 +64,69 @@ impl DynamicLibrary {
// This function should have a lifetime constraint of 'self on
// T but that feature is still unimplemented

do dl::check_for_errors_in {
let symbol_value = do symbol.as_c_str |raw_string| {
let maybe_symbol_value = do dl::check_for_errors_in {
do symbol.as_c_str |raw_string| {
dl::symbol(self.handle, raw_string)
};
}
};

cast::transmute(symbol_value)
// The value must not be constructed if there is an error so
// the destructor does not run.
match maybe_symbol_value {
Err(err) => Err(err),
Ok(symbol_value) => Ok(cast::transmute(symbol_value))
}
}
}

#[test]
#[ignore(cfg(windows))]
priv fn test_loading_cosine () {
// The math library does not need to be loaded since it is already
// statically linked in
let libm = match DynamicLibrary::open(None) {
Err (error) => fail!("Could not load self as module: %s", error),
Ok (libm) => libm
};

// Unfortunately due to issue #6194 it is not possible to call
// this as a C function
let cosine: extern fn(libc::c_double) -> libc::c_double = unsafe {
match libm.symbol("cos") {
Err (error) => fail!("Could not load function cos: %s", error),
Ok (cosine) => cosine

#[cfg(test)]
mod test {
use super::*;
use option::*;
use result::*;
use path::*;
use libc;

#[test]
fn test_loading_cosine() {
// The math library does not need to be loaded since it is already
// statically linked in
let libm = match DynamicLibrary::open(None) {
Err(error) => fail!("Could not load self as module: %s", error),
Ok(libm) => libm
};

// Unfortunately due to issue #6194 it is not possible to call
// this as a C function
let cosine: extern fn(libc::c_double) -> libc::c_double = unsafe {
match libm.symbol("cos") {
Err(error) => fail!("Could not load function cos: %s", error),
Ok(cosine) => cosine
}
};

let argument = 0.0;
let expected_result = 1.0;
let result = cosine(argument);
if result != expected_result {
fail!("cos(%?) != %? but equaled %? instead", argument,
expected_result, result)
}
}

#[test]
#[cfg(target_os = "linux")]
#[cfg(target_os = "macos")]
#[cfg(target_os = "freebsd")]
fn test_errors_do_not_crash() {
// Open /dev/null as a library to get an error, and make sure
// that only causes an error, and not a crash.
let path = GenericPath::from_str("/dev/null");
match DynamicLibrary::open(Some(&path)) {
Err(_) => {}
Ok(_) => fail!("Successfully opened the empty library.")
}
};

let argument = 0.0;
let expected_result = 1.0;
let result = cosine(argument);
if result != expected_result {
fail!("cos(%?) != %? but equaled %? instead", argument,
expected_result, result)
}
}

Expand Down

5 comments on commit 2047026

@bors
Copy link
Contributor

@bors bors commented on 2047026 Aug 3, 2013

Choose a reason for hiding this comment

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

saw approval from graydon
at mstewartgallus@2047026

@bors
Copy link
Contributor

@bors bors commented on 2047026 Aug 3, 2013

Choose a reason for hiding this comment

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

merging sstewartgallus/rust/fix_dynamic_lib = 2047026 into auto

@bors
Copy link
Contributor

@bors bors commented on 2047026 Aug 3, 2013

Choose a reason for hiding this comment

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

sstewartgallus/rust/fix_dynamic_lib = 2047026 merged ok, testing candidate = 800dbff

@bors
Copy link
Contributor

@bors bors commented on 2047026 Aug 3, 2013

Choose a reason for hiding this comment

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

fast-forwarding master to auto = 800dbff

Please sign in to comment.