From 8a9c45e7d4e9f8d7e155c3a90ca07afc65e87238 Mon Sep 17 00:00:00 2001
From: Laurenz <laurmaedje@gmail.com>
Date: Thu, 30 May 2024 14:46:47 +0200
Subject: [PATCH] Fix race condition in interners (#4300)

---
 crates/typst-syntax/src/file.rs | 11 +++++++----
 crates/typst-utils/src/pico.rs  | 12 ++++++++----
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/crates/typst-syntax/src/file.rs b/crates/typst-syntax/src/file.rs
index b76cb9e39..356337f3f 100644
--- a/crates/typst-syntax/src/file.rs
+++ b/crates/typst-syntax/src/file.rs
@@ -36,17 +36,20 @@ impl FileId {
     #[track_caller]
     pub fn new(package: Option<PackageSpec>, path: VirtualPath) -> Self {
         // Try to find an existing entry that we can reuse.
+        //
+        // We could check with just a read lock, but if the pair is not yet
+        // present, we would then need to recheck after acquiring a write lock,
+        // which is probably not worth it.
         let pair = (package, path);
-        if let Some(&id) = INTERNER.read().unwrap().to_id.get(&pair) {
+        let mut interner = INTERNER.write().unwrap();
+        if let Some(&id) = interner.to_id.get(&pair) {
             return id;
         }
 
-        let mut interner = INTERNER.write().unwrap();
-        let num = interner.from_id.len().try_into().expect("out of file ids");
-
         // Create a new entry forever by leaking the pair. We can't leak more
         // than 2^16 pair (and typically will leak a lot less), so its not a
         // big deal.
+        let num = interner.from_id.len().try_into().expect("out of file ids");
         let id = FileId(num);
         let leaked = Box::leak(Box::new(pair));
         interner.to_id.insert(leaked, id);
diff --git a/crates/typst-utils/src/pico.rs b/crates/typst-utils/src/pico.rs
index b58d6809c..d531f14a5 100644
--- a/crates/typst-utils/src/pico.rs
+++ b/crates/typst-utils/src/pico.rs
@@ -27,15 +27,19 @@ pub struct PicoStr(u32);
 impl PicoStr {
     /// Creates a new interned string.
     pub fn new(string: &str) -> Self {
-        if let Some(&id) = INTERNER.read().unwrap().to_id.get(string) {
+        // Try to find an existing entry that we can reuse.
+        //
+        // We could check with just a read lock, but if the string is not yet
+        // present, we would then need to recheck after acquiring a write lock,
+        // which is probably not worth it.
+        let mut interner = INTERNER.write().unwrap();
+        if let Some(&id) = interner.to_id.get(string) {
             return id;
         }
 
-        let mut interner = INTERNER.write().unwrap();
-        let num = interner.from_id.len().try_into().expect("out of string ids");
-
         // Create a new entry forever by leaking the string. PicoStr is only
         // used for strings that aren't created en masse, so it is okay.
+        let num = interner.from_id.len().try_into().expect("out of string ids");
         let id = Self(num);
         let string = Box::leak(string.to_string().into_boxed_str());
         interner.to_id.insert(string, id);