diff --git a/rust/nix-expr/src/eval_state.rs b/rust/nix-expr/src/eval_state.rs index 40c3a45..d564adb 100644 --- a/rust/nix-expr/src/eval_state.rs +++ b/rust/nix-expr/src/eval_state.rs @@ -52,8 +52,9 @@ impl EvalState { store.raw_ptr(), ) }; + context.check_err()?; if eval_state.is_null() { - bail!("nix_state_create returned a null pointer"); + panic!("nix_state_create returned a null pointer without an error"); } Ok(EvalState { eval_state: NonNull::new(eval_state).unwrap(), @@ -152,7 +153,7 @@ where if unsafe { raw::GC_thread_is_registered() } != 0 { return Ok(f()); } else { - gc_register_my_thread().unwrap(); + gc_register_my_thread()?; let r = f(); unsafe { raw::GC_unregister_my_thread(); diff --git a/rust/nix-expr/src/value.rs b/rust/nix-expr/src/value.rs index 9d48412..f1211e3 100644 --- a/rust/nix-expr/src/value.rs +++ b/rust/nix-expr/src/value.rs @@ -1,6 +1,6 @@ use nix_c_raw as raw; use nix_util::context::Context; -use std::ptr::NonNull; +use std::ptr::{null_mut, NonNull}; // TODO: test: cloning a thunk does not duplicate the evaluation. @@ -35,6 +35,7 @@ impl ValueType { raw::ValueType_NIX_TYPE_PATH => ValueType::Path, raw::ValueType_NIX_TYPE_STRING => ValueType::String, raw::ValueType_NIX_TYPE_THUNK => ValueType::Thunk, + // This would happen if a new type of value is added in Nix. _ => ValueType::Unknown, } } @@ -56,17 +57,17 @@ impl Value { } impl Drop for Value { fn drop(&mut self) { - let context = Context::new(); unsafe { - raw::gc_decref(context.ptr(), self.inner.as_ptr()); + // ignoring error because the only failure mode is leaking memory + raw::gc_decref(null_mut(), self.inner.as_ptr()); } - // ignore error from context, because drop should not panic } } impl Clone for Value { fn clone(&self) -> Self { let context = Context::new(); unsafe { raw::gc_incref(context.ptr(), self.inner.as_ptr()) }; + // can't return an error here, but we don't want to ignore the error either as it means we could use-after-free context.check_err().unwrap(); Value { inner: self.inner } } diff --git a/rust/nix-store/src/store.rs b/rust/nix-store/src/store.rs index e717d24..ba1b4bc 100644 --- a/rust/nix-store/src/store.rs +++ b/rust/nix-store/src/store.rs @@ -62,7 +62,7 @@ impl Store { }; context.check_err()?; if store.is_null() { - bail!("nix_c_store_open returned a null pointer"); + panic!("nix_c_store_open returned a null pointer without an error"); } let store = Store { inner: StoreRef { diff --git a/rust/nix-util/src/context.rs b/rust/nix-util/src/context.rs index fc8ad48..772511e 100644 --- a/rust/nix-util/src/context.rs +++ b/rust/nix-util/src/context.rs @@ -11,6 +11,8 @@ impl Context { pub fn new() -> Self { let ctx = unsafe { raw::c_context_create() }; if ctx.is_null() { + // We've failed to allocate a (relatively small) Context struct. + // We're almost certainly going to crash anyways. panic!("nix_c_context_create returned a null pointer"); } let ctx = Context { @@ -24,7 +26,7 @@ impl Context { pub fn check_err(&self) -> Result<()> { let err = unsafe { raw::err_code(self.inner.as_ptr()) }; if err != raw::NIX_OK.try_into().unwrap() { - // msgp is a borrowed pointer, so we don't need to free it + // msgp is a borrowed pointer (pointing into the context), so we don't need to free it let msgp = unsafe { raw::err_msg(null_mut(), self.inner.as_ptr(), null_mut()) }; // Turn the i8 pointer into a Rust string by copying let msg: &str = unsafe { core::ffi::CStr::from_ptr(msgp).to_str()? }; diff --git a/rust/nix-util/src/string_return.rs b/rust/nix-util/src/string_return.rs index 28b8c09..9fe5a8f 100644 --- a/rust/nix-util/src/string_return.rs +++ b/rust/nix-util/src/string_return.rs @@ -1,6 +1,8 @@ /// Callback for nix_store_get_uri and other functions that return a string. /// /// This function is used by the other nix_* crates, and you should never need to call it yourself. +/// +/// Some functions in the nix library "return" strings without giving you ownership over them, by letting you pass a callback function that gets to look at that string. This callback simply turns that string pointer into an owned rust String. pub unsafe extern "C" fn callback_get_vec_u8( start: *const ::std::os::raw::c_char, n: std::os::raw::c_uint,