[PATCH] D154838: [analyzer] Add check for null pointer passed to %p of printf family

Mon Jul 10 05:59:55 PDT 2023

CuriousGeorgiy added a comment.

Hi! This is my first time contributing to the LLVM project and particularly the clang static analyzer. This patch is based off https://reviews.llvm.org/D139604?id=481154. I have several questions regarding the reviewers comments (most of which I tried to address) and the patch itself:

> 4. One should be able to disable your rule in case something goes wild. So, your checker should be registered at the end of the file by applying the REGISTER_CHECKER() macro.

This check is part of the UnixAPIPortability checker, which is already registered. Do you suggest creating a separate checker for this check?

> 5. One should update the documentation and also mention the new check in the release docs to give visibility.

The current documentation for the UnixAPIPortability checker is quite generic (docs/analyazer/checkers.rst) and doesn’t mention specific checks. Should I mention this new check there?

> 1. Add tests demonstrating all possible aspects of your change.

I added a number of tests for various aspects of the new check, but I’m not sure they are sufficient and I’m not sure what needs to be covered. For instance, should I add test that the warning is not issued for functions not located in the global namespace? Should I add a test case for `nullptr`?

A couple of questions regarding the patch itself:

1. Should I cover non-standard (i.e., non ISO C standard) functions from the `printf_s` family? Should I cover non-standard functions like `dprintf`?
2. What is the right way to emit a bug report? I have studied the code base and found several patterns: in some cases, an error node is simply generated, in others a new transition is also added based on the analyzed state. In some cases an expression value is also tracked.
3. Should I try to report all possible bugs, or only report the first one found?
4. What is the correct way to update expected plists for test inputs? I tried to simply replace the existing one (unix-fns.c.plist) with the output I get from executing the first command of unix-fns.c test, but I still get the following error: 

  console
  georgiy.lebedev at georgiy-lebedev llvm-project % build-debug/bin/llvm-lit -sv clang/test/Analysis/unix-fns.c
  llvm-lit: /Users/georgiy.lebedev/Work/llvm-project/llvm/utils/lit/lit/llvm/config.py:484: note: using clang: /Users/georgiy.lebedev/Work/llvm-project/build-debug/bin/clang
  llvm-lit: /Users/georgiy.lebedev/Work/llvm-project/llvm/utils/lit/lit/util.py:439: note: using SDKROOT: '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk'
  FAIL: Clang :: Analysis/unix-fns.c (1 of 1)
  ******************** TEST 'Clang :: Analysis/unix-fns.c' FAILED ********************
  Script:
  --
  : 'RUN: at line 1';   /Users/georgiy.lebedev/Work/llvm-project/build-debug/bin/clang -cc1 -internal-isystem /Users/georgiy.lebedev/Work/llvm-project/build-debug/lib/clang/17/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -triple x86_64-apple-darwin10 -analyzer-checker=core,unix.API,osx.API,optin.portability /Users/georgiy.lebedev/Work/llvm-project/clang/test/Analysis/unix-fns.c -analyzer-output=plist -analyzer-config faux-bodies=true  -fblocks -verify -o /Users/georgiy.lebedev/Work/llvm-project/build-debug/tools/clang/test/Analysis/Output/unix-fns.c.tmp.plist
  : 'RUN: at line 2';   grep -Ev '^[[:space:]]*<string>.* version .*</string>[[:space:]]*$|^[[:space:]]*<string>/.*</string>[[:space:]]*$|^[[:space:]]*<string>.:.*</string>[[:space:]]*$' </Users/georgiy.lebedev/Work/llvm-project/build-debug/tools/clang/test/Analysis/Output/unix-fns.c.tmp.plist | diff -ub /Users/georgiy.lebedev/Work/llvm-project/clang/test/Analysis/Inputs/expected-plists/unix-fns.c.plist -
  : 'RUN: at line 3';   mkdir -p /Users/georgiy.lebedev/Work/llvm-project/build-debug/tools/clang/test/Analysis/Output/unix-fns.c.tmp.dir
  : 'RUN: at line 4';   /Users/georgiy.lebedev/Work/llvm-project/build-debug/bin/clang -cc1 -internal-isystem /Users/georgiy.lebedev/Work/llvm-project/build-debug/lib/clang/17/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -analyzer-checker=core,unix.API,osx.API,optin.portability -analyzer-output=html -analyzer-config faux-bodies=true -fblocks -o /Users/georgiy.lebedev/Work/llvm-project/build-debug/tools/clang/test/Analysis/Output/unix-fns.c.tmp.dir /Users/georgiy.lebedev/Work/llvm-project/clang/test/Analysis/unix-fns.c
  : 'RUN: at line 5';   rm -fR /Users/georgiy.lebedev/Work/llvm-project/build-debug/tools/clang/test/Analysis/Output/unix-fns.c.tmp.dir
  --
  Exit Code: 1

  Command Output (stdout):
  --
  --- /Users/georgiy.lebedev/Work/llvm-project/clang/test/Analysis/Inputs/expected-plists/unix-fns.c.plist	2023-07-08 19:58:06
  +++ -	2023-07-08 19:58:20
  @@ -3,7 +3,6 @@
   <plist version="1.0">
   <dict>
    <key>clang_version</key>
  -<string>clang version 17.0.0 (git at github.com:llvm/llvm-project.git 1882a4ee69b3cc2202d8d7d7b6465475f6d19886)</string>
    <key>diagnostics</key>
    <array>
     <dict>
  @@ -4065,7 +4064,6 @@
    </array>
    <key>files</key>
    <array>
  -  <string>/Users/georgiy.lebedev/Work/llvm-project/clang/test/Analysis/unix-fns.c</string>
    </array>
   </dict>
   </plist>

  --

  ********************
  ********************
  Failed Tests (1):
    Clang :: Analysis/unix-fns.c

  Testing Time: 0.79s
    Failed: 1

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154838