[clang][analyzer] Improve documentation of StdCLibraryFunctionArgs checker (NFC)

[balazske]

Summary

Documentation is made more exact, term "constraint" is removed entirely,
description of checker option is corrected.

[Szelethus]

clang/docs/analyzer/checkers.rst

2495–2496

This makes sense to me, and likely all of us knowledgable about symbolic execution (and clang in particular), but make little sense to end users. Could you add an example to ease reading?

"For instance, if the argument to <fn> must be in between 0 and 255. If the value of the argument is unknown, the analyzer will assume that it is in this interval, even if warnings for this checker are disabled. Similarly, if a function mustn't be called with a null pointer but it is, analysis will stop on that execution path (similarly to a division by zero), with or without a warning."

2495–2496

Restriction is not a bad word, but lets ease into it a bit.

"You can think of this checker as defining restrictions (pre- and postconditions) on standard library functions. Preconditions are checked, and when they are violated, a warning is emitted. Post conditions are added to the analysis, e.g. that the return value must be no greater than 255."

2496–2529

We should create an option or something the actual list of functions we model. This is the prime example of unsustainable documenting.

2513

Isn't this something that we either do or do not enable by default? My memory betrays me.

[balazske]

Applied review suggestions, removed the list of functions.

[balazske]

clang/docs/analyzer/checkers.rst

2496–2529

Such function lists are used at documentation of other checkers, but I am not sure if it is good to add such a long list here. Probably the "DisplayLoadedSummaries" option of apiModeling.StdCLibraryFunctions checker can be used, this lists only the actually found functions (that have available declaration and are enabled), and the console output needs to be observed to see the list. And this option is currently not documented.

2513

Currently ModelPOSIX is false by default.

[Szelethus]

clang/docs/analyzer/checkers.rst

2476–2477

2496–2529

Such function lists are used at documentation of other checkers

Is it possible that those lists are not really expected to change? We do expect the list for this checker to grow, do we not?

[balazske]

Applied another review suggestion.

[balazske]

clang/docs/analyzer/checkers.rst

2496–2529

At first look not all other checkers check every possible function, only the most common ones, it looks possible to extend these. Some have an incomplete list in the documentation. My concern was if an user want to know if a specific function is checked or not.

clang/docs/analyzer/checkers.rst

 """"""""""""""""""""""""""""""""""""""
 Check for calls of standard library functions that violate predefined argument
 constraints. For example, it is stated in the C standard that for the ``int
 isalnum(int ch)`` function the behavior is undefined if the value of ``ch`` is
 not representable as unsigned char and is not equal to ``EOF``.
 
 .. code-block:: c
 
+  #define EOF -1
   void test_alnum_concrete(int v) {
     int ret = isalnum(256); // \
-    // warning: Function argument constraint is not satisfied
+    // warning: Function argument outside of allowed range
     (void)ret;
   }
 
-If the argument's value is unknown then the value is assumed to hold the proper value range.
+  void buffer_size_violation(FILE *file) {
+    enum { BUFFER_SIZE = 1024 };
+    wchar_t wbuf[BUFFER_SIZE];
+
+    const size_t size = sizeof(*wbuf);   // 4
+    const size_t nitems = sizeof(wbuf);  // 4096
+
+    // Below we receive a warning because the 3rd parameter should be the
+    // number of elements to read, not the size in bytes. This case is a known
+    // vulnerability described by the ARR38-C SEI-CERT rule.
+    fread(wbuf, size, nitems, file);
+  }
+
+You can think of this checker as defining restrictions (pre- and postconditions)
+on standard library functions. Preconditions are checked, and when they are
+violated, a warning is emitted. Post conditions are added to the analysis, e.g.
+that the return value must be no greater than 255.
+
+These are the possible checks on the values passed as function arguments:
+ - The argument has an allowed range (or multiple ranges) of values. The checker
+   can detect if a passed value is outside of the allowed range and show the
+   actual and allowed values.
+ - The argument has pointer type and is not allowed to be null pointer. Many
+   (but not all) standard functions can produce undefined behavior if a null
+   pointer is passed, these cases can be detected by the checker.
+ - The argument is a pointer to a memory block and the minimal size of this
+   buffer is determined by another argument to the function, or by
+   multiplication of two arguments (like at function ``fread``), or is a fixed
+   value (for example ``asctime_r`` requires at least a buffer of size 26). The
+   checker can detect if the buffer size is too small and in optimal case show
+   the size of the buffer and the values of the corresponding arguments.
+
+If the user disables the checker then the argument violation warning is
+suppressed. However, the assumption about the argument is still modeled.
+For instance, if the argument to a function must be in between 0 and 255,
+but the value of the argument is unknown, the analyzer will conservatively
+assume that it is in this interval, even if warnings for this checker are
+disabled. Similarly, if a function mustn't be called with a null pointer but it
+is, analysis will stop on that execution path (similarly to a division by zero),
+with or without a warning. If the null value of the argument can not be proven,
+the analyzer will assume that it is non-null.
 
 .. code-block:: c
 
-  #define EOF -1
   int test_alnum_symbolic(int x) {
     int ret = isalnum(x);
     // after the call, ret is assumed to be in the range [-1, 255]
 
     if (ret > 255)      // impossible (infeasible branch)
       if (x == 0)
         return ret / x; // division by zero is not reported
     return ret;
   }
 
-If the user disables the checker then the argument violation warning is
-suppressed. However, the assumption about the argument is still modeled. This
-is because exploring an execution path that already contains undefined behavior
-is not valuable.
-
-There are different kind of constraints modeled: range constraint, not null
-constraint, buffer size constraint. A **range constraint** requires the
-argument's value to be in a specific range, see ``isalnum`` as an example above.
-A **not null constraint** requires the pointer argument to be non-null.
-
-A **buffer size** constraint specifies the minimum size of the buffer
-argument. The size might be a known constant. For example, ``asctime_r`` requires
-that the buffer argument's size must be greater than or equal to ``26`` bytes. In
-other cases, the size is denoted by another argument or as a multiplication of
-two arguments.
-For instance, ``size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream)``.
-Here, ``ptr`` is the buffer, and its minimum size is ``size * nmemb``
-
-.. code-block:: c
-
-  void buffer_size_constraint_violation(FILE *file) {
-    enum { BUFFER_SIZE = 1024 };
-    wchar_t wbuf[BUFFER_SIZE];
-
-    const size_t size = sizeof(*wbuf);   // 4
-    const size_t nitems = sizeof(wbuf);  // 4096
-
-    // Below we receive a warning because the 3rd parameter should be the
-    // number of elements to read, not the size in bytes. This case is a known
-    // vulnerability described by the ARR38-C SEI-CERT rule.
-    fread(wbuf, size, nitems, file);
-  }
-
 **Limitations**
 
-The checker is in alpha because the reports cannot provide notes about the
-values of the arguments. Without this information it is hard to confirm if the
-constraint is indeed violated. For example, consider the above case for
-``fread``. We display in the warning message that the size of the 1st arg
-should be equal to or less than the value of the 2nd arg times the 3rd arg.
-However, we fail to display the concrete values (``4`` and ``4096``) for those
-arguments.
+The checker can not always provide notes about the values of the arguments.
+Without this information it is hard to confirm if the constraint is indeed
+violated. The argument values are shown if they are known constants or the value
+is determined by previous (not too complicated) assumptions.
+
+The checker can produce false positives in cases such as if the program has
+invariants not known to the analyzer engine or the bug report path contains
+calls to unknown functions. In these cases the analyzer fails to detect the real
+range of the argument.
 
 **Parameters**
 
 The checker models functions (and emits diagnostics) from the C standard by
-default. The ``ModelPOSIX`` option enables the checker to model (and emit
-diagnostics) for functions that are defined in the POSIX standard. This option
-is disabled by default.
+default. The ``apiModeling.StdCLibraryFunctions:ModelPOSIX`` option enables
+modeling (and emit diagnostics) of additional functions that are defined in the
+POSIX standard. This option is disabled by default. Note that this option
+belongs to a separate built-in checker ``apiModeling.StdCLibraryFunctions`` and
+can have effect on other checkers because it toggles modeling of the functions
+in various aspects.
 
 .. _alpha-unix-BlockInCriticalSection:
 
 alpha.unix.BlockInCriticalSection (C)
 """""""""""""""""""""""""""""""""""""
 Check for calls to blocking functions inside a critical section.
 Applies to: ``lock, unlock, sleep, getc, fgets, read, recv, pthread_mutex_lock,``
 `` pthread_mutex_unlock, mtx_lock, mtx_timedlock, mtx_trylock, mtx_unlock, lock_guard, unique_lock``
 
 .. code-block:: c
 
  void test() {
    std::mutex m;
    m.lock();
    sleep(3); // warn: a blocking function sleep is called inside a critical
              //       section
    m.unlock();
  }
 
 .. _alpha-unix-Chroot: