From 49750fa1e7e307a323e22f4f0d98ad2dc88d78bf Mon Sep 17 00:00:00 2001
From: edef <edef@edef.eu>
Date: Sat, 25 May 2024 04:24:59 +0000
Subject: 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 <flokli@flokli.de>
---
 tvix/eval/src/value/string.rs | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

(limited to 'tvix')

diff --git a/tvix/eval/src/value/string.rs b/tvix/eval/src/value/string.rs
index 2a7a20209634..d0c7a254fb82 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<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:
         //
         // 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<Box<NixContext>> {
@@ -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<Item = &NixContext> {
-- 
cgit 1.4.1