[PATCH v2] libstdc++: Ensure filesystem::path internals are sufficiently aligned [PR122255]

[Tomasz Kaminski]

On Mon, Oct 13, 2025 at 3:47 PM Jonathan Wakely <jwakely@redhat.com> wrote:

> We need the memory allocated for a path::_List::_Impl to be at least
> 4-byte aligned, so that we can use the two least significant bits to
> store a _Type value. Use __STDCPP_DEFAULT_NEW_ALIGNMENT__ to check that
> in a static_assert. Also add a static_assert to check the memory will be
> aligned suitably for the _Impl object itself. In practice both
> assertions should pass as long as operator new guarantees to return
> memory with at least 4-byte alignment, which seems to be true for malloc
> on GCC's supported targets.
>
> Allocation of path::_List::_Impl objects is refactored into a new
> _Impl::create function so that the memory allocation is done in one
> place, rather than being repeated in path::_List::_Impl::copy and
> path::_List::reserve. If we late decide to use aligned-new to support
> targets that fail the new static assertions we won't need to do that in
> two different places. Calling operator delete already only happens in
> one place, the _Impl_deleter.
>
> The create function is actually implemented in terms of another new
> function, create_unchecked. The overflow checks in create aren't needed
> when copying an existing object, because we already checked its size
> doesn't overflow.
>
> Destroying the components is now done by a destructor, which the
> _Impl_deleter invokes.
>
> libstdc++-v3/ChangeLog:
>
>         PR libstdc++/122255
>         * src/c++17/fs_path.cc (path::_List::_Impl::~_Impl): Define
>         destructor.
>         (path::_List::_Impl::copy): Use create_unchecked.
>         (path::_List::_Impl): Add static assertions.
>         (path::_List::_Impl::create): New static member function.
>         (path::_List::_Impl::create_unchecked): Likewise.
>         (path::_List::_Impl_deleter::operator()): Use destructor.
>         (path::_List::reserve): Use create.
> ---
>
> v2: Just assert that __STDCPP_DEFAULT_NEW_ALIGNMENT__ is sufficient,
> instead of adding code that probably won't be needed. Split the new
> create function into create and create_unchecked. Add a destructor.
>
I was sure that I had responded, LGTM.

>
> Tested x86_64-linux.
>
>  libstdc++-v3/src/c++17/fs_path.cc | 74 +++++++++++++++++++++----------
>  1 file changed, 50 insertions(+), 24 deletions(-)
>
> diff --git a/libstdc++-v3/src/c++17/fs_path.cc
> b/libstdc++-v3/src/c++17/fs_path.cc
> index 215afa08ad25..03bb5ecb7be1 100644
> --- a/libstdc++-v3/src/c++17/fs_path.cc
> +++ b/libstdc++-v3/src/c++17/fs_path.cc
> @@ -34,6 +34,7 @@
>  #include <filesystem>
>  #include <algorithm>
>  #include <array>
> +#include <new>
>  #include <bits/stl_uninitialized.h>
>  #include <ext/numeric_traits.h> // __gnu_cxx::__int_traits
>
> @@ -207,6 +208,10 @@ struct path::_List::_Impl
>
>    _Impl(int cap) : _M_size(0), _M_capacity(cap) { }
>
> +  ~_Impl() { clear(); }
> +
> +  // Align the first member like the value_type so that we can store one
> or
> +  // more objects of that type immediately after the memory occupied by
> *this.
>    alignas(value_type) int _M_size;
>    int _M_capacity;
>
> @@ -246,29 +251,67 @@ struct path::_List::_Impl
>    unique_ptr<_Impl, _Impl_deleter> copy() const
>    {
>      const auto n = size();
> -    void* p = ::operator new(sizeof(_Impl) + n * sizeof(value_type));
> -    unique_ptr<_Impl, _Impl_deleter> newptr(::new (p) _Impl{n});
> +    // *this already has n elements so don't need to check if n overflows:
> +    auto newptr = create_unchecked(n);
>      std::uninitialized_copy_n(begin(), n, newptr->begin());
>      newptr->_M_size = n;
>      return newptr;
>    }
>
> +  // We use the two least significant bits to store a _Type value so
> +  // require memory aligned to at least 4 bytes:
> +  static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ >= 4);
> +  // Require memory suitably aligned for an _Impl and its value types:
> +  static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ >= alignof(value_type));
> +
>    // Clear the lowest two bits from the pointer (i.e. remove the _Type
> value)
>    static _Impl* notype(_Impl* p)
>    {
>      constexpr uintptr_t mask = ~(uintptr_t)0x3;
>      return reinterpret_cast<_Impl*>(reinterpret_cast<uintptr_t>(p) &
> mask);
>    }
> +
> +  // Create a new _Impl with capacity for n components.
> +  static unique_ptr<_Impl, _Impl_deleter>
> +  create(int n)
> +  {
> +    using __gnu_cxx::__int_traits;
> +    // Nobody should need paths with this many components.
> +    if (n >= __int_traits<int>::__max / 4)
> +      std::__throw_bad_alloc();
> +
> +    if constexpr (__int_traits<int>::__max >= __int_traits<size_t>::__max)
> +      {
> +       // Check that the calculation in create_unchecked(n) won't
> overflow.
> +       size_t bytes;
> +       if (__builtin_mul_overflow(n, sizeof(value_type), &bytes)
> +             || __builtin_add_overflow(sizeof(_Impl), bytes, &bytes))
> +         std::__throw_bad_alloc();
> +      }
> +    // Otherwise, it can't overflow, even for 20-bit size_t on msp430.
> +
> +    return create_unchecked(n);
> +  }
> +
> +  // pre: no overflow in Si + n * Sv
> +  static unique_ptr<_Impl, _Impl_deleter>
> +  create_unchecked(int n)
> +  {
> +    void* p = ::operator new(sizeof(_Impl) + n * sizeof(value_type));
> +    return std::unique_ptr<_Impl, _Impl_deleter>(::new(p) _Impl{n});
> +  }
>  };
>
> -void path::_List::_Impl_deleter::operator()(_Impl* p) const noexcept
> +// Destroy and deallocate an _Impl.
> +void
> +path::_List::_Impl_deleter::operator()(_Impl* p) const noexcept
>  {
>    p = _Impl::notype(p);
>    if (p)
>      {
> -      __glibcxx_assert(p->_M_size <= p->_M_capacity);
> -      p->clear();
> -      ::operator delete(p, sizeof(*p) + p->_M_capacity *
> sizeof(value_type));
> +      const auto n = p->_M_capacity;
> +      p->~_Impl();
> +      ::operator delete(p, sizeof(_Impl) + n * sizeof(_Impl::value_type));
>      }
>  }
>
> @@ -455,24 +498,7 @@ path::_List::reserve(int newcap, bool exact = false)
>             newcap = nextcap;
>         }
>
> -      using __gnu_cxx::__int_traits;
> -      // Nobody should need paths with this many components.
> -      if (newcap >= __int_traits<int>::__max / 4)
> -       std::__throw_bad_alloc();
> -
> -      size_t bytes;
> -      if constexpr (__int_traits<int>::__max >=
> __int_traits<size_t>::__max)
> -       {
> -         size_t components;
> -         if (__builtin_mul_overflow(newcap, sizeof(value_type),
> &components)
> -               || __builtin_add_overflow(sizeof(_Impl), components,
> &bytes))
> -           std::__throw_bad_alloc();
> -       }
> -      else // This won't overflow, even for 20-bit size_t on msp430.
> -       bytes = sizeof(_Impl) + newcap * sizeof(value_type);
> -
> -      void* p = ::operator new(bytes);
> -      std::unique_ptr<_Impl, _Impl_deleter> newptr(::new(p)
> _Impl{newcap});
> +      auto newptr = _Impl::create(newcap);
>        const int cursize = curptr ? curptr->size() : 0;
>        if (cursize)
>         {
> --
> 2.51.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20251014/464392c0/attachment.htm>