[PATCH] D113251: [analyzer][doc] Add user documenation for taint analysis

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang/docs/analyzer/checkers.rst:2341-2342
+
+Default propagations defined by `GenericTaintChecker`:
+``atoi``, ``atol``, ``atoll``, ``fgetc``, ``fgetln``, ``fgets``, ``fscanf``, ``sscanf``, ``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``, ``pread``, ``read``, ``strchr``, ``strrchr``, ``tolower``, ``toupper``
+
----------------
What does it mean that these are "default propagations"? That taint passes through calls to them trivially?


================
Comment at: clang/docs/analyzer/checkers.rst:2345
+Default sinks defined in `GenericTaintChecker`:
+``printf``, ``setproctitle``, ``system``, ``popen``, ``execl``, ``execle``, ``execlp``, ``execv``, ``execvp``, ``execvP``, ``execve``, ``dlopen``, ``memcpy``, ``memmove``, ``strncpy``, ``strndup``, ``malloc``, ``calloc``, ``alloca``, ``memccpy``, ``realloc``, ``bcopy``
+
----------------
`execvp` and `execvP`? What is the capital `P` for? I can't find this overload in the POSIX docs.


================
Comment at: clang/docs/analyzer/checkers.rst:2353
+
+External taint configuration is in `YAML <https://yaml.org/>`_ format. The taint-related options defined in the config file extend but do not override the built-in sources, rules, sinks.
+
----------------
Perhaps we should link to the in-house YAML doc (http://llvm.org/docs/YamlIO.html#introduction-to-yaml) first, and have the user move to other sources from there?


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:5
+
+Clang Static Analyzer uses taint analysis to detect security-related issues in code.
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which the user can access via the :ref:`alpha-security-taint-TaintPropagation` checker alias and this checker has a default taint-related configuration.
----------------
//The// Clang Static [...]?


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:5
+
+Clang Static Analyzer uses taint analysis to detect security-related issues in code.
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which the user can access via the :ref:`alpha-security-taint-TaintPropagation` checker alias and this checker has a default taint-related configuration.
----------------
whisperity wrote:
> //The// Clang Static [...]?
uses, or can use?


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:6
+Clang Static Analyzer uses taint analysis to detect security-related issues in code.
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which the user can access via the :ref:`alpha-security-taint-TaintPropagation` checker alias and this checker has a default taint-related configuration.
+The checker also provides a configuration interface for extending the default settings by providing a configuration file in `YAML <https://yaml.org/>`_ format.
----------------
Clang -> Clang SA / CSA?


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:6
+Clang Static Analyzer uses taint analysis to detect security-related issues in code.
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which the user can access via the :ref:`alpha-security-taint-TaintPropagation` checker alias and this checker has a default taint-related configuration.
+The checker also provides a configuration interface for extending the default settings by providing a configuration file in `YAML <https://yaml.org/>`_ format.
----------------
whisperity wrote:
> Clang -> Clang SA / CSA?
So the checker has the defaults all the time, or only by enabling the alias can I get the defaults?


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:7
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which the user can access via the :ref:`alpha-security-taint-TaintPropagation` checker alias and this checker has a default taint-related configuration.
+The checker also provides a configuration interface for extending the default settings by providing a configuration file in `YAML <https://yaml.org/>`_ format.
+This documentation describes the syntax of the configuration file and gives the informal semantics of the configuration options.
----------------
Ditto about YAML.


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:13
+
+.. _taint-configuration-overview
+
----------------
I think this wanted to be an anchor? Also see how the syntax highlighting broke in the next section.


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:23
+
+.. _taint-configuration-example:
+
----------------
AFAIK, RST anchors are global identifiers for the project, so maybe we should take care in adding "clang-sa" or some other sort of namespaceing...

(Although this might depend on how exactly the docs are built!)


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:30
+
+  Filters:
+    # signature:
----------------
//Filters// are not mentioned earlier.


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:31-36
+    # signature:
+    # void cleanse_first_arg(int* arg)
+    #
+    # example:
+    # int x; // x is tainted
+    # cleanse_first_arg(&x); // x is not tainted anymore
----------------
If none of the comments themselves are just commented out ("optional") keys in the YAML, it would be better readable with a bit of formatting.


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:41
+  Propagations:
+  # sources:
+    # signature:
----------------
steakhal wrote:
> Since there the `SrcArgs` is //empty//, `fread` will work as an //unconditional// propagator - aka. a taint source.
> It's probably better to phrase it something like that.
> 
> By doing so you could signify that in the next example you have a //non-empty// `SrcArgs` list, thus the propagation is //conditional//.
> 
> I think it would be better to follow this pattern in the corresponding section later as well.
Why is this comment at indent 0?


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:51
+    - Name: fread
+      DstArgs: [0, -1]
+
----------------
What are these magic numbers and what is their meaning?


================
Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:125-126
+   If any `SrcArgs` arguments are tainted, the checker will consider all `DstArgs` arguments tainted after the call.
+ - `DstArgs` is a list of numbers in the range of [-1..int_max] that indicates the indexes of arguments in the function call event.
+   The number -1 specifies the return value of the function.
+   If any `SrcArgs` arguments are tainted, the checker will consider all `DstArgs` arguments tainted after the call.
----------------
Ensure that the numeric literal shows up as "code" with double backtick.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113251