This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.
Closed by commit rG60e19f6752b7: [clangd] Fix use-after-free in
HeaderIncluderCache (authored by kadircet).
Changed prior to commit:
https://reviews.llvm.org/D112130?vs=380895&id=380951#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112130/new/
https://reviews.llvm.org/D112130
Files:
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1200,10 +1200,10 @@
Unreliable = testPath("unreliable.h"),
OK = testPath("ok.h"),
NotIncluded = testPath("not_included.h");
- class NoHeadersCDB : public GlobalCompilationDatabase {
+ struct NoHeadersCDB : public GlobalCompilationDatabase {
llvm::Optional<tooling::CompileCommand>
getCompileCommand(PathRef File) const override {
- if (File == NoCmd || File == NotIncluded)
+ if (File == NoCmd || File == NotIncluded || FailAll)
return llvm::None;
auto Basic = getFallbackCommand(File);
Basic.Heuristic.clear();
@@ -1218,6 +1218,8 @@
}
return Basic;
}
+
+ std::atomic<bool> FailAll{false};
} CDB;
TUScheduler S(CDB, optsForTest());
auto GetFlags = [&](PathRef Header) {
@@ -1288,6 +1290,21 @@
<< "association invalidated but not reclaimed";
EXPECT_THAT(GetFlags(NotIncluded), Contains("-DMAIN2"))
<< "association still valid";
+
+ // Delete the file from CDB, it should invalidate the associations.
+ CDB.FailAll = true;
+ EXPECT_THAT(GetFlags(NoCmd), Not(Contains("-DMAIN3")))
+ << "association should've been invalidated.";
+ // Also run update for Main3 to invalidate the preeamble to make sure next
+ // update populates include cache associations.
+ S.update(Main3, getInputs(Main3, SomeIncludes), WantDiagnostics::Yes);
+ EXPECT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ // Re-add the file and make sure nothing crashes.
+ CDB.FailAll = false;
+ S.update(Main3, getInputs(Main3, SomeIncludes), WantDiagnostics::Yes);
+ EXPECT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ EXPECT_THAT(GetFlags(NoCmd), Contains("-DMAIN3"))
+ << "association invalidated and then claimed by main3";
}
TEST_F(TUSchedulerTests, PreservesLastActiveFile) {
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -286,8 +286,12 @@
void remove(PathRef MainFile) {
std::lock_guard<std::mutex> Lock(Mu);
Association *&First = MainToFirst[MainFile];
- if (First)
+ if (First) {
invalidate(First);
+ First = nullptr;
+ }
+ // MainToFirst entry should stay alive, as Associations might be pointing
at
+ // its key.
}
/// Get the mainfile associated with Header, or the empty string if none.
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1200,10 +1200,10 @@
Unreliable = testPath("unreliable.h"),
OK = testPath("ok.h"),
NotIncluded = testPath("not_included.h");
- class NoHeadersCDB : public GlobalCompilationDatabase {
+ struct NoHeadersCDB : public GlobalCompilationDatabase {
llvm::Optional<tooling::CompileCommand>
getCompileCommand(PathRef File) const override {
- if (File == NoCmd || File == NotIncluded)
+ if (File == NoCmd || File == NotIncluded || FailAll)
return llvm::None;
auto Basic = getFallbackCommand(File);
Basic.Heuristic.clear();
@@ -1218,6 +1218,8 @@
}
return Basic;
}
+
+ std::atomic<bool> FailAll{false};
} CDB;
TUScheduler S(CDB, optsForTest());
auto GetFlags = [&](PathRef Header) {
@@ -1288,6 +1290,21 @@
<< "association invalidated but not reclaimed";
EXPECT_THAT(GetFlags(NotIncluded), Contains("-DMAIN2"))
<< "association still valid";
+
+ // Delete the file from CDB, it should invalidate the associations.
+ CDB.FailAll = true;
+ EXPECT_THAT(GetFlags(NoCmd), Not(Contains("-DMAIN3")))
+ << "association should've been invalidated.";
+ // Also run update for Main3 to invalidate the preeamble to make sure next
+ // update populates include cache associations.
+ S.update(Main3, getInputs(Main3, SomeIncludes), WantDiagnostics::Yes);
+ EXPECT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ // Re-add the file and make sure nothing crashes.
+ CDB.FailAll = false;
+ S.update(Main3, getInputs(Main3, SomeIncludes), WantDiagnostics::Yes);
+ EXPECT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ EXPECT_THAT(GetFlags(NoCmd), Contains("-DMAIN3"))
+ << "association invalidated and then claimed by main3";
}
TEST_F(TUSchedulerTests, PreservesLastActiveFile) {
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -286,8 +286,12 @@
void remove(PathRef MainFile) {
std::lock_guard<std::mutex> Lock(Mu);
Association *&First = MainToFirst[MainFile];
- if (First)
+ if (First) {
invalidate(First);
+ First = nullptr;
+ }
+ // MainToFirst entry should stay alive, as Associations might be pointing at
+ // its key.
}
/// Get the mainfile associated with Header, or the empty string if none.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
