Fix use after free bug around StringInterner::clone() (#10)

* Add a test which may fail for use after free bug #9

* Implement `Clone` manually for `StringInterner`

New `clone()` implementation recreates `HashMap` to update key
`InternalStrRef`s with pointer to newly cloned strings.

`#[derive(Clone)]` requires `S: Clone` automatically, and the new
`Clone` implementation does not require it explicitly.
However, `S: Symbol` trait bound indirectly requires `S: Clone`, so
the restriction for `S` is actually not changed.

Fixes #9.

src/lib.rs

@@ -192,7 +192,7 @@ pub type DefaultStringInterner = StringInterner<Sym>;
 
 /// Caches strings efficiently, with minimal memory footprint and associates them with unique symbols.
 /// These symbols allow constant time comparisons and look-ups to the underlying interned strings.
-#[derive(Debug, Clone, Eq)]
+#[derive(Debug, Eq)]
 pub struct StringInterner<S, H = RandomState>
 where
 	S: Symbol,
@@ -219,6 +219,28 @@ impl Default for StringInterner<Sym, RandomState> {
 	}
 }
 
+// Should be manually cloned.
+// See <https://github.com/Robbepop/string-interner/issues/9>.
+impl<S, H> Clone for StringInterner<S, H>
+where
+	S: Symbol,
+	H: Clone + BuildHasher,
+{
+	fn clone(&self) -> Self {
+		let values = self.values.clone();
+		let mut map = HashMap::with_capacity_and_hasher(values.len(), self.map.hasher().clone());
+		// Recreate `InternalStrRef` from the newly cloned `Box<str>`s.
+		// Use `extend()` to avoid `H: Default` trait bound required by `FromIterator for HashMap`.
+		map.extend(
+			values
+			.iter()
+			.enumerate()
+			.map(|(i, s)| (InternalStrRef::from_str(s), S::from_usize(i))),
+		);
+		Self { values, map }
+	}
+}
+
 // About `Send` and `Sync` impls for `StringInterner`
 // --------------------------------------------------
 //

src/tests.rs

@@ -373,3 +373,43 @@ mod from_iterator {
 		);
 	}
 }
+
+// See <https://github.com/Robbepop/string-interner/issues/9>.
+mod clone_and_drop {
+	use super::*;
+
+	fn clone_and_drop() -> (DefaultStringInterner, Sym) {
+		let mut old = DefaultStringInterner::new();
+		let foo = old.get_or_intern("foo");
+
+		// Return newly created (cloned) interner, and drop the original `old` itself.
+		(old.clone(), foo)
+	}
+
+	#[test]
+	fn no_use_after_free() {
+		let (mut new, foo) = clone_and_drop();
+
+		// This assert may fail if there are use after free bug.
+		// See <https://github.com/Robbepop/string-interner/issues/9> for detail.
+		assert_eq!(
+			new.get_or_intern("foo"),
+			foo,
+			"`foo` should represent the string \"foo\" so they should be equal"
+		);
+	}
+
+	#[test]
+	// Test for new (non-`derive`) `Clone` impl.
+	fn clone() {
+		let mut old = DefaultStringInterner::new();
+		let strings = &["foo", "bar", "baz", "qux", "quux", "corge"];
+		let syms = strings.iter().map(|&s| old.get_or_intern(s)).collect::<Vec<_>>();
+
+		let mut new = old.clone();
+		for (&s, &sym) in strings.iter().zip(&syms) {
+			assert_eq!(new.resolve(sym), Some(s));
+			assert_eq!(new.get_or_intern(s), sym);
+		}
+	}
+}