From 49750fa1e7e307a323e22f4f0d98ad2dc88d78bf Mon Sep 17 00:00:00 2001 From: edef Date: Sat, 25 May 2024 04:24:59 +0000 Subject: [PATCH] fix(tvix/eval): disallow empty but allocated string contexts Both `Some(NixContext::new())` and `None` represent empty contexts, but the former trips up `NixString::has_context`, and seems likely to trip up other things. We could hide the difference in the accessors, but we don't really *want* the distinction to exist, since heap-allocating a null value is pretty much always a mistake. Change-Id: Ie84d26fb0d4b59e68354891ba13bde3bae40ab6e Reviewed-on: https://cl.tvl.fyi/c/depot/+/11712 Tested-by: BuildkiteCI Reviewed-by: flokli --- tvix/eval/src/value/string.rs | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/tvix/eval/src/value/string.rs b/tvix/eval/src/value/string.rs index 2a7a20209..d0c7a254f 100644 --- a/tvix/eval/src/value/string.rs +++ b/tvix/eval/src/value/string.rs @@ -625,6 +625,11 @@ mod arbitrary { impl NixString { fn new(contents: &[u8], context: Option>) -> Self { + debug_assert!( + !context.as_deref().is_some_and(NixContext::is_empty), + "BUG: initialized with empty context" + ); + // SAFETY: We're always fully initializing a NixString here: // // 1. NixStringInner::alloc sets up the len for us @@ -649,7 +654,10 @@ impl NixString { { Self::new( Self::from(new_contents).as_ref(), - other.context().map(|c| Box::new(c.clone())), + other + .context() + .filter(|ctx| !ctx.is_empty()) + .map(|c| Box::new(c.clone())), ) } @@ -730,7 +738,14 @@ impl NixString { // // Also, we're using the same lifetime and mutability as self, to fit the // pointer-to-reference conversion rules - unsafe { NixStringInner::context_ref(self.0).as_deref() } + let context = unsafe { NixStringInner::context_ref(self.0).as_deref() }; + + debug_assert!( + !context.is_some_and(NixContext::is_empty), + "BUG: empty context" + ); + + context } pub(crate) fn context_mut(&mut self) -> &mut Option> { @@ -739,7 +754,14 @@ impl NixString { // // Also, we're using the same lifetime and mutability as self, to fit the // pointer-to-reference conversion rules - unsafe { NixStringInner::context_mut(self.0) } + let context = unsafe { NixStringInner::context_mut(self.0) }; + + debug_assert!( + !context.as_deref().is_some_and(NixContext::is_empty), + "BUG: empty context" + ); + + context } pub fn iter_context(&self) -> impl Iterator {