gamesh411 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`` + ---------------- whisperity wrote: > What does it mean that these are "default propagations"? That taint passes > through calls to them trivially? Added just 2 sentences about what default means. Please see them a bit further up in the new version of the patch. ================ 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`` + ---------------- whisperity wrote: > `execvp` and `execvP`? What is the capital `P` for? I can't find this > overload in the POSIX docs. I have found a refence to this function inside `TargetLibraryInfoTest.cpp`. ``` 926 │ case LibFunc_execv: 927 │ case LibFunc_execvp: 928 │ return (NumParams == 2 && FTy.getParamType(0)->isPointerTy() && 929 │ FTy.getParamType(1)->isPointerTy() && 930 │ FTy.getReturnType()->isIntegerTy(32)); 931 │ case LibFunc_execvP: 932 │ case LibFunc_execvpe: 933 │ case LibFunc_execve: 934 │ return (NumParams == 3 && FTy.getParamType(0)->isPointerTy() && 935 │ FTy.getParamType(1)->isPointerTy() && 936 │ FTy.getParamType(2)->isPointerTy() && 937 │ FTy.getReturnType()->isIntegerTy(32)); ``` It seems like an OSX-specific spelling and seems like it has the semantics of `execve`. During the implementation of this patch, I just collected the references to functions inside `GenericTaintChecker.cpp`. ================ 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: > whisperity wrote: > > //The// Clang Static [...]? > uses, or can use? For brevity and clarity, I will leave it at `uses`, it is just a generic statement about the availability of taint analysis as a method, and I think it would not improve the understanding to change it to `can use`. IMHO it is implicit, and `can use` would make people wonder as to what does Clang need to use it etc. ================ 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: > 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? Good point, added a line to clarify this. ================ Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:23 + +.. _taint-configuration-example: + ---------------- whisperity wrote: > 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!) added `clangsa-` prefix ================ Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:30 + + Filters: + # signature: ---------------- whisperity wrote: > //Filters// are not mentioned earlier. Added a mention with references in the Overview section. ================ 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 ---------------- whisperity wrote: > 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. Reformatted the examples section according to your suggestions. ================ Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:41 + Propagations: + # sources: + # signature: ---------------- whisperity wrote: > 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? It is because `Propagations` `YAML`-keys are used to define both sources and propagations. So to comment the "source-section" 0-indent is used, and to comment on individual entries within, 2-indent comments are used. I have consolidated the indents and introduced vertical whitespace instead of multiple indentation levels. ================ Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:51 + - Name: fread + DstArgs: [0, -1] + ---------------- whisperity wrote: > What are these magic numbers and what is their meaning? Added an extra comment on top of the example YAML file. 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
