Skip to content

fix(libstore-tests): remove use-after-free bug for StringSource (backport #11813)#12190

Merged
mergify[bot] merged 1 commit into2.24-maintenancefrom
mergify/bp/2.24-maintenance/pr-11813
Jan 10, 2025
Merged

fix(libstore-tests): remove use-after-free bug for StringSource (backport #11813)#12190
mergify[bot] merged 1 commit into2.24-maintenancefrom
mergify/bp/2.24-maintenance/pr-11813

Conversation

@mergify
Copy link
Copy Markdown
Contributor

@mergify mergify bot commented Jan 10, 2025

Unfortunately StringSource class is very easy was very easy to misuse because the ctor took a plain std::string_view which has a bad habit of being implicitly convertible from an rvalue std::string. This lead to unintentional use-after-free bugs.

This patch makes StringSource much harder to misuse by disabling the ctor from a std::string && (but const std::string & is ok).

Fix affected tests from libstore-tests.
Reformat those tests with clangd's range formatting since the diff is tiny and it seems appropriate.

Motivation

Ran some tests under ASAN and UBSAN and got shocked by the amount of errors. This looks a good starting point for starting to untangle them.

Context

For reference here's the ASAN log for one of the two tests:

==3145773==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fffdbca8500 at pc 0x7ffff7993882 bp 0x7ffffffeffb0 sp 0x7ffffffef770
READ of size 1 at 0x7fffdbca8500 thread T0
    #0 0x7ffff7993881 in __interceptor_memcpy (/nix/store/40yjzm7r5ki59kkk9423dnwbm86x7pyd-gcc-13.2.0-lib/lib/libasan.so.8+0x71881)
    #1 0x7fffe3d752cb in std::char_traits<char>::copy(char*, char const*, unsigned long) /nix/store/fg7ass3a5m5pgl26qzfdniicbwbgzccy-gcc-13.2.0/include/c++/13.2.0/bits/char_traits.h:445
    #2 0x7fffe3d752cb in std::char_traits<char>::copy(char*, char const*, unsigned long) /nix/store/fg7ass3a5m5pgl26qzfdniicbwbgzccy-gcc-13.2.0/include/c++/13.2.0/bits/char_traits.h:437
    #3 0x7fffe3d752cb in std::basic_string_view<char, std::char_traits<char> >::copy(char*, unsigned long, unsigned long) const /nix/store/fg7ass3a5m5pgl26qzfdniicbwbgzccy-gcc-13.2.0/include/c++/13.2.0/string_view:327
    #4 0x7fffe3d752cb in nix::StringSource::read(char*, unsigned long) ../src/libutil/serialise.cc:192
    #5 0x7fffe3d59cbf in nix::Source::operator()(char*, unsigned long) ../src/libutil/serialise.cc:81
    #6 0x7fffeeab3596 in unsigned int nix::readNum<unsigned int>(nix::Source&) ../src/libutil/serialise.hh:415
    #7 0x7fffef710339 in nix::readInt(nix::Source&) ../src/libutil/serialise.hh:428
    #8 0x7fffef710339 in nix::WorkerProto::BasicClientConnection::handshake(nix::BufferedSink&, nix::Source&, unsigned int, std::set<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) ../src/libstore/worker-protocol-connection.cc:168
    #9 0xd538b8 in operator() ../src/libstore-tests/worker-protocol.cc:733
    #10 0xd538b8 in readTest<nix::WorkerProtoTest_handshake_client_truncated_replay_throws_Test::TestBody()::<lambda(std::string)> > ../src/libutil-test-support/tests/characterization.hh:61
    #11 0xd538b8 in nix::WorkerProtoTest_handshake_client_truncated_replay_throws_Test::TestBody() ../src/libstore-tests/worker-protocol.cc:725
    #12 0x7ffff788125c in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/nix/store/8br7fhn6qdrqm9f1acn35zadn0jrmlg9-gtest-1.14.0/lib/libgtest.so.1.14.0+0x6025c)
    #13 0x7ffff786806d in testing::Test::Run() (/nix/store/8br7fhn6qdrqm9f1acn35zadn0jrmlg9-gtest-1.14.0/lib/libgtest.so.1.14.0+0x4706d)
    #14 0x7ffff7868224 in testing::TestInfo::Run() (/nix/store/8br7fhn6qdrqm9f1acn35zadn0jrmlg9-gtest-1.14.0/lib/libgtest.so.1.14.0+0x47224)
    #15 0x7ffff7868456 in testing::TestSuite::Run() (/nix/store/8br7fhn6qdrqm9f1acn35zadn0jrmlg9-gtest-1.14.0/lib/libgtest.so.1.14.0+0x47456)
    #16 0x7ffff78779e6 in testing::internal::UnitTestImpl::RunAllTests() (/nix/store/8br7fhn6qdrqm9f1acn35zadn0jrmlg9-gtest-1.14.0/lib/libgtest.so.1.14.0+0x569e6)
    #17 0x7ffff786865a in testing::UnitTest::Run() (/nix/store/8br7fhn6qdrqm9f1acn35zadn0jrmlg9-gtest-1.14.0/lib/libgtest.so.1.14.0+0x4765a)
    #18 0x7ffff789e0bf in main (/nix/store/8br7fhn6qdrqm9f1acn35zadn0jrmlg9-gtest-1.14.0/lib/libgtest_main.so.1.14.0+0x10bf)
    #19 0x7fffdebe610d in __libc_start_call_main (/nix/store/87848rvrg5c7jmplpi0iapvbxyj9kfid-glibc-2.39-52/lib/libc.so.6+0x2a10d) (BuildId: 74e5f374333670d3fbe07e0abb58717eeababaa6)
    #20 0x7fffdebe61c8 in __libc_start_main_alias_1 (/nix/store/87848rvrg5c7jmplpi0iapvbxyj9kfid-glibc-2.39-52/lib/libc.so.6+0x2a1c8) (BuildId: 74e5f374333670d3fbe07e0abb58717eeababaa6)
    #21 0x4fedc4 in _start (nix/build/src/libstore-tests/nix-store-tests+0x4fedc4) (BuildId: 5ae976e9c97622accddce7010abbfe9dff93178b)

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

This is an automatic backport of pull request #11813 done by [Mergify](https://mergify.com).

@xokdvium @mergify
fix(libstore-tests): remove use-after-free bug for StringSource
e863e6a 
Unfortunately `StringSource` class is very easy was very easy to misuse
because the ctor took a plain `std::string_view` which has a bad habit
of being implicitly convertible from an rvalue `std::string`. This lead
to unintentional use-after-free bugs.

This patch makes `StringSource` much harder to misuse by disabling the ctor
from a `std::string &&` (but `const std::string &` is ok).

Fix affected tests from libstore-tests.
Reformat those tests with clangd's range formatting since the diff is tiny
and it seems appropriate.

(cherry picked from commit 5bc8957)
@mergify mergify bot requested a review from edolstra as a code owner January 10, 2025 10:08
@mergify mergify bot added the merge-queue label Jan 10, 2025
mergify bot added a commit that referenced this pull request Jan 10, 2025
@mergify
Merge of #12190
c4de384
@mergify mergify bot mentioned this pull request Jan 10, 2025
Closed
85 tasks
@mergify mergify bot merged commit 51e41ed into 2.24-maintenance Jan 10, 2025
@mergify mergify bot deleted the mergify/bp/2.24-maintenance/pr-11813 branch January 10, 2025 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees

No one assigned

Labels

merge-queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@xokdvium