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

[Georgiy Lebedev via Phabricator via cfe-commits]

CuriousGeorgiy added a comment.

@steakhal

Thanks for the review comments!

> This line should be just as long, as the line above.

Fixed.

> Our docs aren't great, but we should have a brief description what the checker detects, basically here it would be "Find null pointers being passed to printf-like functions. Usually, passing null pointers to such functions is implementation defined, thus non-portable. Printf-like functions are: x, y, z." 
> Illustrate one example for diagnosing a case. Also illustrate one case similar to the bad one, that is fixed (basically ensure that the pointer is not null there).

Fixed. Added a generic comment about the checker collection, added a check list and described the printf pointer conversion specifier checker and provided TP and TN examples for it.

> I don't think we should create a separate package. A separate checker should be enough in `alpha.unix`, or `alpha.optin`, or just `optin`.

Moved the checker to the `alpha.unix` package.

> Usually, the user-facing checker name which is the <"..."> part there, matches the name of the checker's class name. It helps for maintenance.
> I would strongly suggest keeping this naming convention.

Changed the checker name to `APIPortabilityMinor`, dropping the `Unix` prefix, which seems redundant as it is part of the checker path.

> Usually, we put separate checkers into their own file.

Fixed.

> Prefer using the check::PreCall callback over the PreStmt.

Fixed.

> Nowdays, if I'm not mistaken, we just inline initialize these eagerly.
> That way you could also avoid marking it mutable.

Fixed.

> Feel free to just call it reportBug.

I cannot call it this way, since it is bug reporting function for the printf family pointer conversion specifier specific check.

> Also, in the cpp file, usually we use the namespaces clang,llvm,ento already. You don't need to fully-qualify these names.

Fixed. Though, AFAIC, I still need to qualify the `register` and `shouldRegister` functions with the `ento` namespace, otherwise the ADL fails.

> For matching names, we usually use `CallDescriptions` to do it for us.
> If think you could use them while also checking if the Call is a `CallEvent::isGlobalCFunction()`. I suspect, you only wanna check those.
> I think you should define a `CallDescriptionMap`, because you will also need your specific data_arg_index...
> Look for uses of such maps and you will get inspired.

Thanks a lot for pointing this out, it looks very neat indeed, applied your suggestion, and the checker looks a lot cleaner now.

> I'd highly recommend not touching this file to avoid the bulk change in the expected plist output. Please just introduce a new test file.

Fixed.

> I would appreciate a test demonstrating that the pointer argument won't get constrained after the printf call. I think such case was already discussed.
> We should also demonstrate that unknown pointers (which doesn't known to be neither null, nor non-null) won't raise issues.

Added these test cases to the `printf_pointer_conversion_specifier_null_pointer_constraints` test.

> In the checker-logic, we have the `data_args_index` for each printf-like function.
> Can you demonstrate that each is handled as expected?

AFAIC, I have already done this in the `test_printf_pointer_conversion_specifier_null_various_arguments` test.

> Also have a test when the formatstring itself is null.

AFAIC, passing a null format string is already UB, so the check shouldn't handle this case specially. What test would you like to see?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154838