[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D71524#2304742 <https://reviews.llvm.org/D71524#2304742>, @boga95 wrote:

> As far as I remember I tried to make `std::cin` tainted, but it was complicated.

What sort of issues did you find by implementing that approach? Could you elaborate?

> I run the checker against many projects and there wasn't any false positive related to this heuristic.

It seems a bit hand-wavy to me.

> We can restrict the `operator>>`  to `std::basic_stream` and cover only the standard library.

If we choose this approach, then we have to do something about this, yes. This could be a reasonable choice.

We are arguing about whether the `operator>>` is a //propagation// or a //source// function.
IMO this operator should //propagate// taint, from the //stream// to an object.

I've opened the cppreference <https://en.cppreference.com/w/cpp/io/manip/setw> to find an example where your logic would introduce serious false positives.
I've spent like 2 minutes to come up with this, there might be many more:

  std::istringstream is("hello, world");
  char arr[10];
  is >> std::setw(6) >> arr;
  std::cout << "Input from \"" << is.str() << "\" with setw(6) gave \""
            << arr << "\"\n";

godbolt <https://godbolt.org/z/K7zeKd>

You would deliberately mark `arr` as a tainted.
Fortunately/unfortunately it's quite hard to //remove// taint - so any unnecessarily tainted value is a serious problem, thus requires careful design decisions.

This example might seem to be a made-up one, however, we should consider developers who want to extract data from streams - which aren't tainted.
Marking the stream itself tainted, and propagating taint over the //extraction operator// would not have such false positives.


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

https://reviews.llvm.org/D71524