Use the allocator to properly realign heap buffers when converting between String and SmartString.

Closes bodil#28

Author: bodil

.github/workflows/ci.yml

@@ -110,18 +110,18 @@ jobs:
                   token: ${{ secrets.GITHUB_TOKEN }}
                   args: --all-features
 
-    # miri:
-    #     name: Miri
-    #     runs-on: ubuntu-latest
-    #     steps:
-    #         - uses: actions/checkout@v2
-    #         - uses: actions-rs/toolchain@v1
-    #           with:
-    #               profile: minimal
-    #               toolchain: nightly
-    #               override: true
-    #               components: miri
-    #         - name: Run Miri
-    #           run: |
-    #               cargo miri setup
-    #               cargo miri test -- -Zmiri-disable-isolation -- --skip proptest --skip ser_de
+    miri:
+        name: Miri
+        runs-on: ubuntu-latest
+        steps:
+            - uses: actions/checkout@v2
+            - uses: actions-rs/toolchain@v1
+              with:
+                  profile: minimal
+                  toolchain: nightly
+                  override: true
+                  components: miri
+            - name: Run Miri
+              run: |
+                  cargo miri setup
+                  env MIRIFLAGS=-Zmiri-disable-isolation cargo miri test -- --skip proptest

CHANGELOG.md

@@ -5,6 +5,20 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project
 adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### FIXED
+
+-   To avoid an issue where allocated heap memory may be deallocated with a different layout
+    alignment than it was officially allocated with when converting between `std::string::String`
+    and `SmartString`, even if otherwise correctly aligned, the respective `From` implementations
+    now use `std::alloc::Allocator::grow()` to re-align the heap data as necessary. An unfortunate
+    consequence of this is that because the `std::alloc::Allocator` API hasn't been stabilised yet,
+    unless you're on nightly or some future stable rustc version after `allocator_api` has been
+    stabilised, converting between `String` and `SmartString` will always reallocate and copy
+    (making it always O(n) rather than O(1) when correctly aligned and O(n) otherwise).
+    ([#28](https://github.com/bodil/smartstring/issues/28))
+
 ## [1.0.0] - 2022-02-24
 
 ### CHANGED

Cargo.toml

@@ -12,6 +12,7 @@ categories = ["data-structures"]
 keywords = ["cache-local", "cpu-cache", "small-string", "sso", "inline-string"]
 exclude = ["release.toml", "proptest-regressions/**"]
 rust-version = "1.57"
+build = "./build.rs"
 
 [package.metadata.docs.rs]
 features = ["arbitrary", "proptest", "serde"]
@@ -40,3 +41,7 @@ proptest-derive = "0.3"
 criterion = "0.3"
 rand = "0.8"
 serde_test = "1"
+
+[build-dependencies]
+version_check = "0.9"
+autocfg = "1"

build.rs

@@ -0,0 +1,18 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+use version_check as rustc;
+
+fn main() {
+    let ac = autocfg::new();
+    let has_feature = Some(true) == rustc::supports_feature("allocator_api");
+    let has_api = ac.probe_trait("alloc::alloc::Allocator");
+    if has_feature || has_api {
+        autocfg::emit("has_allocator");
+    }
+    if has_feature {
+        autocfg::emit("needs_allocator_feature");
+    }
+    autocfg::rerun_path("build.rs");
+}

src/boxed.rs

@@ -4,7 +4,7 @@
 
 use alloc::{alloc::Layout, string::String};
 use core::{
-    mem::{align_of, forget},
+    mem::align_of,
     ops::{Deref, DerefMut},
     ptr::NonNull,
 };
@@ -167,30 +167,63 @@ impl DerefMut for BoxedString {
 }
 
 impl From<String> for BoxedString {
+    #[allow(unsafe_code, unused_mut)]
     fn from(mut s: String) -> Self {
         if s.is_empty() {
             Self::new(s.capacity())
         } else {
-            // If the `String`'s buffer isn't word aligned, we can't reuse it.
-            if check_alignment(s.as_ptr()) {
-                return Self::from_str(s.capacity(), &s);
+            #[cfg(has_allocator)]
+            {
+                // TODO: Use String::into_raw_parts when stabilised, meanwhile let's get unsafe
+                let len = s.len();
+                let cap = s.capacity();
+                #[allow(unsafe_code)]
+                let ptr = unsafe { NonNull::new_unchecked(s.as_mut_ptr()) };
+                let old_layout = Layout::array::<u8>(cap).unwrap();
+
+                use alloc::alloc::Allocator;
+                let allocator = alloc::alloc::Global;
+                if let Ok(aligned_ptr) =
+                    unsafe { allocator.grow(ptr, old_layout, Self::layout_for(cap)) }
+                {
+                    core::mem::forget(s);
+                    Self {
+                        cap,
+                        len,
+                        ptr: aligned_ptr.cast(),
+                    }
+                } else {
+                    Self::from_str(cap, &s)
+                }
             }
-            // TODO: Use String::into_raw_parts when stabilised, meanwhile let's get unsafe
-            let len = s.len();
-            let cap = s.capacity();
-            #[allow(unsafe_code)]
-            let ptr = unsafe { NonNull::new_unchecked(s.as_mut_ptr()) };
-            forget(s);
-            Self { cap, len, ptr }
+            #[cfg(not(has_allocator))]
+            Self::from_str(s.capacity(), &s)
         }
     }
 }
 
 impl From<BoxedString> for String {
+    #[allow(unsafe_code)]
     fn from(s: BoxedString) -> Self {
-        #[allow(unsafe_code)]
-        let out = unsafe { String::from_raw_parts(s.ptr.as_ptr(), s.len(), s.capacity()) };
-        forget(s);
-        out
+        #[cfg(has_allocator)]
+        {
+            let ptr = s.ptr;
+            let cap = s.cap;
+            let len = s.len;
+            let new_layout = Layout::array::<u8>(cap).unwrap();
+
+            use alloc::alloc::Allocator;
+            let allocator = alloc::alloc::Global;
+            if let Ok(aligned_ptr) =
+                unsafe { allocator.grow(ptr, BoxedString::layout_for(cap), new_layout) }
+            {
+                core::mem::forget(s);
+                unsafe { String::from_raw_parts(aligned_ptr.as_ptr().cast(), len, cap) }
+            } else {
+                String::from(s.deref())
+            }
+        }
+        #[cfg(not(has_allocator))]
+        String::from(s.deref())
     }
 }

src/lib.rs

@@ -101,6 +101,7 @@
 #![deny(nonstandard_style)]
 #![warn(unreachable_pub, missing_debug_implementations, missing_docs)]
 #![cfg_attr(not(feature = "std"), no_std)]
+#![cfg_attr(needs_allocator_feature, feature(allocator_api))]
 
 extern crate alloc;
 

src/test.rs

@@ -551,4 +551,16 @@ mod tests {
         s.clear();
         assert_eq!(Discriminant::Inline, s.discriminant());
     }
+
+    #[test]
+    fn from_string() {
+        let std_s =
+            String::from("I am a teapot short and stout; here is my handle, here is my snout");
+        let smart_s: SmartString<LazyCompact> = std_s.clone().into();
+        assert_eq!(std_s, smart_s);
+        let unsmart_s: String = smart_s.clone().into();
+        assert_eq!(smart_s, unsmart_s);
+        assert_eq!(std_s, unsmart_s);
+        // This test exists just to provoke a Miri problem when dropping a string created by SmartString::into::<String>() (#28)
+    }
 }