From b9b1bbd22fc3b34a4d8e2090f0d033f742c32ee0 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Tue, 18 Jun 2024 00:54:05 -0700 Subject: [PATCH] diff-closures: fix a use after free 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, 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, std::allocator>>>::operator std::__cxx11::basic_string, 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::ref, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:37:26 10 0x6403cf6cd70c in nix::printClosureDiff(nix::ref, nix::StorePath const&, nix::StorePath const&, std::basic_string_view>) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:54:25 11 0x6403cf873331 in CmdProfileDiffClosures::run(nix::ref) /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, 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::ref, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:36:13 8 0x6403cf6cd70c in nix::printClosureDiff(nix::ref, nix::StorePath const&, nix::StorePath const&, std::basic_string_view>) /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, std::allocator>::basic_string>, void>(std::basic_string_view> 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::ref, nix::StorePath const&) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:33:21 11 0x6403cf6cd70c in nix::printClosureDiff(nix::ref, nix::StorePath const&, nix::StorePath const&, std::basic_string_view>) /home/jade/lix/lix2/build/src/nix/diff-closures.cc:54:25 12 0x6403cf873331 in CmdProfileDiffClosures::run(nix::ref) /home/jade/lix/lix2/build/src/nix/profile.cc:479:17 <...> Change-Id: I9c408cf2a3d3155f9f7b2ad4848ee6c741331db0 --- src/nix/diff-closures.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/nix/diff-closures.cc b/src/nix/diff-closures.cc index 736cbf55d..5eda309d5 100644 --- a/src/nix/diff-closures.cc +++ b/src/nix/diff-closures.cc @@ -30,9 +30,18 @@ GroupedPaths getClosureInfo(ref store, const StorePath & toplevel) version suffixes like "unstable"). */ static std::regex regex("(.*)-([a-z]+|lib32|lib64)"); std::smatch match; - std::string name(path.name()); + std::string name{path.name()}; + // Used to keep name alive through being potentially overwritten below + // (to not invalidate the references from the regex result) + // + // n.b. cannot be just path.name().{begin,end}() since that returns const + // char *, which does not, for some reason, convert as required on + // libstdc++. Seems like a libstdc++ bug or standard bug to me... we + // can afford the allocation in any case. + const std::string origName{path.name()}; std::string outputName; - if (std::regex_match(name, match, regex)) { + + if (std::regex_match(origName, match, regex)) { name = match[1]; outputName = match[2]; }