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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
