[PATCH] D83407: [analyzer][StdLibraryFunctionsChecker] Add POSIX networking functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1735-1747
+      if (!addToFunctionSummaryMap(
+              "accept", Summary(ArgTypes{IntTy, *StructSockaddrPtrRestrictTy,
+                                         *Socklen_tPtrRestrictTy},
+                                RetType{IntTy}, NoEvalCall)
+                            .ArgConstraint(ArgumentCondition(
+                                0, WithinRange, Range(0, IntMax)))))
+        // Do not add constraints on sockaddr.
----------------
martong wrote:
> Szelethus wrote:
> > This is quite a bit of code duplication -- if we failed to match the precise `accept`, we just try again with the middle argument type being unspecified? Whats the difference between this and trying to match with `Irrelevant` straight away?
> >  if we failed to match the precise accept, we just try again with the middle argument type being unspecified?
> Yes. And that will be a successful match in the case where `sockaddr` is a transparent union.
> 
> > Whats the difference between this and trying to match with Irrelevant straight away?
> Yeah, it's a good catch. There is no difference with `accept` because we don't have any constraints on `sockaddr` anyway. But, the other functions have fewer constraints with the transparent union.
> Perhaps we should separate the body of the `Summary` from the `Signature` and in this case we could reuse that for both signatures. Or as you suggested we could just match by default with the `Irrelevant`. What do you think?
> Perhaps we should separate the body of the Summary from the Signature and in this case we could reuse that for both signatures. Or as you suggested we could just match by default with the Irrelevant. What do you think?

Separating the summary and the signature sounds great! But, that also adds sufficiently complex code that would definitely warrant individual tests, so we should add that as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83407