[PATCH] D69813: [analyzer] CERTStrChecker: Model gets()

Wed Nov 6 10:09:07 PST 2019

Szelethus added a comment.

In D69813#1734272 <https://reviews.llvm.org/D69813#1734272>, @Charusso wrote:

> In D69813#1734193 <https://reviews.llvm.org/D69813#1734193>, @Szelethus wrote:
>
> > Hmm, so this checker is rather a collection of CERT rule checkers, right? Shouldn't the checker name contain the actual rule name (STR31-C)? User interfacewise, I would much prefer smaller, leaner checkers than a big one with a lot of options, which are barely supported for end-users. I would expect a `cert` package to contain subpackages like `cert.str`, and checker names `cert.str.31StringSize`, or similar.
>
>
> It is the STR rules of CERT, nothing else. Most of the rules are tied together, and that is why the checker needs to be designed as one checker at first. I am not sure which part of the STR I will cover, so may when the checker evolves and some functions does not need the same helper methods we need to create new checkers. STR31 and STR32 are my projects which is like one single project. Also I did not except the users to specify the rule number, but this checker could be something like `cert.str.Termination`. There is two floating-point CERT checkers inside the `insecureAPI` that is why I have introduced the `cert` package, which will have three members, one is that new checker. I think a new package is only necessary if it contains at least two checkers.

Implementationwise that sounds wonderful, these rules are fairly similar to have a single checker responsible for them. The user interface however (enabling different CERT rules) they should probably be split up into separate checkers per rule, rather than options, wouldn't you agree? @NoQ? Also,  `cert.str.Termination` sounds like a wonderful name, I don't insist on the rule number being present in it, but at the very least, it should be in the description.

>> Also, shouldn't we move related checkers from `security.insecureAPI` to `cert`? Or just mention the rule name in the description, and continue not having a  `cert` package?
> 
> We should not, they does not fit into CERT rules, but it has two CERT floating-point checkers. The `cert` package should be well described with CERT rules. I want to move the CERT checkers from it into that `cert` package, and leave the rest.

I meant //related// ones only, though I didn't go through the checkers individually, it might just be the case that `insecureApi` doesn't implement any specific CERT rules :)

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

https://reviews.llvm.org/D69813