llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.11k stars 11.61k forks source link

[analyzer] Dereference checker should check binds on label locations #89264

Closed steakhal closed 4 months ago

steakhal commented 5 months ago

Consider the following input:

// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s

void clang_analyzer_dump(char);
void clang_analyzer_dump_ptr(char*);

// https://github.com/llvm/llvm-project/issues/89185
void binding_to_label_loc() {
  char *b = &&MyLabel;
MyLabel:
  *b = 0; // no-crash
  clang_analyzer_dump_ptr(b); // expected-warning {{&&MyLabel}}
  clang_analyzer_dump(*b); // expected-warning {{Unknown}}
  // FIXME: We should never get here, as the dereference checker should have caught the binding operation to the label location.
}

One should never store values to the addresses of labels. I think the closest checker is the Dereference checker, which could check for label locs after it checked for undefined locations. Like this:

diff --git a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -287,6 +287,12 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
   if (V.isUndef())
     return;

+  // One should never write to label addresses.
+  if (auto Label = L.getAs<loc::GotoLabel>()) {
+    llvm::errs() << "WRITING TO LABEL: " << L << "\n";
+    // TODO: report a fatal issue like: "Dereference of the address of a label"
+  }
+
   const MemRegion *MR = L.getAsRegion();
   const TypedValueRegion *TVR = dyn_cast_or_null<TypedValueRegion>(MR);
   if (!TVR)
llvmbot commented 5 months ago

@llvm/issue-subscribers-clang-static-analyzer

Author: Balazs Benics (steakhal)

Consider the following input: ```c++ // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s void clang_analyzer_dump(char); void clang_analyzer_dump_ptr(char*); // https://github.com/llvm/llvm-project/issues/89185 void binding_to_label_loc() { char *b = &&MyLabel; MyLabel: *b = 0; // no-crash clang_analyzer_dump_ptr(b); // expected-warning {{&&MyLabel}} clang_analyzer_dump(*b); // expected-warning {{Unknown}} // FIXME: We should never get here, as the dereference checker should have caught the binding operation to the label location. } ``` One should never store values to the addresses of labels. I think the closest checker is the Dereference checker, which could check for label locs after it checked for undefined locations. Like this: ```diff diff --git a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp @@ -287,6 +287,12 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S, if (V.isUndef()) return; + // One should never write to label addresses. + if (auto Label = L.getAs<loc::GotoLabel>()) { + llvm::errs() << "WRITING TO LABEL: " << L << "\n"; + // TODO: report a fatal issue like: "Dereference of the address of a label" + } + const MemRegion *MR = L.getAsRegion(); const TypedValueRegion *TVR = dyn_cast_or_null<TypedValueRegion>(MR); if (!TVR) ```
llvmbot commented 5 months ago

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot commented 5 months ago

@llvm/issue-subscribers-good-first-issue

Author: Balazs Benics (steakhal)

Consider the following input: ```c++ // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s void clang_analyzer_dump(char); void clang_analyzer_dump_ptr(char*); // https://github.com/llvm/llvm-project/issues/89185 void binding_to_label_loc() { char *b = &&MyLabel; MyLabel: *b = 0; // no-crash clang_analyzer_dump_ptr(b); // expected-warning {{&&MyLabel}} clang_analyzer_dump(*b); // expected-warning {{Unknown}} // FIXME: We should never get here, as the dereference checker should have caught the binding operation to the label location. } ``` One should never store values to the addresses of labels. I think the closest checker is the Dereference checker, which could check for label locs after it checked for undefined locations. Like this: ```diff diff --git a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp @@ -287,6 +287,12 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S, if (V.isUndef()) return; + // One should never write to label addresses. + if (auto Label = L.getAs<loc::GotoLabel>()) { + llvm::errs() << "WRITING TO LABEL: " << L << "\n"; + // TODO: report a fatal issue like: "Dereference of the address of a label" + } + const MemRegion *MR = L.getAsRegion(); const TypedValueRegion *TVR = dyn_cast_or_null<TypedValueRegion>(MR); if (!TVR) ```
steakhal commented 5 months ago

Split from #89185

saichatla commented 5 months ago

Can I take this issue?

steakhal commented 5 months ago

Can I take this issue?

You can work on it, and if anyone else also find this interesting, then they can also work on it. I don't mind.