Skip to content

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

Merged
Mic92 merged 1 commit intoNixOS:masterfrom
xokdvium:dev/fix-use-after-free-libstore-tests
Nov 6, 2024
Merged

fix(libstore-tests): remove use-after-free bug for StringSource#11813
Mic92 merged 1 commit intoNixOS:masterfrom
xokdvium:dev/fix-use-after-free-libstore-tests

Conversation

@xokdvium
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium commented Nov 5, 2024

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.

@xokdvium
fix(libstore-tests): remove use-after-free bug for StringSource
5bc8957 
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.
@xokdvium xokdvium requested a review from edolstra as a code owner November 5, 2024 23:32
@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Nov 6, 2024

@mergify queue

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Nov 6, 2024 •
edited
Loading

queue

🛑 The pull request has been merged manually

Details

The pull request has been merged manually at 2ef5e22

@Mic92 Mic92 merged commit 2ef5e22 into NixOS:master Nov 6, 2024
@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Nov 6, 2024 •
edited
Loading

Now that we have proper meson support enabling asan/ubsan should be much easier.
And we should also run one unittest build with it enabled, so that we can catch all those nasty bugs.
I think asan is not compatible with boehmgc, so we have to disable it in those instances.
@xokdvium so keep those fixes coming.

@xokdvium
Copy link
Copy Markdown
Contributor Author

xokdvium commented Nov 6, 2024

@Mic92 There's already something akin to a tracking issue for sanitizers: #10969.

In the meantime I'll try to compile the list of problems uncovered with sanitizers (and there are countless). Almost everything either leaks, has UB or commits memory crimes.

Maybe a good approach would to be first enable only UBSAN and tackle everything it has to say. Addressing memory leaks and memory crimes would be a huge undertaking that deserves a tracking issue of its own.

@xokdvium xokdvium deleted the dev/fix-use-after-free-libstore-tests branch November 6, 2024 06:27
@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Nov 6, 2024

@xokdvium ok. Maybe add a list of errors you got to it and I might also work on some of them.

@xokdvium xokdvium mentioned this pull request Nov 8, 2024
Closed
@mergify mergify bot mentioned this pull request Jan 10, 2025
Merged
mergify bot added a commit that referenced this pull request Jan 10, 2025
@mergify
Merge pull request #12190 from NixOS/mergify/bp/2.24-maintenance/pr-1…
51e41ed 
…1813

fix(libstore-tests): remove use-after-free bug for `StringSource` (backport #11813)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mic92 Mic92 Mic92 approved these changes

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xokdvium @Mic92 @roberth