Page MenuHomePhabricator

[libc++] Fix memory leaks when throwing inside std::vector constructors
Needs ReviewPublic

Authored by philnik on Wed, Nov 23, 12:54 PM.

Details

Reviewers
ldionne
Mordante
var-const
Group Reviewers
Restricted Project
Summary

Fixes #58392

Diff Detail

Unit TestsFailed

TimeTest
1,000 mslibcxx CI AArch64 -fno-exceptions > llvm-libc++-shared-cfg-in.std/containers/sequences/vector_bool::ctor_exceptions.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/std/containers/sequences/vector.bool/ctor_exceptions.pass.cpp --target=aarch64-linux-gnu -nostdinc++ -I /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/include/c++/v1 -I /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/include/c++/v1 -I /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/lib -Wl,-rpath,/home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/lib -lc++ -pthread -o /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/test/std/containers/sequences/vector.bool/Output/ctor_exceptions.pass.cpp.dir/t.tmp.exe
1,040 mslibcxx CI Armv7 -fno-exceptions > llvm-libc++-shared-cfg-in.std/containers/sequences/vector_bool::ctor_exceptions.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildbot/worker/linaro-armv8-libcxx-03/llvm-project/libcxx-ci/libcxx/test/std/containers/sequences/vector.bool/ctor_exceptions.pass.cpp --target=armv7l-linux-gnueabihf -nostdinc++ -I /home/tcwg-buildbot/worker/linaro-armv8-libcxx-03/llvm-project/libcxx-ci/build/armv7-noexceptions/include/c++/v1 -I /home/tcwg-buildbot/worker/linaro-armv8-libcxx-03/llvm-project/libcxx-ci/build/armv7-noexceptions/include/c++/v1 -I /home/tcwg-buildbot/worker/linaro-armv8-libcxx-03/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/tcwg-buildbot/worker/linaro-armv8-libcxx-03/llvm-project/libcxx-ci/build/armv7-noexceptions/lib -Wl,-rpath,/home/tcwg-buildbot/worker/linaro-armv8-libcxx-03/llvm-project/libcxx-ci/build/armv7-noexceptions/lib -lc++ -pthread -o /home/tcwg-buildbot/worker/linaro-armv8-libcxx-03/llvm-project/libcxx-ci/build/armv7-noexceptions/test/std/containers/sequences/vector.bool/Output/ctor_exceptions.pass.cpp.dir/t.tmp.exe
980 mslibcxx CI Armv8 -fno-exceptions > llvm-libc++-shared-cfg-in.std/containers/sequences/vector_bool::ctor_exceptions.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildbot/worker/linaro-armv8-libcxx-01/llvm-project/libcxx-ci/libcxx/test/std/containers/sequences/vector.bool/ctor_exceptions.pass.cpp --target=armv8l-linux-gnueabihf -nostdinc++ -I /home/tcwg-buildbot/worker/linaro-armv8-libcxx-01/llvm-project/libcxx-ci/build/armv8-noexceptions/include/c++/v1 -I /home/tcwg-buildbot/worker/linaro-armv8-libcxx-01/llvm-project/libcxx-ci/build/armv8-noexceptions/include/c++/v1 -I /home/tcwg-buildbot/worker/linaro-armv8-libcxx-01/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/tcwg-buildbot/worker/linaro-armv8-libcxx-01/llvm-project/libcxx-ci/build/armv8-noexceptions/lib -Wl,-rpath,/home/tcwg-buildbot/worker/linaro-armv8-libcxx-01/llvm-project/libcxx-ci/build/armv8-noexceptions/lib -lc++ -pthread -o /home/tcwg-buildbot/worker/linaro-armv8-libcxx-01/llvm-project/libcxx-ci/build/armv8-noexceptions/test/std/containers/sequences/vector.bool/Output/ctor_exceptions.pass.cpp.dir/t.tmp.exe
4,160 mslibcxx CI Debug mode > llvm-libc++-shared-cfg-in.libcxx/debug/containers::sequence_container_iterators.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-16 /home/libcxx-builder/.buildkite-agent/builds/f47fef16e99b-1/llvm-project/libcxx-ci/libcxx/test/libcxx/debug/containers/sequence_container_iterators.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/f47fef16e99b-1/llvm-project/libcxx-ci/build/generic-debug-mode/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f47fef16e99b-1/llvm-project/libcxx-ci/build/generic-debug-mode/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f47fef16e99b-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/f47fef16e99b-1/llvm-project/libcxx-ci/build/generic-debug-mode/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/f47fef16e99b-1/llvm-project/libcxx-ci/build/generic-debug-mode/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/f47fef16e99b-1/llvm-project/libcxx-ci/build/generic-debug-mode/test/libcxx/debug/containers/Output/sequence_container_iterators.pass.cpp.dir/t.tmp.exe
1,060 mslibcxx CI No exceptions > llvm-libc++-shared-cfg-in.std/containers/sequences/vector_bool::ctor_exceptions.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-16 /home/libcxx-builder/.buildkite-agent/builds/ca66041a1965-1/llvm-project/libcxx-ci/libcxx/test/std/containers/sequences/vector.bool/ctor_exceptions.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ca66041a1965-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ca66041a1965-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ca66041a1965-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/ca66041a1965-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ca66041a1965-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/ca66041a1965-1/llvm-project/libcxx-ci/build/generic-noexceptions/test/std/containers/sequences/vector.bool/Output/ctor_exceptions.pass.cpp.dir/t.tmp.exe

Event Timeline

philnik created this revision.Wed, Nov 23, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 23, 12:54 PM
philnik requested review of this revision.Wed, Nov 23, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 23, 12:54 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik edited the summary of this revision. (Show Details)Wed, Nov 23, 12:58 PM
philnik updated this revision to Diff 477668.Wed, Nov 23, 6:28 PM

Try to fix CI

ldionne added inline comments.Thu, Nov 24, 6:05 AM
libcxx/include/vector
297

In the bug report, you mention this was introduced when we removed the base class. Are you referring to b82da8b555603b172f826fdd5ea902eff3aeb7ad?

I looked at the commit and I still don't see how that introduced this issue (but I do acknowledge that this is a real issue, and a somewhat embarrassing one at that).

1068

This comment applies to all the constructors. If __vallocate() fails, we won't call the destructor, so we won't remove *this from the debug database.

That being said, the legacy debug mode is already broken pretty bad so I'm not sure we can or want to fix it.

libcxx/test/std/containers/sequences/vector/vector.cons/throwing.pass.cpp
11 ↗(On Diff #477668)

Please add a link to the bug report.

92 ↗(On Diff #477668)

Instead, I would list the signatures of the constructors you're testing. It will make it clearer what's going on at a glance.

ldionne added inline comments.Thu, Nov 24, 6:10 AM
libcxx/include/vector
297

Oh crap, I see it now. It's from 12b55821a578929a7b03448a22c3a678aa649bd5.

And now I finally understand what the motivation for introducing __vector_base probably was when Howard wrote it. What a great lesson. I still think it was too subtle to not even have a comment, but there's definitely something to learn from this one.

Since we made similar changes to string and perhaps other types, we'll have to also look at those with this bug in mind.

ldionne added inline comments.Thu, Nov 24, 6:19 AM
libcxx/include/vector
297

I looked at the changes we made in string and deque, and we didn't do anything similar.

Independently of any changes we might have made, I looked at the string implementation and I don't think it suffers from the same issue. Indeed, we sometimes allocate and then copy characters into the string, but char_traits is required not to throw anything, so I don't think the copy step can fail.

deque however seems to have the same problem (but not introduced by us). In deque::__append (which is called from various constructors), we sometimes seem to allocate and then copy over elements, the constructors of which may throw.

ldionne added inline comments.Thu, Nov 24, 6:22 AM
libcxx/test/std/containers/sequences/vector/vector.cons/throwing.pass.cpp
19 ↗(On Diff #477668)

I don't understand how this test is testing anything -- we're not asserting anything. Are we relying on the msan bot?

Instead, I would suggest that we have a counting allocator that ensures that everything that was allocated also was deallocated. That would make the checking explicit.

philnik updated this revision to Diff 477800.Thu, Nov 24, 8:11 AM
philnik marked 4 inline comments as done.
  • Address comments
  • Try to fix CI
libcxx/include/vector
297

Either way, I'll create additional patches to test the constructors of deque and string, since we also removed the base classes there. (BTW libstdc++ has a comment explaining the rationale for their base classes)

1068

We could just make the transaction cover the whole constructor, that should fix the problem. Since the fix should be easy enough, I think we should do it.

libcxx/test/std/containers/sequences/vector/vector.cons/throwing.pass.cpp
19 ↗(On Diff #477668)

ASan, but yes. An allocator doesn't really work, since we can't give it a proper state in all cases. I opted for checking new + delete calls.

In addition to this memory leak, it seems like we have a pre-existing bug that has been there for a long long time. In __construct_at_end (which is used in like 40 places), we fail to destroy the elements (in reverse order) in case one of the constructors throws. While it is not 100% clear to me that it's required in the constructors (I couldn't find the part of the spec that says so), I am pretty sure we're expected to do that. Let's fix this leak and then make a pass at our containers to see which ones suffer from this issue. I'm extremely surprised that we've been going for so long without this being noticed.

libcxx/include/vector
441–447

I would move the definition of the class here:

private:
  // add comment: Helper class to make it easier to rollback when we fail during one of vector's constructors
  struct __destroy_vector {
    operator() etc..
    vector& __vec;
  };

public:
  ~vector() { __vector_destroy()(*this); }
2503

We have the same problem here if the iterators throw inside std::copy -- we will end up leaking the memory allocated before the call to __construct_at_end.

libcxx/test/std/containers/sequences/vector/vector.cons/throwing.pass.cpp
1 ↗(On Diff #477800)

Maybe rename to /exceptions.pass.cpp?

philnik updated this revision to Diff 478403.Mon, Nov 28, 3:38 PM
philnik marked 6 inline comments as done.

Address comments