mdebray/tvl-depot
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(tvix/eval): disallow empty but allocated string contexts

Browse source 
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 <flokli@flokli.de>
This commit is contained in:
edef 2024-05-25 04:24:59 +00:00
parent c4c42c8b6c
commit 49750fa1e7
1 changed files with 25 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
tvix/eval/src/value/string.rs
View file

@ -625,6 +625,11 @@ mod arbitrary {
impl NixString { impl NixString {
fn new(contents: &[u8], context: Option<Box<NixContext>>) -> Self { fn new(contents: &[u8], context: Option<Box<NixContext>>) -> 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: // SAFETY: We're always fully initializing a NixString here:
// //
// 1. NixStringInner::alloc sets up the len for us // 1. NixStringInner::alloc sets up the len for us
@ -649,7 +654,10 @@ impl NixString {
{ {
Self::new( Self::new(
Self::from(new_contents).as_ref(), 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 // Also, we're using the same lifetime and mutability as self, to fit the
// pointer-to-reference conversion rules // 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<Box<NixContext>> { pub(crate) fn context_mut(&mut self) -> &mut Option<Box<NixContext>> {
@ -739,7 +754,14 @@ impl NixString {
// //
// Also, we're using the same lifetime and mutability as self, to fit the // Also, we're using the same lifetime and mutability as self, to fit the
// pointer-to-reference conversion rules // 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<Item = &NixContext> { pub fn iter_context(&self) -> impl Iterator<Item = &NixContext> {