[analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[zukatsinadze]

Summary

The patch adds two new checkers.
core.StoreToImmutable that warns binds on immutable memory and
alpha.security.cert.env.ModelConstQualifiedReturn that models return values
of some functions that should be considered const qualified. Based on SEI CERT
ENV30-C: https://wiki.sei.cmu.edu/confluence/x/79UxBQ

[zukatsinadze]

fix clang format

[steakhal]

Looks great. I only have nits mostly for the docs.

clang/docs/analyzer/checkers.rst

2384–2386

IMO the list of functions should be mentioned separately. What if the list gets longer and longer.
A separate paragraph could list these instead. Also, wrap them with double backticks.

``getenv()``

2390

Use crossref links.

2393–2401

I don't like the fact that this example works only if the alpha.cert.pos.StoreToImmutableChecker is enabled, which is an alpha checker.

There are other places where such immutable memory regions arise, such as writable strings.
Anyway, at least let's mention that the given alpha checker has anything to do with this checker (+ crossref!).

2402

You should also embed an example of how to fix this issue: E.g. advertises making a copy of the immutable region to a mutable location and doing the mutation there instead. And use const as a preventive measure for the future for the original pointer.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

766–769

Please merge these into a single isa<T1, T2,...>().

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

68–70

Put this in the right alphabetical place.

clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp

28–30

You could expose a pointer to this by creating a const BugType *getBugType() const; getter method, which could be used by other checkers.

52

We capitalize the static analyzer warnings and notes.

clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp

2–3

Fix the comment.

11–12

Comments should be capitalized and punctuated as usual.

31–44

I can see a small issue by evaCall-ing these functions.
The problem is that we might not model all aspects of these functions, thus the modeling can cause harm to the analysis. E.g. by not doing invalidation where we actually would need invalidation to kick in, or anything else really.
Consequently, another problem is that no other checker can evaluate these, since we evaluate them here.
Thus, fixing such improper modeling could end up in further changes down the line.
Unfortunately, I don't have any better ATM, so let's go with this evalCall approach.

67

One should achieve this by comparing the BugType pointers.

clang/test/Analysis/cert/env30-c.cpp

3

Also enable the 'core' package.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp

44

This check catches a lot more regions than the ones produced by the other checker. In particular, it'll warn on all global constants. Did you intend this to happen? You don't seem to have tests for that.

53

I also recommend trackExpressionValue to make sure we have all reassignments highlighted as the value gets bounced between pointer variables. The user will need proof that the pointer actually points to const memory.

clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp

31–44

Right, this definitely shouldn't be evalCall. Post-call seems to be sufficient for this checker.

[martong]

The patch adds two new checkers.

I don't see any technical dependencies between the two, so, please split it into two independent patches.

[martong]

Also, could you please update the Static Analyzer section of clang/docs/ReleaseNotes.rst with the new checkers and their description?

[zukatsinadze]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

766–769

error: macro "assert" passed 4 arguments, but takes just 1
assert(isa<UnknownSpaceRegion, HeapSpaceRegion, GlobalSystemSpaceRegion, GlobalImmutableSpaceRegion>(sreg));

So I had to add extra ()-s around isa, I guess macro expands weirdly?

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

68–70

I think it is in the right alphabetical place not considering cert/ (other certs are in similar order), but maybe I should remove the cert directory, what do you think?
It was created by me a few years ago, but I don't see a point now anymore.

clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp

28–30

I did something similar based on SmartPtrChecker.

44

Could you give an example? I tried with the following snippet and it didn't give a warning

int a = 1, b = 1;
void foo(int *p) {
  a = 2;
  p[b++] = 3;
  int *c = &a;
  *c = 5;
}

53

What expression should I use for tracking? I tried to simply cast S to Expr, but it didn't really work.
Then I came up with something ugly: cast S to BinaryOperator -> getLHS -> getSubExpr -> ignoreImpCasts.

[zukatsinadze]

In D124244#3493307, @martong wrote:

The patch adds two new checkers.

I don't see any technical dependencies between the two, so, please split it into two independent patches.

StoreToImmutable does not depend on the modeling checker, but I am using the BugType pointer from StoreToImmutable in the modeling checker for Note diagnostics.
Should it still be split and just not merge the second patch until the first one is committed?

In D124244#3493310, @martong wrote:

Also, could you please update the Static Analyzer section of clang/docs/ReleaseNotes.rst with the new checkers and their description?

Sorry, I missed this comment. I will add release notes in the next round.

[steakhal]

I found this report at vim/src/term.c (https://github.com/vim/vim.git at v8.2.1920):

I'm not sure what the exact type of BC global variable is but I think it's declared as either extern char *BC; or just char *BC;.
empty_option is declared as either extern unsigned char *empty_option; or unsigned char *empty_option = (unsigned char *)"";

Could you please have a look at this FP @zukatsinadze?

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

68–70

Leave it here.

clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp

53

Transitive interestingness might be related to this: D125362

[steakhal]

Any update on this. I'm really excited about this one. Let me know if something blocks you.

[steakhal]

In D124244#3521728, @steakhal wrote:

I found this report at vim/src/term.c (https://github.com/vim/vim.git at v8.2.1920):

I'm not sure what the exact type of BC global variable is but I think it's declared as either extern char *BC; or just char *BC;.
empty_option is declared as either extern unsigned char *empty_option; or unsigned char *empty_option = (unsigned char *)"";

Could you please have a look at this FP @zukatsinadze?

I've checked why we report this. It turns out mutable global variables were considered immutable (except the errno), which caused FPs in this proposed checker.
I made D127306 to keep mutable (system) globals as mutable memory.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp

44

I was thinking about constants, i.e. const int a etc.

clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp

31–44

Yeah so the reason why this was made an evalCall is because we needed to replace the memory space for the return value. I think the right solution is to re-implement SymRegion memory spaces as traits, so that they could be updated during analysis at any time (under the natural condition that they only ever get narrower). So like dynamic type, just dynamic memory space.

[steakhal]

Sorry for my late reply.

It feels like we have some serious obstacles.
The check::PostCall handler wants to mark some memory region immutable. Currently, the checker creates a new symbolic memregion, spawned into the immutable memory space. After this it simply re-binds the return value.
However, only eval::Call handler is supposed (must) to bind the return value, so the current implementation cannot land.

I played with the trait idea, which was this:

1. Introduce a new program state trait with the REGISTER_SET_WITH_PROGRAMSTATE(ImmutableRegions, const ento::MemRegion *). One should not bind values to these regions. (*)
2. The check::PostCall would simply insert into this set.
3. The StoreToImmutableChecker, at the check::Bind handler, would verify that the region is not contained by the set - otherwise emit a report...
4. Surface this new trait to be reachable by the Core infrastructure.
5. Refactor all the Core functions introducing or expecting MemRegionManager::getGlobalsRegions to insert the region in question into this ImmutableRegions set associated with the current State, producing some new State.

The last point is the most critical. Now, by design, MemRegionManager does not refer to the ProgramState, hence we don't have one that we could mutate by inserting into the ImmutableRegions set. Passing a ProgramStateRef along all the functions is a nightmare. Trust me, I've tried it.
That being said, eradicating GlobalImmutableSpaceRegion seems to be challenging.

I've settled on using the custom trait, and the GlobalImmutableSpaceRegion memspace kind to detect if the store (bind) should be allowed or not.

UsingTraitsForTrackingImmutability.patch6 KBDownload

WDYT @NoQ, how could we get rid of the GlobalImmutableSpaceRegion?

---

(*): Originally I wanted a set of const MemSpaceRegion *, but it turns out a default eval called function which returns a plain old mutable pointer is of SymRegion{conj{}} at the UnknownSpaceRegion. And we probably don't want to mark immutable the whole UnknownSpaceRegion xD.

[zukatsinadze]

In D124244#3588671, @steakhal wrote:

Sorry for my late reply.

It feels like we have some serious obstacles.
The check::PostCall handler wants to mark some memory region immutable. Currently, the checker creates a new symbolic memregion, spawned into the immutable memory space. After this it simply re-binds the return value.
However, only eval::Call handler is supposed (must) to bind the return value, so the current implementation cannot land.

I played with the trait idea, which was this:

1. Introduce a new program state trait with the REGISTER_SET_WITH_PROGRAMSTATE(ImmutableRegions, const ento::MemRegion *). One should not bind values to these regions. (*)
2. The check::PostCall would simply insert into this set.
3. The StoreToImmutableChecker, at the check::Bind handler, would verify that the region is not contained by the set - otherwise emit a report...
4. Surface this new trait to be reachable by the Core infrastructure.
5. Refactor all the Core functions introducing or expecting MemRegionManager::getGlobalsRegions to insert the region in question into this ImmutableRegions set associated with the current State, producing some new State.

The last point is the most critical. Now, by design, MemRegionManager does not refer to the ProgramState, hence we don't have one that we could mutate by inserting into the ImmutableRegions set. Passing a ProgramStateRef along all the functions is a nightmare. Trust me, I've tried it.
That being said, eradicating GlobalImmutableSpaceRegion seems to be challenging.

I've settled on using the custom trait, and the GlobalImmutableSpaceRegion memspace kind to detect if the store (bind) should be allowed or not.

UsingTraitsForTrackingImmutability.patch6 KBDownload

WDYT @NoQ, how could we get rid of the GlobalImmutableSpaceRegion?

---

(*): Originally I wanted a set of const MemSpaceRegion *, but it turns out a default eval called function which returns a plain old mutable pointer is of SymRegion{conj{}} at the UnknownSpaceRegion. And we probably don't want to mark immutable the whole UnknownSpaceRegion xD.

@NoQ what do you think about steakhal's proposal?

[zukatsinadze]

@NoQ gentle ping.

[donat.nagy]

Overall this seems to be a promising checker; I added a few minor remarks as inline comments. Unfortunately @steakhal's comment that

The check::PostCall handler wants to mark some memory region immutable. Currently, the checker creates a new symbolic memregion, spawned into the immutable memory space. After this it simply re-binds the return value.
However, only eval::Call handler is supposed (must) to bind the return value, so the current implementation cannot land.

highlights question that is as far as I see a blocking obstacle. I hope that you can find a solution for that technical difficulty.

Moreover, I agree with @martong's comment that you should probably create two commits for these two checkers. (One-way dependency between the checkers is not an obstacle: simply pay attention to merging the commits in the right order.)

clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp

68–72

This code is a bit hard to understand, perhaps add a comment like
// If the binding statement looks like "*ptr = ...", then track the pointer "ptr"

clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp

39

If you don't have concrete plans for extending this checker to cases where you need a different HandlerFn, then I strongly suggest replacing this map with a set (or a CallDescriptionMap<bool> where the bool is just a dummy placeholder). There is no reason to juggle member function pointers when they are always pointing to the same function.

clang/docs/analyzer/checkers.rst

  }
 
  void test() {
    static int *x;
    int y;
    x = &y; // warn
  }
 
+.. _core-StoreToImmutable:
+
+core.StoreToImmutable (C, C++)
+"""""""""""""""""""""""""""""""""""""""
+Check for writes to immutable memory.
+
+.. code-block:: cpp
+
+ void write_to_envvar() {
+  char *p = getenv("VAR");
+  if (!p)
+    return;
+  p[0] = '\0'; // warn
+ }
+
+Note that this example depends on :ref:`_alpha-security-cert-env-ModelConstQualifiedReturn`
+checker that models return value of getenv as const-qualified, however there are other
+scenarios where such immutable memory regions arise.
+
+To fix the error, user can add `const` as preventive measure and make a copy of the
+immutable memory region to a mutable location and do the mutation there instead.
+
+.. code-block:: cpp
+
+ void write_to_envvar() {
+  const char *p = getenv("VAR");
+  if (!p)
+    return;
+
+  copy_of_p = (char *)malloc(strlen(p) + 1);
+  strcpy(copy_of_p, p);
+  copy_of_p[0] = '\0';
+ }
 
 .. _core-UndefinedBinaryOperatorResult:
 
 core.UndefinedBinaryOperatorResult (C)
 """"""""""""""""""""""""""""""""""""""
 Check for undefined results of binary operators.
 
 .. code-block:: c
void previous_call_invalidation() {
     p = getenv("VAR");
     pp = getenv("VAR2");
     // subsequent call to 'getenv' invalidated previous one
 
     *p;
     // dereferencing invalid pointer
   }
 
+.. _alpha-security-cert-env-ModelConstQualifiedReturn:
+
+alpha.security.cert.env.ModelConstQualifiedReturn
+"""""""""""""""""""""""""""""""""""""""""""""""""
+
+Corresponds to SEI CERT Rules ENV30-C.
+
+Some functions return a pointer to an object that cannot be
+modified without causing undefined behavior. In such cases,
+the function call results must be treated as being const-qualified.
+
+These functions include ``getenv(), setlocale(), localeconv(), asctime(), strerror()``.
+
+Checker models return values of these functions as const
+qualified. Their modification is checked in :ref:`_core-StoreToImmutable`.
+
+.. code-block:: c
+
+  void writing_to_envvar() {
+    char *p = getenv("VAR"); // note: getenv return value
+                             // should be treated as const
+    if (!p)
+      return;
+    p[0] = '\0'; // warn
+  }
+
 alpha.security.taint
 ^^^^^^^^^^^^^^^^^^^^
 
 Checkers implementing `taint analysis <https://en.wikipedia.org/wiki/Taint_checking>`_.
 
 .. _alpha-security-taint-TaintPropagation:
 
 alpha.security.taint.TaintPropagation (C, C++)
 """"""""""""""""""""""""""""""""""""""""""""""

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

def StackAddrEscapeBase : Checker<"StackAddrEscapeBase">,
   Documentation<NotDocumented>,
   Hidden;
 
 def StackAddrEscapeChecker : Checker<"StackAddressEscape">,
   HelpText<"Check that addresses to stack memory do not escape the function">,
   Dependencies<[StackAddrEscapeBase]>,
   Documentation<HasDocumentation>;
 
+def StoreToImmutableChecker : Checker<"StoreToImmutable">,
+  HelpText<"Check for writes to immutable memory.">,
+  Documentation<HasDocumentation>;
+
 def DynamicTypePropagation : Checker<"DynamicTypePropagation">,
   HelpText<"Generate dynamic type information">,
   Documentation<NotDocumented>,
   Hidden;
 
 def NonnullGlobalConstantsChecker: Checker<"NonnilStringConstants">,
   HelpText<"Assume that const string-like globals are non-null">,
   Documentation<NotDocumented>,

 } // end "alpha.cert.pos"
 
 let ParentPackage = ENV in {
 
   def InvalidPtrChecker : Checker<"InvalidPtr">,
   HelpText<"Finds usages of possibly invalidated pointers">,
   Documentation<HasDocumentation>;
 
+  def ModelConstQualifiedReturnChecker : Checker<"ModelConstQualifiedReturn">,
+  HelpText<"Model return values of some functions as const-qualified. "
+           "StoreToImmutable checker would warn on their modification.">,
+  Documentation<HasDocumentation>;
+
+
 } // end "alpha.cert.env"
 
 let ParentPackage = SecurityAlpha in {
 
 def ArrayBoundChecker : Checker<"ArrayBound">,
   HelpText<"Warn about buffer overflows (older checker)">,
   Documentation<HasDocumentation>;
 

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

class SymbolicRegion : public SubRegion {
 
   SymbolicRegion(const SymbolRef s, const MemSpaceRegion *sreg)
       : SubRegion(sreg, SymbolicRegionKind), sym(s) {
     // Because pointer arithmetic is represented by ElementRegion layers,
     // the base symbol here should not contain any arithmetic.
     assert(s && isa<SymbolData>(s));
     assert(s->getType()->isAnyPointerType() ||
            s->getType()->isReferenceType() ||
            s->getType()->isBlockPointerType());
-    assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg) ||
-           isa<GlobalSystemSpaceRegion>(sreg));
+    assert((isa<UnknownSpaceRegion, HeapSpaceRegion, GlobalSystemSpaceRegion,
+                GlobalImmutableSpaceRegion>(sreg)));
   }
 
 public:
   SymbolRef getSymbol() const { return sym; }
 
   bool isBoundable() const override { return true; }
 
   void Profile(llvm::FoldingSetNodeID& ID) const override;
 

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   LocalizationChecker.cpp
   MacOSKeychainAPIChecker.cpp
   MacOSXAPIChecker.cpp
   MallocChecker.cpp
   MallocOverflowSecurityChecker.cpp
   MallocSizeofChecker.cpp
   MismatchedIteratorChecker.cpp
   MmapWriteExecChecker.cpp
   MIGChecker.cpp
+  cert/ModelConstQualifiedReturnChecker.cpp
   MoveChecker.cpp
   MPI-Checker/MPIBugReporter.cpp
   MPI-Checker/MPIChecker.cpp
   MPI-Checker/MPIFunctionClassifier.cpp
   NSAutoreleasePoolChecker.cpp
   NSErrorChecker.cpp
   NoReturnFunctionChecker.cpp
   NonNullParamChecker.cpp
   NonnullGlobalConstantsChecker.cpp
add_clang_library(clangStaticAnalyzerCheckers
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
   STLAlgorithmModeling.cpp
+  StoreToImmutableChecker.cpp
   StreamChecker.cpp
   StringChecker.cpp
   Taint.cpp
   TaintTesterChecker.cpp
   TestAfterDivZeroChecker.cpp
   TraversalChecker.cpp
   TrustNonnullChecker.cpp
   TrustReturnsNonnullChecker.cpp

clang/lib/StaticAnalyzer/Checkers/StoreToImmutable.h

+//=== StoreToImmutable.h - Expose ImmutableMemoryBind BugType ------*- C++ -*-//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Defines getter function for ImmutableMemoryBind BugType
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_STORETOIMMUTABLE_H
+#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_STORETOIMMUTABLE_H
+
+
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+
+namespace clang {
+namespace ento {
+namespace immutablestore {
+
+const BugType *getImmutableMemoryBindBugType();
+
+} // namespace immutablestore
+} // namespace ento
+} // namespace clang
+
+#endif // LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_STORETOIMMUTABLE_H

clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp

+//== StoreToImmutableChecker.cpp ------------------------------- -*- C++ -*--=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines StoreToImmutableChecker which warns binds on immutable
+// memory.
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/Type.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+static const BugType *ImmutableMemoryBindPtr;
+
+class StoreToImmutableChecker : public Checker<check::Bind> {
+public:
+  BugType ImmutableMemoryBind{this,
+                              "Immutable memory should not be overwritten",
+                              categories::MemoryError};
+  void checkBind(SVal loc, SVal val, const Stmt *S, CheckerContext &C) const;
+};
+} // namespace
+
+
+namespace clang {
+namespace ento {
+namespace immutablestore {
+
+const BugType *getImmutableMemoryBindBugType() { return ImmutableMemoryBindPtr; }
+
+} // namespace immutablestore
+} // namespace ento
+} // namespace clang
+
+
+
+
+// Check if region that should be immutable is being modified
+void StoreToImmutableChecker::checkBind(SVal Loc, SVal Val, const Stmt *S,
+                                        CheckerContext &C) const {
+  const MemRegion *R = Loc.getAsRegion();
+  if (!R)
+    return;
+
+  if (!isa<GlobalImmutableSpaceRegion>(R->getMemorySpace()))
+    return;
+
+  ExplodedNode *ErrorNode = C.generateErrorNode();
+  if (!ErrorNode)
+    return;
+
+  auto Report = std::make_unique<PathSensitiveBugReport>(
+      ImmutableMemoryBind, "Modifying immutable memory", ErrorNode);
+  Report->markInteresting(R);
+
+  if (const auto *BP = dyn_cast<BinaryOperator>(S))
+    if (const auto *UP = dyn_cast<UnaryOperator>(BP->getLHS())) {
+      bugreporter::trackExpressionValue(
+          Report->getErrorNode(), UP->getSubExpr()->IgnoreImpCasts(), *Report);
+    }
+
+  C.emitReport(std::move(Report));
+}
+
+void ento::registerStoreToImmutableChecker(CheckerManager &Mgr) {
+  StoreToImmutableChecker *Checker = Mgr.registerChecker<StoreToImmutableChecker>();
+  ImmutableMemoryBindPtr = &Checker->ImmutableMemoryBind;
+}
+
+bool ento::shouldRegisterStoreToImmutableChecker(const CheckerManager &) {
+  return true;
+}

clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp

+//== ModelConstQualifiedReturnChecker.cpp ----------------------- -*- C++ -*--=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines ModelConstQualifiedReturnChecker.
+// Return values of certain functions should be treated as const-qualified.
+//
+// Checker is based on CERT SEI Rule ENV30-C.
+// For more information see: https://wiki.sei.cmu.edu/confluence/x/79UxBQ
+//===----------------------------------------------------------------------===//
+
+#include "../StoreToImmutable.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class ModelConstQualifiedReturnChecker : public Checker<check::PostCall> {
+private:
+  using HandlerFn = void (ModelConstQualifiedReturnChecker::*)(
+      const CallEvent &Call, CheckerContext &C) const;
+
+  void postConstQualifiedReturnCall(const CallEvent &Call,
+                                    CheckerContext &C) const;
+
+  // SEI CERT ENV30-C
+  const CallDescriptionMap<HandlerFn> ConstQualifiedReturnFunctions = {
+      {{"getenv", 1},
+       &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall},
+      {{"setlocale", 2},
+       &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall},
+      {{"strerror", 1},
+       &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall},
+      {{"localeconv", 0},
+       &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall},
+      {{"asctime", 1},
+       &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall},
+  };
+
+public:
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+};
+} // namespace
+
+void ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall(
+    const CallEvent &Call, CheckerContext &C) const {
+  const LocationContext *LCtx = C.getLocationContext();
+  SValBuilder &SVB = C.getSValBuilder();
+  MemRegionManager &RegMgr = SVB.getRegionManager();
+
+  // Function call will return a pointer to the new symbolic region.
+  SymbolRef FreshSym = SVB.getSymbolManager().conjureSymbol(
+      Call.getOriginExpr(), LCtx, C.blockCount());
+  const SymbolicRegion *SymReg = RegMgr.getSymbolicRegion(
+      FreshSym,
+      RegMgr.getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind));
+
+  ProgramStateRef State = C.getState();
+  State =
+      State->BindExpr(Call.getOriginExpr(), LCtx, loc::MemRegionVal{SymReg});
+
+  StringRef FunctionName = Call.getCalleeIdentifier()->getName();
+
+  const NoteTag *Note = C.getNoteTag(
+      [SymReg, FunctionName](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
+        if (&BR.getBugType() != immutablestore::getImmutableMemoryBindBugType() ||
+            !BR.isInteresting(SymReg))
+          return;
+        Out << "Return value of '" << FunctionName
+            << "' should be treated as const-qualified";
+      });
+  C.addTransition(State, Note);
+}
+
+void ModelConstQualifiedReturnChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
+  // Check if function return value should be const qualified
+  const auto *Handler = ConstQualifiedReturnFunctions.lookup(Call);
+  if (!Handler)
+    return;
+
+  (this->**Handler)(Call, C);
+}
+
+void ento::registerModelConstQualifiedReturnChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<ModelConstQualifiedReturnChecker>();
+}
+
+bool ento::shouldRegisterModelConstQualifiedReturnChecker(const CheckerManager &) {
+  return true;
+}

clang/test/Analysis/analyzer-enabled-checkers.c

 // CHECK-NEXT: core.CallAndMessage
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonNullParamChecker
 // CHECK-NEXT: core.NonnilStringConstants
 // CHECK-NEXT: core.NullDereference
 // CHECK-NEXT: core.StackAddrEscapeBase
 // CHECK-NEXT: core.StackAddressEscape
+// CHECK-NEXT: core.StoreToImmutable
 // CHECK-NEXT: core.UndefinedBinaryOperatorResult
 // CHECK-NEXT: core.VLASize
 // CHECK-NEXT: core.builtin.BuiltinFunctions
 // CHECK-NEXT: core.builtin.NoReturnFunctions
 // CHECK-NEXT: core.uninitialized.ArraySubscript
 // CHECK-NEXT: core.uninitialized.Assign
 // CHECK-NEXT: core.uninitialized.Branch
 // CHECK-NEXT: core.uninitialized.CapturedBlockVariable

clang/test/Analysis/cert/env30-c.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,alpha.security.cert.env.ModelConstQualifiedReturn \
+// RUN: -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+int setenv(const char *name, const char *value, int overwrite);
+void free(void *memblock);
+void *malloc(size_t size);
+char *strdup(const char *s);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char *, const char *);
+void const_parameter_function(const char *);
+
+
+
+void getenv_test1() {
+  char *a, *b, *c;
+  a = getenv("VAR");
+  // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+  // expected-note@-2{{Value assigned to 'a'}}
+  b = a;
+  // expected-note@-1{{The value of 'a' is assigned to 'b'}}
+  c = b;
+  // expected-note@-1{{The value of 'b' is assigned to 'c'}}
+  *c = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void getenv_test2() {
+  char *p;
+  p = getenv("VAR");
+  const_parameter_function(p); // no-warning
+}
+
+void modify(char *p) {
+  *p = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void getenv_test3() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+  // expected-note@-2{{'p' initialized here}}
+  char *pp = p;
+  // expected-note@-1{{'pp' initialized to the value of 'p'}}
+  modify(pp); // Writing to immutable region within the call.
+  // expected-note@-1{{Calling 'modify'}}
+  // expected-note@-2{{Passing value via 1st parameter 'p'}}
+}
+
+void does_not_modify(char *p) {
+  *p;
+}
+
+void getenv_test4() {
+  char *p = getenv("VAR");
+  does_not_modify(p); // no-warning
+}
+
+void getenv_test5() {
+  static char *array[] = {0};
+
+  if (!array[0]) {
+    // expected-note@-1{{Taking true branch}}
+    array[0] = getenv("TEMPDIR");
+    // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+    // expected-note@-2{{Assigning value}}
+  }
+
+  *array[0] = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void getenv_test6() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+  if (!p)
+    // expected-note@-1{{Assuming 'p' is non-null}}
+    // expected-note@-2{{Taking false branch}}
+    return;
+  p[0] = '\0';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void setlocale_test() {
+  char *p = setlocale(0, "VAR");
+  // expected-note@-1{{Return value of 'setlocale' should be treated as const-qualified}}
+  // expected-note@-2{{'p' initialized here}}
+  *p = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void strerror_test() {
+    char *p = strerror(0);
+    // expected-note@-1{{Return value of 'strerror' should be treated as const-qualified}}
+    // expected-note@-2{{'p' initialized here}}
+    *p = 'A';
+    // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+    // expected-note@-2{{Modifying immutable memory}}
+}
+
+void asctime_test() {
+  char* p = asctime(nullptr);
+  // expected-note@-1{{Return value of 'asctime' should be treated as const-qualified}}
+  // expected-note@-2{{'p' initialized here}}
+
+  *p = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+
+// Test cases from CERT Rule Page
+
+namespace noncompliant {
+void trstr(char *c_str, char orig, char rep) {
+  while (*c_str != '\0') {
+    // expected-note@-1{{Assuming the condition is true}}
+    // expected-note@-2{{Loop condition is true.  Entering loop body}}
+    if (*c_str == orig) {
+      // expected-note@-1{{Assuming the condition is true}}
+      // expected-note@-2{{Taking true branch}}
+      *c_str = rep;
+      // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+      // expected-note@-2{{Modifying immutable memory}}
+    }
+    ++c_str;
+  }
+}
+
+void func(void) {
+  char *env = getenv("TEST_ENV");
+  // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+  // expected-note@-2{{'env' initialized here}}
+  if (env == NULL) {
+    // expected-note@-1{{Assuming 'env' is not equal to NULL}}
+    // expected-note@-2{{Taking false branch}}
+    return;
+  }
+  trstr(env, '"', '_'); // Writing to immutable region within the call.
+  // expected-note@-1{{Calling 'trstr'}}
+  // expected-note@-2{{Passing value via 1st parameter 'c_str'}}
+}
+
+} // namespace noncompliant
+
+namespace compliant1 {
+void trstr(char *c_str, char orig, char rep) {
+  while (*c_str != '\0') {
+    if (*c_str == orig) {
+      *c_str = rep; // no-warning
+    }
+    ++c_str;
+  }
+}
+
+void func(void) {
+  const char *env;
+  char *copy_of_env;
+
+  env = getenv("TEST_ENV");
+  if (env == NULL) {
+    return;
+  }
+
+  copy_of_env = (char *)malloc(strlen(env) + 1);
+  if (copy_of_env == NULL) {
+    return;
+  }
+
+  strcpy(copy_of_env, env);
+  trstr(copy_of_env, '"', '_');
+  /* ... */
+  free(copy_of_env);
+}
+} // namespace compliant1
+
+namespace compliant2 {
+void trstr(char *c_str, char orig, char rep) {
+  while (*c_str != '\0') {
+    if (*c_str == orig) {
+      *c_str = rep; // no-warning
+    }
+    ++c_str;
+  }
+}
+
+void func(void) {
+  const char *env;
+  char *copy_of_env;
+
+  env = getenv("TEST_ENV");
+  if (env == NULL) {
+    return;
+  }
+
+  copy_of_env = strdup(env);
+  if (copy_of_env == NULL) {
+    return;
+  }
+
+  trstr(copy_of_env, '"', '_');
+
+  if (setenv("TEST_ENV", copy_of_env, 1) != 0) {
+    return;
+  }
+  /* ... */
+  free(copy_of_env);
+}
+} // namespace compliant2

clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c

 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
 // CHECK-NEXT: core.NullDereference
 // CHECK-NEXT: core.StackAddrEscapeBase
 // CHECK-NEXT: core.StackAddressEscape
+// CHECK-NEXT: core.StoreToImmutable
 // CHECK-NEXT: core.UndefinedBinaryOperatorResult
 // CHECK-NEXT: core.VLASize
 // CHECK-NEXT: core.builtin.BuiltinFunctions
 // CHECK-NEXT: core.builtin.NoReturnFunctions
 // CHECK-NEXT: core.uninitialized.ArraySubscript
 // CHECK-NEXT: core.uninitialized.Assign
 // CHECK-NEXT: core.uninitialized.Branch
 // CHECK-NEXT: core.uninitialized.CapturedBlockVariable