diff-closures: fix a use after free #11187

Mic92 · 2024-07-25T19:15:32Z

Found by looking for interesting asan reports from the test suite.

What happened here is that name got overwritten, but it was what actually held the backing memory for the thing it got overwritten by, which was a by-reference value coming out of std::regex.

Due to absurd reasons I cannot seem to use a string_view iterator here, so I just copy the string with a longer lifetime instead. idk lol

==3796364==ERROR: AddressSanitizer: heap-use-after-free on address 0x503000014c61 at pc 0x74843523bf1d bp 0x7ffc68351330 sp 0x7ffc68350af0 READ of size 3 at 0x503000014c61 thread T0
0 0x74843523bf1c in __asan_memcpy (/nix/store/mzhqknx2mc94jdz4n320hn1lml86398y-clang-wrapper-17.0.6/resource-root/lib/linux/libclang_rt.asan-x86_64.so+0x159f1c)
1 0x6403cf6cbff4 in std::char_traits::copy(char*, char const*, unsigned long) /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/char_traits.h:445:33
<...>
7 0x6403cf6cbff4 in std::__cxx11::sub_match<__gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits, std::allocator>>>::str() const /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/regex.h:966:6
8 0x6403cf6cbff4 in std::__cxx11::sub_match<__gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits, std::allocator>>>::operator std::__cxx11::basic_string<char, std::char_traits, std::allocator>() const /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/regex.h:955:16
9 0x6403cf6cbff4 in nix::getClosureInfo[abi:cxx11](nix::refnix::Store, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:37:26
10 0x6403cf6cd70c in nix::printClosureDiff(nix::refnix::Store, nix::StorePath const&, nix::StorePath const&, std::basic_string_view<char, std::char_traits>) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:54:25
11 0x6403cf873331 in CmdProfileDiffClosures::run(nix::refnix::Store) /home/jade/lix/lix2/build/src/nix/profile.cc:479:17
<...>

0x503000014c61 is located 17 bytes inside of 21-byte region [0x503000014c50,0x503000014c65) freed by thread T0 here:
0 0x748435250470 in operator delete(void*) (/nix/store/mzhqknx2mc94jdz4n320hn1lml86398y-clang-wrapper-17.0.6/resource-root/lib/linux/libclang_rt.asan-x86_64.so+0x16e470)
<...>
6 0x6403cf6cbda2 in std::__cxx11::basic_string<char, std::char_traits, std::allocator>::~basic_string() /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/basic_string.h:792:9
7 0x6403cf6cbda2 in nix::getClosureInfo[abi:cxx11](nix::refnix::Store, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:36:13
8 0x6403cf6cd70c in nix::printClosureDiff(nix::refnix::Store, nix::StorePath const&, nix::StorePath const&, std::basic_string_view<char, std::char_traits>) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:54:25
<...>

previously allocated by thread T0 here:
0 0x74843524fa38 in operator new(unsigned long) (/nix/store/mzhqknx2mc94jdz4n320hn1lml86398y-clang-wrapper-17.0.6/resource-root/lib/linux/libclang_rt.asan-x86_64.so+0x16da38)
<...>
9 0x6403cf6cb68c in std::__cxx11::basic_string<char, std::char_traits, std::allocator>::basic_string<std::basic_string_view<char, std::char_traits>, void>(std::basic_string_view<char, std::char_traits> const&, std::allocator const&) /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/basic_string.h:784:4
10 0x6403cf6cb68c in nix::getClosureInfo[abi:cxx11](nix::refnix::Store, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:33:21
11 0x6403cf6cd70c in nix::printClosureDiff(nix::refnix::Store, nix::StorePath const&, nix::StorePath const&, std::basic_string_view<char, std::char_traits>) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:54:25
12 0x6403cf873331 in CmdProfileDiffClosures::run(nix::refnix::Store) /home/jade/lix/lix2/build/src/nix/profile.cc:479:17
<...>

(cherry-picked from https://git.lix.systems/lix-project/lix/commit/b9b1bbd22fc3b34a4d8e2090f0d033f742c32ee0)

depends on #11186

Motivation

Context

Priorities and Process

Add 👍 to pull requests you find important.

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

Arose because NixOS#9014 merged before NixOS#11131, but the latter did not rebase / merge against the latest master.

Found by looking for interesting asan reports from the test suite. What happened here is that name got overwritten, but it was what actually held the backing memory for the thing it got overwritten by, which was a by-reference value coming out of std::regex. Due to absurd reasons I cannot seem to use a string_view iterator here, so I just copy the string with a longer lifetime instead. idk lol ==3796364==ERROR: AddressSanitizer: heap-use-after-free on address 0x503000014c61 at pc 0x74843523bf1d bp 0x7ffc68351330 sp 0x7ffc68350af0 READ of size 3 at 0x503000014c61 thread T0 0 0x74843523bf1c in __asan_memcpy (/nix/store/mzhqknx2mc94jdz4n320hn1lml86398y-clang-wrapper-17.0.6/resource-root/lib/linux/libclang_rt.asan-x86_64.so+0x159f1c) 1 0x6403cf6cbff4 in std::char_traits<char>::copy(char*, char const*, unsigned long) /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/char_traits.h:445:33 <...> 7 0x6403cf6cbff4 in std::__cxx11::sub_match<__gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>::str() const /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/regex.h:966:6 8 0x6403cf6cbff4 in std::__cxx11::sub_match<__gnu_cxx::__normal_iterator<char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>::operator std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>() const /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/regex.h:955:16 9 0x6403cf6cbff4 in nix::getClosureInfo[abi:cxx11](nix::ref<nix::Store>, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:37:26 10 0x6403cf6cd70c in nix::printClosureDiff(nix::ref<nix::Store>, nix::StorePath const&, nix::StorePath const&, std::basic_string_view<char, std::char_traits<char>>) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:54:25 11 0x6403cf873331 in CmdProfileDiffClosures::run(nix::ref<nix::Store>) /home/jade/lix/lix2/build/src/nix/profile.cc:479:17 <...> 0x503000014c61 is located 17 bytes inside of 21-byte region [0x503000014c50,0x503000014c65) freed by thread T0 here: 0 0x748435250470 in operator delete(void*) (/nix/store/mzhqknx2mc94jdz4n320hn1lml86398y-clang-wrapper-17.0.6/resource-root/lib/linux/libclang_rt.asan-x86_64.so+0x16e470) <...> 6 0x6403cf6cbda2 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::~basic_string() /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/basic_string.h:792:9 7 0x6403cf6cbda2 in nix::getClosureInfo[abi:cxx11](nix::ref<nix::Store>, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:36:13 8 0x6403cf6cd70c in nix::printClosureDiff(nix::ref<nix::Store>, nix::StorePath const&, nix::StorePath const&, std::basic_string_view<char, std::char_traits<char>>) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:54:25 <...> previously allocated by thread T0 here: 0 0x74843524fa38 in operator new(unsigned long) (/nix/store/mzhqknx2mc94jdz4n320hn1lml86398y-clang-wrapper-17.0.6/resource-root/lib/linux/libclang_rt.asan-x86_64.so+0x16da38) <...> 9 0x6403cf6cb68c in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::basic_string<std::basic_string_view<char, std::char_traits<char>>, void>(std::basic_string_view<char, std::char_traits<char>> const&, std::allocator<char> const&) /nix/store/14c6s4xzhy14i2b05s00rjns2j93gzz4-gcc-13.2.0/include/c++/13.2.0/bits/basic_string.h:784:4 10 0x6403cf6cb68c in nix::getClosureInfo[abi:cxx11](nix::ref<nix::Store>, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:33:21 11 0x6403cf6cd70c in nix::printClosureDiff(nix::ref<nix::Store>, nix::StorePath const&, nix::StorePath const&, std::basic_string_view<char, std::char_traits<char>>) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:54:25 12 0x6403cf873331 in CmdProfileDiffClosures::run(nix::ref<nix::Store>) /home/jade/lix/lix2/build/src/nix/profile.cc:479:17 <...> (cherry-picked from https://git.lix.systems/lix-project/lix/commit/b9b1bbd22fc3b34a4d8e2090f0d033f742c32ee0)

This was done originally because std::smatch does not accept `const char *` as iterators. However, this was because we should have been using std::cmatch instead. (cherry picked from commit https://git.lix.systems/lix-project/lix/commit/12a5838d11f790d36e2ac626e8916a2c19bcb80b)

dpulls · 2024-07-25T19:47:20Z

🎉 All dependencies have been resolved !

roberth · 2024-07-26T22:52:07Z

src/nix/diff-closures.cc

        /* Strip the output name. Unfortunately this is ambiguous (we
           can't distinguish between output names like "bin" and
           version suffixes like "unstable"). */
        static std::regex regex("(.*)-([a-z]+|lib32|lib64)");
-        std::smatch match;
-        std::string name(path.name());
+        std::cmatch match;


smatch might have been nice, but I don't see a way to make that work either. The std::regex does not seem to support it when the input is not an actual std::string. Unfortunate.

roberth · 2024-07-26T22:52:53Z

Thanks!

Mic92 requested a review from edolstra as a code owner July 25, 2024 19:15

github-actions bot added the new-cli Relating to the "nix" command label Jul 25, 2024

Mic92 marked this pull request as draft July 25, 2024 19:38

cole-h and others added 3 commits July 25, 2024 21:41

Fix reference to experimental features docs

90459e6

Arose because NixOS#9014 merged before NixOS#11131, but the latter did not rebase / merge against the latest master.

Mic92 force-pushed the diff-closure-fix branch from 3ebc194 to 07aeedd Compare July 25, 2024 19:41

github-actions bot added the documentation label Jul 25, 2024

Mic92 marked this pull request as ready for review July 25, 2024 19:42

Mic92 requested a review from fricklerhandwerk as a code owner July 25, 2024 19:42

roberth reviewed Jul 26, 2024

View reviewed changes

roberth approved these changes Jul 26, 2024

View reviewed changes

roberth merged commit 88e8c90 into NixOS:master Jul 26, 2024
13 checks passed

Mic92 deleted the diff-closure-fix branch July 27, 2024 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diff-closures: fix a use after free #11187

diff-closures: fix a use after free #11187

Mic92 commented Jul 25, 2024 •

edited

Loading

dpulls bot commented Jul 25, 2024

roberth Jul 26, 2024

roberth commented Jul 26, 2024

diff-closures: fix a use after free #11187

diff-closures: fix a use after free #11187

Conversation

Mic92 commented Jul 25, 2024 • edited Loading

Motivation

Context

Priorities and Process

dpulls bot commented Jul 25, 2024

roberth Jul 26, 2024

Choose a reason for hiding this comment

roberth commented Jul 26, 2024

Mic92 commented Jul 25, 2024 •

edited

Loading