Comment 1 Yuri Victorovich 2024-06-07 16:10:09 UTC

> When I run gperftools unit tests "debug" configurations pretty much ~all fail due to mismatch between requested allocation size and allocation size passed to sized delete.

I can't reproduce this on FreeBSD 14.1-STABLE stable/14-n267671-9a8a26aefb36.

There are various other failures in tests, but most tests pass.

Here is reproduction independent from gperftools. There is likely even simpler reproduction as I haven't looked deeper what gtest bits are doing, but they seem to be doing just general STL bits. Also please note that we're making it a bug by passing -fsized-deallocation to clang.

* clone https://github.com/google/googletest
* cd googletest
* grab https://gist.github.com/alk/f73d13f73d47deaca3921cb8d7660bf2
* then run:

c++ -o tt -pthread -std=gnu++17 -Wall -fsized-deallocation -Og -ggdb3 -Igoogletest/include -Igoogletest trivial-sized-test.cc googletest/src/gtest_main.cc googletest/src/gtest-assertion-result.cc googletest/src/gtest-death-test.cc googletest/src/gtest-filepath.cc googletest/src/gtest-matchers.cc googletest/src/gtest-port.cc googletest/src/gtest-printers.cc googletest/src/gtest-test-part.cc googletest/src/gtest-typed-test.cc googletest/src/gtest.cc

It fails on my freebsd 14.1 VM (x64).

Comment 3 Dimitry Andric 2024-06-07 18:35:25 UTC

This is a bit of a false positive due to internal details of libc++ std::string.

Normally the size of a std::string is 24 bytes on amd64, but only if it contains a 'short' string. When the contained string is too long to fit in the object itself, it's reallocated on the heap instead.

In this particular case some TCMalloc test string comes out as 25 bytes, and that is what libc++ eventually calls malloc() with. However due to internal administration issues, it calculates the wrong size for the corresponding delete.

This was also noticed by AddressSanitizer after <https://github.com/llvm/llvm-project/pull/83774> ("Enable sized deallocation by default in C++14 onwards"), and fixed by Vitaly Buka in <https://github.com/llvm/llvm-project/commit/d129ea8d2fa3>.

I will merge that fix into -CURRENT, with one prerequisite fix added, so we can see if there's any fallout (but it's not likely). Then I will MFC it, and we can think about a 14.1 erratum.

Comment 4 commit-hook 2024-06-07 18:46:09 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ead8e4c081e5c4de4d508fc353f381457b058ca6

commit ead8e4c081e5c4de4d508fc353f381457b058ca6
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2024-06-07 18:42:53 +0000
Commit:     Dimitry Andric <dim@FreeBSD.org>
CommitDate: 2024-06-07 18:43:46 +0000

    Merge commit 382f70a877f0 from llvm-project (by Louis Dionne):

      [libc++][NFC] Rewrite function call on two lines for clarity (#79141)

      Previously, there was a ternary conditional with a less-than comparison
      appearing inside a template argument, which was really confusing because
      of the <...> of the function template. This patch rewrites the same
      statement on two lines for clarity.

    Merge commit d129ea8d2fa3 from llvm-project (by Vitaly Buka):

      [libcxx] Align `__recommend() + 1`  by __endian_factor (#90292)

      This is detected by asan after #83774

      Allocation size will be divided by `__endian_factor` before storing. If
      it's not aligned,
      we will not be able to recover allocation size to pass into
      `__alloc_traits::deallocate`.

      we have code like this
      ```
       auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
          __p               = __allocation.ptr;
          __set_long_cap(__allocation.count);

      void __set_long_cap(size_type __s) _NOEXCEPT {
          __r_.first().__l.__cap_     = __s / __endian_factor;
          __r_.first().__l.__is_long_ = true;
        }

      size_type __get_long_cap() const _NOEXCEPT {
          return __r_.first().__l.__cap_ * __endian_factor;
        }

      inline ~basic_string() {
          __annotate_delete();
          if (__is_long())
            __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
        }
      ```
      1. __recommend() -> even size
      2. `std::__allocate_at_least(__alloc(), __recommend(__sz) + 1)` - > not
      even size
      3. ` __set_long_cap() `- > lose one bit of size for __endian_factor == 2
      (see `/ __endian_factor`)
      4. `__alloc_traits::deallocate(__alloc(), __get_long_pointer(),
      __get_long_cap())` -> uses even size (see `__get_long_cap`)

    This should fix incorrect deallocation sizes for some instances of
    std::string. Memory profiling or debugging tools like AddressSanitizer,
    LeakSanitizer or TCMalloc could then complain about the the size passed
    to a deallocation not matching the size originally passed to the
    allocation.

    Reported by:    Aliaksei Kandratsenka <alkondratenko@gmail.com>
    PR:             279560
    MFC after:      3 days

 contrib/llvm-project/libcxx/include/string | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comment 5 Dimitry Andric 2024-06-07 18:49:01 UTC

After this change, with a rebuilt and reinstalled libc++ (headers + so), almost all gperftools tests succeed now, except for one test that is dependent on /proc filesystem layout:

make  check-TESTS
PASS: unique_path_unittest
PASS: generic_writer_test
FAIL: proc_maps_iterator_test
PASS: low_level_alloc_unittest
PASS: stacktrace_unittest
PASS: check_address_test
PASS: addressmap_unittest
PASS: packed_cache_test
PASS: safe_strerror_test
PASS: cleanup_test
PASS: function_ref_test
PASS: pagemap_unittest
PASS: page_heap_test
PASS: stack_trace_table_test
PASS: malloc_hook_test
PASS: mmap_hook_test
PASS: sampler_test
PASS: tcmalloc_minimal_unittest
PASS: tcm_min_asserts_unittest
PASS: tcmalloc_minimal_large_unittest
PASS: tcmalloc_minimal_large_heap_fragmentation_unittest
PASS: system_alloc_unittest
PASS: frag_unittest
PASS: markidle_unittest
PASS: current_allocated_bytes_test
PASS: malloc_extension_test
PASS: malloc_extension_c_test
PASS: memalign_unittest
PASS: realloc_unittest
PASS: thread_dealloc_unittest
PASS: min_per_thread_cache_size_test
PASS: tcmalloc_minimal_debug_unittest
PASS: malloc_extension_debug_test
PASS: memalign_debug_unittest
PASS: realloc_debug_unittest
PASS: debugallocation_test
PASS: tcmalloc_unittest
PASS: tcm_asserts_unittest
PASS: tcmalloc_both_unittest
PASS: tcmalloc_large_unittest
PASS: tcmalloc_large_heap_fragmentation_unittest
PASS: sampling_test
rm -f heap-profiler_unittest.sh
cp -p /home/dim/src/gperftools/src/tests/heap-profiler_unittest.sh heap-profiler_unittest.sh
PASS: heap-profiler_unittest.sh
PASS: tcmalloc_debug_unittest
PASS: sampling_debug_test
rm -f heap-profiler_debug_unittest.sh
cp -p /home/dim/src/gperftools/src/tests/heap-profiler_unittest.sh heap-profiler_debug_unittest.sh
PASS: heap-profiler_debug_unittest.sh
============================================================================
Testsuite summary for gperftools 2.15
============================================================================
# TOTAL: 46
# PASS:  45
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0
============================================================================

This is with gperftools master, gperftools-2.15-179-gaddf751.

Comment 6 Dimitry Andric 2024-06-07 18:53:40 UTC

Right, the procmaps failure was because I had no /proc mounted at all on this 14.1-R VM. :)

After mounting procfs on /proc, all gperftools tests succeed now.

Thanks for the update. I am frankly shocked, because the issue appears to be just std::string, so Google should have caught this internally. And somehow they haven't.  Do you have any details on the age of that bug? (I.e. what bad sizing was introduced in std::string, not when it was caught by asan changing defaults)

Also curious what makes you say "This is a bit of a false positive". It is a proper bug, as mallocs that implement sized delete and actually trust passed size will crash. Notably, our behavior by default is to not trust the passed size exactly for fear of bugs like this, but our debugging malloc is checking things (as is "abseil" tcmalloc's).

When/if I can expect the fixed version ? I don't have to bother, but I do prefer to have 14.1 testing VM and if it simply fails, I cannot test on freebsd 14.1. And I am not eager to add workarounds. On the other hand, I won't be surprised if this bug is reproducible on some other libcxx configs (i.e. you can ask for it on GNU/Linux)

So looking at this more, indeed, at least libc++-18-dev on Debian is broken as well.

Comment 9 Dimitry Andric 2024-06-07 19:29:55 UTC

(In reply to Aliaksei Kandratsenka from comment #7)
> Thanks for the update. I am frankly shocked, because the issue appears to be just std::string, so Google should have caught this internally. And somehow they haven't.

Look at https://github.com/llvm/llvm-project/commit/cf5a8b489464, which was committed on 2024-04-26 so just a few weeks ago. This was a retry of https://reviews.llvm.org/D112921 (from 2021) which also attempted to enable sized deallocation by default. Apparently, it was not that easy to fix all the fallout it caused, which is likely why it took so long to get this into llvm main.

The day after that commit, the ASan people (Vitaly is one of them) noticed the std::string mismatches, and submitted https://github.com/llvm/llvm-project/pull/90292 . There is a bit of discussion in there which may be interesting.

Long story short: the Google people did notice it, but only a few weeks ago. And it only applies to relatively cutting edge llvm (19.x only). And of course to people who turned on -fsized-deallocation manually!


> Do you have any details on the age of that bug? (I.e. what bad sizing was introduced in std::string, not when it was caught by asan changing defaults)

I'm not entirely sure, but since the beginning libc++'s string has been a SSO variant, with a __get_long_cap() function to retrieve the 'long' capacity if the contents was allocated on the heap. Possibly https://github.com/llvm/llvm-project/commit/29c8c070a1770fc510ccad3be753f6f50336f8cc (from 2022-04-21) could have introduced a bug, but maybe it is even older.

I guess the only way to really check is to build older libc++'s and run gperftools against them... Maybe if I'm bored this weekend. :-)


> Also curious what makes you say "This is a bit of a false positive". It is a proper bug, as mallocs that implement sized delete and actually trust passed size will crash.

Yes, apologies for that, this was too strong. It is indeed a real bug, but since sized deallocation is not on by default, and you must also use an allocator that even cares about the free'd size it will not affect many people.


> When/if I can expect the fixed version ?

I will merge the fix to stable/14 and stable/13 after 3 days, then I can send a EN request to the re@ people for post-14.1-RELEASE, but generally that takes a while. My guess is that there are quite a few fixes lined up, but they were eager to get 14.1-RELEASE out the door.

Comment 10 Dimitry Andric 2024-06-08 16:54:21 UTC

Some further digging shows that at least for amd64, this regressed with https://github.com/llvm/llvm-project/commit/04ce0baf015c ("Unconditionally lower std::string's alignment requirement from 16 to 8"), which went in very soon after 18.x branched. And it was merged to 18.x in https://github.com/llvm/llvm-project/commit/5de1bcb70db2, so a bit after 18.1.0-rc1.

Therefore I think this problem will only occur for libc++ 18.1.0 and later.

Comment 11 commit-hook 2024-06-10 07:30:24 UTC

A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=55c5dad2f305f74d1ff5ca85c453635511aab9b2

commit 55c5dad2f305f74d1ff5ca85c453635511aab9b2
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2024-06-07 18:42:53 +0000
Commit:     Dimitry Andric <dim@FreeBSD.org>
CommitDate: 2024-06-10 07:29:25 +0000

    Merge commit 382f70a877f0 from llvm-project (by Louis Dionne):

      [libc++][NFC] Rewrite function call on two lines for clarity (#79141)

      Previously, there was a ternary conditional with a less-than comparison
      appearing inside a template argument, which was really confusing because
      of the <...> of the function template. This patch rewrites the same
      statement on two lines for clarity.

    Merge commit d129ea8d2fa3 from llvm-project (by Vitaly Buka):

      [libcxx] Align `__recommend() + 1`  by __endian_factor (#90292)

      This is detected by asan after #83774

      Allocation size will be divided by `__endian_factor` before storing. If
      it's not aligned,
      we will not be able to recover allocation size to pass into
      `__alloc_traits::deallocate`.

      we have code like this
      ```
       auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
          __p               = __allocation.ptr;
          __set_long_cap(__allocation.count);

      void __set_long_cap(size_type __s) _NOEXCEPT {
          __r_.first().__l.__cap_     = __s / __endian_factor;
          __r_.first().__l.__is_long_ = true;
        }

      size_type __get_long_cap() const _NOEXCEPT {
          return __r_.first().__l.__cap_ * __endian_factor;
        }

      inline ~basic_string() {
          __annotate_delete();
          if (__is_long())
            __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
        }
      ```
      1. __recommend() -> even size
      2. `std::__allocate_at_least(__alloc(), __recommend(__sz) + 1)` - > not
      even size
      3. ` __set_long_cap() `- > lose one bit of size for __endian_factor == 2
      (see `/ __endian_factor`)
      4. `__alloc_traits::deallocate(__alloc(), __get_long_pointer(),
      __get_long_cap())` -> uses even size (see `__get_long_cap`)

    This should fix incorrect deallocation sizes for some instances of
    std::string. Memory profiling or debugging tools like AddressSanitizer,
    LeakSanitizer or TCMalloc could then complain about the the size passed
    to a deallocation not matching the size originally passed to the
    allocation.

    Reported by:    Aliaksei Kandratsenka <alkondratenko@gmail.com>
    PR:             279560
    MFC after:      3 days

    (cherry picked from commit ead8e4c081e5c4de4d508fc353f381457b058ca6)

 contrib/llvm-project/libcxx/include/string | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comment 12 commit-hook 2024-06-10 07:30:26 UTC

A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ef4d145057c10e452ac07b3111b4b4d0c5382fb5

commit ef4d145057c10e452ac07b3111b4b4d0c5382fb5
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2024-06-07 18:42:53 +0000
Commit:     Dimitry Andric <dim@FreeBSD.org>
CommitDate: 2024-06-10 07:29:30 +0000

    Merge commit 382f70a877f0 from llvm-project (by Louis Dionne):

      [libc++][NFC] Rewrite function call on two lines for clarity (#79141)

      Previously, there was a ternary conditional with a less-than comparison
      appearing inside a template argument, which was really confusing because
      of the <...> of the function template. This patch rewrites the same
      statement on two lines for clarity.

    Merge commit d129ea8d2fa3 from llvm-project (by Vitaly Buka):

      [libcxx] Align `__recommend() + 1`  by __endian_factor (#90292)

      This is detected by asan after #83774

      Allocation size will be divided by `__endian_factor` before storing. If
      it's not aligned,
      we will not be able to recover allocation size to pass into
      `__alloc_traits::deallocate`.

      we have code like this
      ```
       auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
          __p               = __allocation.ptr;
          __set_long_cap(__allocation.count);

      void __set_long_cap(size_type __s) _NOEXCEPT {
          __r_.first().__l.__cap_     = __s / __endian_factor;
          __r_.first().__l.__is_long_ = true;
        }

      size_type __get_long_cap() const _NOEXCEPT {
          return __r_.first().__l.__cap_ * __endian_factor;
        }

      inline ~basic_string() {
          __annotate_delete();
          if (__is_long())
            __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
        }
      ```
      1. __recommend() -> even size
      2. `std::__allocate_at_least(__alloc(), __recommend(__sz) + 1)` - > not
      even size
      3. ` __set_long_cap() `- > lose one bit of size for __endian_factor == 2
      (see `/ __endian_factor`)
      4. `__alloc_traits::deallocate(__alloc(), __get_long_pointer(),
      __get_long_cap())` -> uses even size (see `__get_long_cap`)

    This should fix incorrect deallocation sizes for some instances of
    std::string. Memory profiling or debugging tools like AddressSanitizer,
    LeakSanitizer or TCMalloc could then complain about the the size passed
    to a deallocation not matching the size originally passed to the
    allocation.

    Reported by:    Aliaksei Kandratsenka <alkondratenko@gmail.com>
    PR:             279560
    MFC after:      3 days

    (cherry picked from commit ead8e4c081e5c4de4d508fc353f381457b058ca6)

 contrib/llvm-project/libcxx/include/string | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comment 13 commit-hook 2024-06-19 20:37:21 UTC

A commit in branch releng/14.1 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8e0e6b428cb8ba01941e53256880f7993671e786

commit 8e0e6b428cb8ba01941e53256880f7993671e786
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2024-06-07 18:42:53 +0000
Commit:     Gordon Tetlow <gordon@FreeBSD.org>
CommitDate: 2024-06-18 17:36:40 +0000

    Merge commit 382f70a877f0 from llvm-project (by Louis Dionne):

      [libc++][NFC] Rewrite function call on two lines for clarity (#79141)

      Previously, there was a ternary conditional with a less-than comparison
      appearing inside a template argument, which was really confusing because
      of the <...> of the function template. This patch rewrites the same
      statement on two lines for clarity.

    Merge commit d129ea8d2fa3 from llvm-project (by Vitaly Buka):

      [libcxx] Align `__recommend() + 1`  by __endian_factor (#90292)

      This is detected by asan after #83774

      Allocation size will be divided by `__endian_factor` before storing. If
      it's not aligned,
      we will not be able to recover allocation size to pass into
      `__alloc_traits::deallocate`.

      we have code like this
      ```
       auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
          __p               = __allocation.ptr;
          __set_long_cap(__allocation.count);

      void __set_long_cap(size_type __s) _NOEXCEPT {
          __r_.first().__l.__cap_     = __s / __endian_factor;
          __r_.first().__l.__is_long_ = true;
        }

      size_type __get_long_cap() const _NOEXCEPT {
          return __r_.first().__l.__cap_ * __endian_factor;
        }

      inline ~basic_string() {
          __annotate_delete();
          if (__is_long())
            __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
        }
      ```
      1. __recommend() -> even size
      2. `std::__allocate_at_least(__alloc(), __recommend(__sz) + 1)` - > not
      even size
      3. ` __set_long_cap() `- > lose one bit of size for __endian_factor == 2
      (see `/ __endian_factor`)
      4. `__alloc_traits::deallocate(__alloc(), __get_long_pointer(),
      __get_long_cap())` -> uses even size (see `__get_long_cap`)

    This should fix incorrect deallocation sizes for some instances of
    std::string. Memory profiling or debugging tools like AddressSanitizer,
    LeakSanitizer or TCMalloc could then complain about the the size passed
    to a deallocation not matching the size originally passed to the
    allocation.

    Reported by:    Aliaksei Kandratsenka <alkondratenko@gmail.com>
    PR:             279560
    Approved by:    so
    Security:       FreeBSD-EN-24:13.libc++

    (cherry picked from commit ead8e4c081e5c4de4d508fc353f381457b058ca6)
    (cherry picked from commit 55c5dad2f305f74d1ff5ca85c453635511aab9b2)

 contrib/llvm-project/libcxx/include/string | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)