[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Sorry for the delay. Yes, I meant the `--vg` flag which is I think the only 
valid way to test this from your description. That it can't be 
deterministically tested without this (or in configurations that don't run 
valgrind) is fine IMHO.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Gabor Marton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2e90fc2c407b: [AST][PCH][ASTImporter] Fix UB caused by 
uninited SwitchStmt member (authored by martong).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

Files:
  clang/include/clang/AST/Stmt.h
  clang/test/Analysis/Inputs/ctu-other.c
  clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
  clang/test/Analysis/ctu-main.c


Index: clang/test/Analysis/ctu-main.c
===
--- clang/test/Analysis/ctu-main.c
+++ clang/test/Analysis/ctu-main.c
@@ -69,3 +69,8 @@
   d.b = 0;
   clang_analyzer_eval(structInProto() == 0); // expected-warning{{TRUE}} 
expected-warning{{FALSE}}
 }
+
+int switchWithoutCases(int);
+void testSwitchStmtCrash(int x) {
+  switchWithoutCases(x);
+}
Index: clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
===
--- clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
+++ clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
@@ -4,3 +4,4 @@
 c:@F@enumCheck ctu-other.c.ast
 c:@F@identImplicit ctu-other.c.ast
 c:@F@structInProto ctu-other.c.ast
+c:@F@switchWithoutCases ctu-other.c.ast
Index: clang/test/Analysis/Inputs/ctu-other.c
===
--- clang/test/Analysis/Inputs/ctu-other.c
+++ clang/test/Analysis/Inputs/ctu-other.c
@@ -49,3 +49,9 @@
 int structInProto(struct DataType {int a;int b; } * d) {
   return 0;
 }
+
+int switchWithoutCases(int x) {
+  switch (x) {
+  };
+  return 0;
+}
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -2119,7 +2119,7 @@
   friend TrailingObjects;
 
   /// Points to a linked list of case and default statements.
-  SwitchCase *FirstCase;
+  SwitchCase *FirstCase = nullptr;
 
   // SwitchStmt is followed by several trailing objects,
   // some of which optional. Note that it would be more convenient to


Index: clang/test/Analysis/ctu-main.c
===
--- clang/test/Analysis/ctu-main.c
+++ clang/test/Analysis/ctu-main.c
@@ -69,3 +69,8 @@
   d.b = 0;
   clang_analyzer_eval(structInProto() == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
 }
+
+int switchWithoutCases(int);
+void testSwitchStmtCrash(int x) {
+  switchWithoutCases(x);
+}
Index: clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
===
--- clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
+++ clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
@@ -4,3 +4,4 @@
 c:@F@enumCheck ctu-other.c.ast
 c:@F@identImplicit ctu-other.c.ast
 c:@F@structInProto ctu-other.c.ast
+c:@F@switchWithoutCases ctu-other.c.ast
Index: clang/test/Analysis/Inputs/ctu-other.c
===
--- clang/test/Analysis/Inputs/ctu-other.c
+++ clang/test/Analysis/Inputs/ctu-other.c
@@ -49,3 +49,9 @@
 int structInProto(struct DataType {int a;int b; } * d) {
   return 0;
 }
+
+int switchWithoutCases(int x) {
+  switch (x) {
+  };
+  return 0;
+}
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -2119,7 +2119,7 @@
   friend TrailingObjects;
 
   /// Points to a linked list of case and default statements.
-  SwitchCase *FirstCase;
+  SwitchCase *FirstCase = nullptr;
 
   // SwitchStmt is followed by several trailing objects,
   // some of which optional. Note that it would be more convenient to
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 328149.
martong added a comment.
Herald added a reviewer: Szelethus.

Add a test case which fails if lit --vg is used.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

Files:
  clang/include/clang/AST/Stmt.h
  clang/test/Analysis/Inputs/ctu-other.c
  clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
  clang/test/Analysis/ctu-main.c


Index: clang/test/Analysis/ctu-main.c
===
--- clang/test/Analysis/ctu-main.c
+++ clang/test/Analysis/ctu-main.c
@@ -69,3 +69,8 @@
   d.b = 0;
   clang_analyzer_eval(structInProto() == 0); // expected-warning{{TRUE}} 
expected-warning{{FALSE}}
 }
+
+int switchWithoutCases(int);
+void testSwitchStmtCrash(int x) {
+  switchWithoutCases(x);
+}
Index: clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
===
--- clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
+++ clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
@@ -4,3 +4,4 @@
 c:@F@enumCheck ctu-other.c.ast
 c:@F@identImplicit ctu-other.c.ast
 c:@F@structInProto ctu-other.c.ast
+c:@F@switchWithoutCases ctu-other.c.ast
Index: clang/test/Analysis/Inputs/ctu-other.c
===
--- clang/test/Analysis/Inputs/ctu-other.c
+++ clang/test/Analysis/Inputs/ctu-other.c
@@ -49,3 +49,9 @@
 int structInProto(struct DataType {int a;int b; } * d) {
   return 0;
 }
+
+int switchWithoutCases(int x) {
+  switch (x) {
+  };
+  return 0;
+}
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -2119,7 +2119,7 @@
   friend TrailingObjects;
 
   /// Points to a linked list of case and default statements.
-  SwitchCase *FirstCase;
+  SwitchCase *FirstCase = nullptr;
 
   // SwitchStmt is followed by several trailing objects,
   // some of which optional. Note that it would be more convenient to


Index: clang/test/Analysis/ctu-main.c
===
--- clang/test/Analysis/ctu-main.c
+++ clang/test/Analysis/ctu-main.c
@@ -69,3 +69,8 @@
   d.b = 0;
   clang_analyzer_eval(structInProto() == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
 }
+
+int switchWithoutCases(int);
+void testSwitchStmtCrash(int x) {
+  switchWithoutCases(x);
+}
Index: clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
===
--- clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
+++ clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
@@ -4,3 +4,4 @@
 c:@F@enumCheck ctu-other.c.ast
 c:@F@identImplicit ctu-other.c.ast
 c:@F@structInProto ctu-other.c.ast
+c:@F@switchWithoutCases ctu-other.c.ast
Index: clang/test/Analysis/Inputs/ctu-other.c
===
--- clang/test/Analysis/Inputs/ctu-other.c
+++ clang/test/Analysis/Inputs/ctu-other.c
@@ -49,3 +49,9 @@
 int structInProto(struct DataType {int a;int b; } * d) {
   return 0;
 }
+
+int switchWithoutCases(int x) {
+  switch (x) {
+  };
+  return 0;
+}
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -2119,7 +2119,7 @@
   friend TrailingObjects;
 
   /// Points to a linked list of case and default statements.
-  SwitchCase *FirstCase;
+  SwitchCase *FirstCase = nullptr;
 
   // SwitchStmt is followed by several trailing objects,
   // some of which optional. Note that it would be more convenient to
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

I don't think we have any tests that explicitly use valgrind and testing with 
valgrind enabled is an optional thing. I would be fine landing this as-is 
without a test case if it turns out that testing this is too involved, so I'm 
accepting the review. However, if it turns out there is a way to test it that 
others can think of, I would appreciate adding the test coverage (even if it's 
post-commit).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D97849#2600300 , @teemperor wrote:

> We can (and do) run LIT tests under valgrind, so if you have a test that 
> triggers this valgrind error then that seems like the way to go. I don't 
> think the test has to deterministically fail outside valgrind to be a valid 
> test.

Do you mean to use lit with `--vg` or should we explicitly call valgrind in a 
lit test? If the latter, could you please show an example how to do that 
properly (I could not find any explicit valgrind call, not even in lldb) ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D97849#2600220 , @steakhal wrote:

> In D97849#2600201 , @aaron.ballman 
> wrote:
>
>> This looks reasonable to me (good catch!), but is there a way for us to add 
>> a regression test for it?
>
> I'm not sure if it's possible to write a test for deterministically 
> demonstrating the bug - which is a non-deterministic crash.
> So, even if we would have a test case, that would not catch the regression 
> deterministically.
> We could include the minimal reproducer for CTU analysis - the way we 
> observed and tracked down this crash.
>
> AFAIK, it did not reproduce with memory sanitizers.

Address and ubsan could not catch it (LLVM_USE_SANITIZER: Address;Undefined). 
And I had trouble with the Memory and MemoryWithOrigins setup because ninja 
could not generate the .inc files since clang-tblgen failed always with 
uninited errors. Fortunately, vlagrind could catch the error consistently all 
the time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

We can (and do) run LIT tests under valgrind, so if you have a test that 
triggers this valgrind error then that seems like the way to go. I don't think 
the test has to deterministically fail outside valgrind to be a valid test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D97849#2600201 , @aaron.ballman 
wrote:

> This looks reasonable to me (good catch!), but is there a way for us to add a 
> regression test for it?

I'm not sure if it's possible to write a test for deterministically 
demonstrating the bug - which is a non-deterministic crash.
So, even if we would have a test case, that would not catch the regression 
deterministically.
We could include the minimal reproducer for CTU analysis - the way we observed 
and tracked down this crash.

AFAIK, it did not reproduce with memory sanitizers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This looks reasonable to me (good catch!), but is there a way for us to add a 
regression test for it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

F15719554: valgrind-output.txt 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97849/new/

https://reviews.llvm.org/D97849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: riccibruno, aaron.ballman, shafik, teemperor, rsmith.
Herald added subscribers: gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
martong requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The SwitchStmt::FirstCase member is not initialized when the AST is
built by the ASTStmtReader. See the below code of
ASTStmtReader::VisitSwitchStmt in the case where the for loop does not
have any iterations:

// ... more code ...
SwitchCase *PrevSC = nullptr;
for (auto E = Record.size(); Record.getIdx() != E; ) {
  SwitchCase *SC = Record.getSwitchCaseWithID(Record.readInt());
  if (PrevSC)
PrevSC->setNextSwitchCase(SC);
  else
S->setSwitchCaseList(SC); // Sets FirstCase !!!
  
  PrevSC = SC;
}
  } // return

Later, in ASTNodeImporter::VisitSwitchStmt,
we have a condition that depends on this uninited value:

  for (SwitchCase *SC = S->getSwitchCaseList(); SC != nullptr;
   SC = SC->getNextSwitchCase()) {
   // ... more code ...
  }

This is clearly an UB. This causes non-deterministic crashes when
ClangSA analyzes some code with CTU. See the below report by valgrind
(the whole valgrind output is attached):

  ==31019== Conditional jump or move depends on uninitialised value(s)
  ==31019==at 0x12ED1983: 
clang::ASTNodeImporter::VisitSwitchStmt(clang::SwitchStmt*) 
(ASTImporter.cpp:6195)
  ==31019==by 0x12F1D509: clang::StmtVisitorBase>::Visit(clang::Stmt*) 
(StmtNodes.inc:591)
  ==31019==by 0x12EE4FDF: clang::ASTImporter::Import(clang::Stmt*) 
(ASTImporter.cpp:8484)
  ==31019==by 0x12F09498: llvm::Expected 
clang::ASTNodeImporter::import(clang::Stmt*) (ASTImporter.cpp:164)
  ==31019==by 0x12F3A1F5: llvm::Error 
clang::ASTNodeImporter::ImportArrayChecked(clang::Stmt**, clang::Stmt**, clang::Stmt**) 
(ASTImporter.cpp:653)
  ==31019==by 0x12F13152: llvm::Error 
clang::ASTNodeImporter::ImportContainerChecked,
 llvm::SmallVector >(llvm::iterator_range 
const&, llvm::SmallVector&) (ASTImporter.cpp:669)
  ==31019==by 0x12ED099F: 
clang::ASTNodeImporter::VisitCompoundStmt(clang::CompoundStmt*) 
(ASTImporter.cpp:6077)
  ==31019==by 0x12F1CC2D: clang::StmtVisitorBase>::Visit(clang::Stmt*) 
(StmtNodes.inc:73)
  ==31019==by 0x12EE4FDF: clang::ASTImporter::Import(clang::Stmt*) 
(ASTImporter.cpp:8484)
  ==31019==by 0x12F09498: llvm::Expected 
clang::ASTNodeImporter::import(clang::Stmt*) (ASTImporter.cpp:164)
  ==31019==by 0x12F13275: clang::Stmt* 
clang::ASTNodeImporter::importChecked(llvm::Error&, clang::Stmt* 
const&) (ASTImporter.cpp:197)
  ==31019==by 0x12ED0CE6: 
clang::ASTNodeImporter::VisitCaseStmt(clang::CaseStmt*) (ASTImporter.cpp:6098)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97849

Files:
  clang/include/clang/AST/Stmt.h


Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -2119,7 +2119,7 @@
   friend TrailingObjects;
 
   /// Points to a linked list of case and default statements.
-  SwitchCase *FirstCase;
+  SwitchCase *FirstCase = nullptr;
 
   // SwitchStmt is followed by several trailing objects,
   // some of which optional. Note that it would be more convenient to


Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -2119,7 +2119,7 @@
   friend TrailingObjects;
 
   /// Points to a linked list of case and default statements.
-  SwitchCase *FirstCase;
+  SwitchCase *FirstCase = nullptr;
 
   // SwitchStmt is followed by several trailing objects,
   // some of which optional. Note that it would be more convenient to
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits