[analyzer] Extend the documentation of MallocOverflow

[steakhal]

Summary

Previously by following the documentation it was not immediately clear what the capabilities of this checker are.

In this patch, I add some clarification on when does the checker issue a report and what it's limitations are.
I'm also advertising suppressing such reports by adding an assertion, as demonstrated by the test3().
I'm highlighting that this checker might produce an extensive amount of findings, but it might be still useful for code audits.

---

IMO the rule this checker enforces seems to be solid and complete by considering the capabilities of an AST-based approach.

Quoting @NoQ from cfe-dev: Alpha checker statuses. @ Tue May 14 15:40:33 PDT 2019:

alpha.security.MallocOverflow:
This one's extremely loud for me (~1500 false positives). It looks as if it warns on every malloc(x * sizeof(int)) (due to potential integer overflow during multiplication) so i just don't see it working as an AST-based check. We should probably rewrite it on top of taint analysis and then it'll need a constraint solver that knows how to multiply things.

It suggests to me that this checker is supposed to be replaced by a path-sensitive implementation and remove this AST-based one from the source tree.
However, I think this checker has its value, even if its scope is a narrower user-base.

That being said, AFAIK the optin package houses the checkers which are ready for production but enforces a specific coding style or rule, although not recommended for default use. Shouldn't we move this checker to the optin package instead?
If not, we should probably pinpoint the requirements to do so and document them as TODOs in a subsequent patch.
WDYT?

[steakhal]

As soon as D107804 lands, I'm going to update this documentation accordingly.

[martong]

clang/docs/analyzer/checkers.rst

2179

This should be n < 100, shouldn't it?

[martong]

That being said, AFAIK the optin package houses the checkers which are ready for production but enforces a specific coding style or rule, although not recommended for default use. Shouldn't we move this checker to the optin package instead? If not, we should probably pinpoint the requirements to do so and document them as TODOs in a subsequent patch. WDYT?

This is what the code says:

// The OptIn package is for checkers that are not alpha and that would normally
// be on by default but where the driver does not have enough information to
// determine when they are applicable. For example, localizability checkers fit
// this criterion because the driver cannot determine whether a project is
// localized or not -- this is best determined at the IDE or build-system level.
//
// The checker hierarchy under OptIn should mirror that in Alpha: checkers
// should be organized as if they were at the top level.
//
// Note: OptIn is *not* intended for checkers that are too noisy to be on by
// default. Such checkers belong in the alpha package.

So, in my opinion, we should put rather webkit and fuchsia checkers under optin (if they were not having their own parent packages).

And this checker seems to be related to the security SEI CERT rule MEM35-C. Thus, I think security is a good place for it.

[martong]

In D107756#2936274, @steakhal wrote:

As soon as D107804 lands, I'm going to update this documentation accordingly.

Could you please set up the stack then? I mean could you please add that patch as this one's parent?

[steakhal]

n > 100 -> n <= 100

[steakhal]

I'm not exactly sure what does the sphinx build bot complain about:

Warning, treated as error:
/home/buildbot/llvm-build-dir/clang-sphinx-docs/llvm/build/tools/clang/docs/analyzer/checkers.rst:2159:Unexpected indentation.

Unfortunately, I could not (yet) set up sphinx locally, so I cannot reproduce this.
Does anyone have it have sphinx ready to go? Am I on the right track?

clang/docs/analyzer/checkers.rst

2159–2160

Am I supposed to indent this with 2 spaces? Or what.

[steakhal]

clang/docs/analyzer/checkers.rst

2159–2160

It seems like an empty newline solved this before the list.

clang/docs/analyzer/checkers.rst

void test() {
    int x = getchar();
    char c = s[x]; // warn: index is tainted
  }
 
 .. _alpha-security-MallocOverflow:
 
 alpha.security.MallocOverflow (C)
 """""""""""""""""""""""""""""""""
-Check for overflows in the arguments to malloc().
+Check for overflows in the arguments to ``malloc()``.
+It tries to catch ``malloc(n * c)`` patterns, where:
+ - ``n``: a variable or member access of an object
+ - ``c``: a constant foldable integral
+
+This checker was designed for code audits, so expect false-positive reports.
+One is supposed to silence this checker by ensuring proper bounds checking on
+the variable in question using e.g. an ``assert()`` or a branch.
 
 .. code-block:: c
 
  void test(int n) {
    void *p = malloc(n * sizeof(int)); // warn
  }
 
  void test2(int n) {
    if (n > 100) // gives an upper-bound
      return;
    void *p = malloc(n * sizeof(int)); // no warning
  }
 
+ void test3(int n) {
+   assert(n <= 100 && "Contract violated.");
+   void *p = malloc(n * sizeof(int)); // no warning
+ }
+
+Limitations:
+ - The checker won't warn for variables involved in explicit casts,
+   since that might limit the variable's domain.
+   E.g.: ``(unsigned char)int x`` would limit the domain to ``[0,255]``.
+   The checker will miss the true-positive cases when the explicit cast would
+   not tighten the domain to prevent the overflow in the subsequent
+   multiplication operation.
+
+ - If the variable ``n`` participates in a comparison anywhere in the enclosing
+   function's scope, even after the ``malloc()``, the report will be still
+   suppressed.
+
+ - It is an AST-based checker, thus it does not make use of the
+   path-sensitive taint-analysis.
+
 .. _alpha-security-MmapWriteExec:
 
 alpha.security.MmapWriteExec (C)
 """"""""""""""""""""""""""""""""
 Warn on mmap() calls that are both writable and executable.
 
 .. code-block:: c