[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

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

Szelethus added a comment.

Post-commit LGTM on the post-commit changes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Actually, the test added here could have catched that regression.

Correction: It is the new BufferSize argument constraint in `fread`'s summary 
and the existing test `test_fread_uninitialized` in `std-c-library-functions.c` 
that could have catched that regression. (Not the test in 
`std-c-library-functions-vs-stream-checker.c`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa012bc4c42e4: [analyzer][StdLibraryFunctionsChecker] 
Elaborate the summary of fread and fwrite (authored by martong).

Changed prior to commit:
  https://reviews.llvm.org/D87081?vs=291606=291919#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions-vs-stream-checker.c

Index: clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
@@ -0,0 +1,58 @@
+// Check the case when only the StreamChecker is enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.unix.Stream \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify=stream
+
+// Check the case when only the StdLibraryFunctionsChecker is enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify=stdLib 2>&1 | FileCheck %s
+
+// Check the case when both the StreamChecker and the
+// StdLibraryFunctionsChecker are enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.unix.Stream \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify=both 2>&1 | FileCheck %s
+
+// Verify that the summaries are loaded when the StdLibraryFunctionsChecker is
+// enabled.
+//  CHECK: Loaded summary for: int getchar()
+// CHECK-NEXT: Loaded summary for: unsigned long fread(void *restrict, size_t, size_t, FILE *restrict)
+// CHECK-NEXT: Loaded summary for: unsigned long fwrite(const void *restrict, size_t, size_t, FILE *restrict)
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+
+void test_fread_fwrite(FILE *fp, int *buf) {
+  fp = fopen("foo", "r");
+  if (!fp)
+return;
+  size_t x = fwrite(buf, sizeof(int), 10, fp);
+
+  clang_analyzer_eval(x <= 10); // \
+ // stream-warning{{TRUE}} \
+ // stdLib-warning{{TRUE}} \
+ // both-warning{{TRUE}} \
+
+  clang_analyzer_eval(x == 10); // \
+  // stream-warning{{TRUE}} \
+  // stream-warning{{FALSE}} \
+  // stdLib-warning{{UNKNOWN}} \
+  // both-warning{{TRUE}} \
+  // both-warning{{FALSE}}
+
+  fclose(fp);
+}
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -194,6 +194,22 @@
 // bugpath-warning{{Function argument constraint is not satisfied}} \
 // bugpath-note{{Function argument constraint is not satisfied}}
 }
+typedef __WCHAR_TYPE__ wchar_t;
+// This is one test case for the ARR38-C SEI-CERT rule.
+void ARR38_C_F(FILE *file) {
+  enum { BUFFER_SIZE = 1024 };
+  wchar_t wbuf[BUFFER_SIZE]; // bugpath-note{{'wbuf' initialized here}}
+
+  const size_t size = sizeof(*wbuf);
+  const size_t nitems = sizeof(wbuf);
+
+  // The 3rd parameter should be the number of elements to read, not
+  // the size in bytes.
+  fread(wbuf, size, nitems, file); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+}
 
 int __two_constrained_args(int, int);
 void test_constraints_on_multiple_args(int x, int y) {
Index: clang/test/Analysis/analyzer-enabled-checkers.c
===
--- clang/test/Analysis/analyzer-enabled-checkers.c
+++ clang/test/Analysis/analyzer-enabled-checkers.c
@@ -6,11 +6,11 @@
 
 // CHECK:  OVERVIEW: Clang Static Analyzer Enabled Checkers List
 // CHECK-EMPTY:
+// CHECK-NEXT: core.CallAndMessageModeling
 // CHECK-NEXT: apiModeling.StdCLibraryFunctions
 // CHECK-NEXT: apiModeling.TrustNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: 

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

There is a blatant regression we introduced in D87240 
. Actually, the test added here could have 
catched that regression.
See https://reviews.llvm.org/D87240#inline-812062
I am putting back the modeling dependency with this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

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

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

@balazske may have some closing words.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1262
Getline(LongLongTy, LongLongMax)});
+  // FIXME getdelim's signature is differant than getline's!
   addToFunctionSummaryMap("getdelim",

differant->different


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 291606.
martong added a comment.

- Add tests to verify compatibility of the two checkers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/std-c-library-functions-arg-constraints.c
  clang/test/Analysis/std-c-library-functions-vs-stream-checker.c

Index: clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-vs-stream-checker.c
@@ -0,0 +1,58 @@
+// Check the case when only the StreamChecker is enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.unix.Stream \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify=stream
+
+// Check the case when only the StdLibraryFunctionsChecker is enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify=stdLib 2>&1 | FileCheck %s
+
+// Check the case when both the StreamChecker and the
+// StdLibraryFunctionsChecker are enabled.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.unix.Stream \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify=both 2>&1 | FileCheck %s
+
+// Verify that the summaries are loaded when the StdLibraryFunctionsChecker is
+// enabled.
+//  CHECK: Loaded summary for: int getchar()
+// CHECK-NEXT: Loaded summary for: unsigned long fread(void *restrict, size_t, size_t, FILE *restrict)
+// CHECK-NEXT: Loaded summary for: unsigned long fwrite(const void *restrict, size_t, size_t, FILE *restrict)
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+
+void test_fread_fwrite(FILE *fp, int *buf) {
+  fp = fopen("foo", "r");
+  if (!fp)
+return;
+  size_t x = fwrite(buf, sizeof(int), 10, fp);
+
+  clang_analyzer_eval(x <= 10); // \
+ // stream-warning{{TRUE}} \
+ // stdLib-warning{{TRUE}} \
+ // both-warning{{TRUE}} \
+
+  clang_analyzer_eval(x == 10); // \
+  // stream-warning{{TRUE}} \
+  // stream-warning{{FALSE}} \
+  // stdLib-warning{{UNKNOWN}} \
+  // both-warning{{TRUE}} \
+  // both-warning{{FALSE}}
+
+  fclose(fp);
+}
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -194,6 +194,22 @@
 // bugpath-warning{{Function argument constraint is not satisfied}} \
 // bugpath-note{{Function argument constraint is not satisfied}}
 }
+typedef __WCHAR_TYPE__ wchar_t;
+// This is one test case for the ARR38-C SEI-CERT rule.
+void ARR38_C_F(FILE *file) {
+  enum { BUFFER_SIZE = 1024 };
+  wchar_t wbuf[BUFFER_SIZE]; // bugpath-note{{'wbuf' initialized here}}
+
+  const size_t size = sizeof(*wbuf);
+  const size_t nitems = sizeof(wbuf);
+
+  // The 3rd parameter should be the number of elements to read, not
+  // the size in bytes.
+  fread(wbuf, size, nitems, file); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+}
 
 int __two_constrained_args(int, int);
 void test_constraints_on_multiple_args(int x, int y) {
Index: clang/test/Analysis/Inputs/system-header-simulator.h
===
--- clang/test/Analysis/Inputs/system-header-simulator.h
+++ clang/test/Analysis/Inputs/system-header-simulator.h
@@ -46,8 +46,8 @@
 FILE *tmpfile(void);
 FILE *freopen(const char *pathname, const char *mode, FILE *stream);
 int fclose(FILE *fp);
-size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
-size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
+size_t fread(void *restrict, size_t, size_t, FILE *restrict);
+size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
 int fputc(int ch, FILE *stream);
 int fseek(FILE *__stream, long int __off, int __whence);
 long int ftell(FILE *__stream);
Index: 

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D87081#2258637 , @Szelethus wrote:

> The patch looks great, in fact, it demonstrates how well thought out your 
> summary crafting machinery is.
>
> In D87081#2258579 , @martong wrote:
>
>> However, in a similar case with the CallAndMessage Checker, we decided to 
>> list the more specific Checker as a dependency.
>
> We got the answer to D77061#2057063 
> ! We should turn it into a weak 
> dependency though (D80905 ).
>
> In D87081#2256636 , @balazske wrote:
>
>> This checker will make an additional assumption on `fread` and `fwrite` with 
>> the ReturnValueCondition. The return value is constrained by `StreamChecker` 
>> too but it splits the error (if returned value is less that arg 3) and 
>> non-error cases into separate branches. I think this causes no problem 
>> because it will refine the assumption made here (if this assumption is made 
>> first) or the assumption here has no effect (if the split happened already).
>
> Be sure to triple check whether the `ExplodedGraph` looks okay with both 
> checkers enabled.

I'll try to create tests that check the state in both order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

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

Szelethus added a comment.

The patch looks great, in fact, it demonstrates how well thought out your 
summary crafting machinery is.

In D87081#2258579 , @martong wrote:

> However, in a similar case with the CallAndMessage Checker, we decided to 
> list the more specific Checker as a dependency.

We got the answer to D77061#2057063 ! 
We should turn it into a weak dependency though (D80905 
).

In D87081#2256636 , @balazske wrote:

> This checker will make an additional assumption on `fread` and `fwrite` with 
> the ReturnValueCondition. The return value is constrained by `StreamChecker` 
> too but it splits the error (if returned value is less that arg 3) and 
> non-error cases into separate branches. I think this causes no problem 
> because it will refine the assumption made here (if this assumption is made 
> first) or the assumption here has no effect (if the split happened already).

Be sure to triple check whether the `ExplodedGraph` looks okay with both 
checkers enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D87081#2256636 , @balazske wrote:

> This checker will make an additional assumption on `fread` and `fwrite` with 
> the ReturnValueCondition.

There is nothing new in that. This assumption described by the `.Case` has been 
here since the inception of this Checker. This patch does not change it. This 
patch adds two new argument constraints.

> The return value is constrained by `StreamChecker` too but it splits the 
> error (if returned value is less that arg 3) and non-error cases into 
> separate branches. I think this causes no problem because it will refine the 
> assumption made here (if this assumption is made first) or the assumption 
> here has no effect (if the split happened already).

Either way, this is not a problem. However, in a similar case with the 
CallAndMessage Checker, we decided to list the more specific Checker as a 
dependency. We could do that with StreamChecker too, if you think that's better 
that way. But I'd rather keep that as it is now, since as you suggests, it 
works now and will work even after this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

This checker will make an additional assumption on `fread` and `fwrite` with 
the ReturnValueCondition. The return value is constrained by `StreamChecker` 
too but it splits the error (if returned value is less that arg 3) and 
non-error cases into separate branches. I think this causes no problem because 
it will refine the assumption made here (if this assumption is made first) or 
the assumption here has no effect (if the split happened already).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: steakhal, balazske, Szelethus, NoQ, vsavchenko.
Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411, 
dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware, xazax.hun, whisperity.
Herald added a project: clang.
martong requested review of this revision.

Add the BufferSize argument constraint to fread and fwrite. This change
itself makes it possible to discover a security critical case, described
in SEI-CERT ARR38-C.

We also add the not-null constraint on the 3rd arguments.

In this patch, I also remove those lambdas that don't take any
parameters (Fwrite, Fread, Getc), thus making the code better
structured.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87081

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c

Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -194,6 +194,22 @@
 // bugpath-warning{{Function argument constraint is not satisfied}} \
 // bugpath-note{{Function argument constraint is not satisfied}}
 }
+typedef __WCHAR_TYPE__ wchar_t;
+// This is one test case for the ARR38-C SEI-CERT rule.
+void ARR38_C_F(FILE *file) {
+  enum { BUFFER_SIZE = 1024 };
+  wchar_t wbuf[BUFFER_SIZE]; // bugpath-note{{'wbuf' initialized here}}
+
+  const size_t size = sizeof(*wbuf);
+  const size_t nitems = sizeof(wbuf);
+
+  // The 3rd parameter should be the number of elements to read, not
+  // the size in bytes.
+  fread(wbuf, size, nitems, file); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+}
 
 int __two_constrained_args(int, int);
 void test_constraints_on_multiple_args(int x, int y) {
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -989,6 +989,12 @@
   for (const Summary  : Summaries)
 operator()(Name, S);
 }
+// Add the same summary for different names with the Signature explicitly
+// given.
+void operator()(std::vector Names, Signature Sign, Summary Sum) {
+  for (StringRef Name : Names)
+operator()(Name, Sign, Sum);
+}
   } addToFunctionSummaryMap(ACtx, FunctionSummaryMap, DisplayLoadedSummaries);
 
   // Below are helpers functions to create the summaries.
@@ -1030,35 +1036,12 @@
   Optional FilePtrRestrictTy = getRestrictTy(FilePtrTy);
 
   // Templates for summaries that are reused by many functions.
-  auto Getc = [&]() {
-return Summary(ArgTypes{FilePtrTy}, RetType{IntTy}, NoEvalCall)
-.Case({ReturnValueCondition(WithinRange,
-{{EOFv, EOFv}, {0, UCharRangeMax}})});
-  };
   auto Read = [&](RetType R, RangeInt Max) {
 return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
NoEvalCall)
 .Case({ReturnValueCondition(LessThanOrEq, ArgNo(2)),
ReturnValueCondition(WithinRange, Range(-1, Max))});
   };
-  auto Fread = [&]() {
-return Summary(
-   ArgTypes{VoidPtrRestrictTy, SizeTy, SizeTy, FilePtrRestrictTy},
-   RetType{SizeTy}, NoEvalCall)
-.Case({
-ReturnValueCondition(LessThanOrEq, ArgNo(2)),
-})
-.ArgConstraint(NotNull(ArgNo(0)));
-  };
-  auto Fwrite = [&]() {
-return Summary(ArgTypes{ConstVoidPtrRestrictTy, SizeTy, SizeTy,
-FilePtrRestrictTy},
-   RetType{SizeTy}, NoEvalCall)
-.Case({
-ReturnValueCondition(LessThanOrEq, ArgNo(2)),
-})
-.ArgConstraint(NotNull(ArgNo(0)));
-  };
   auto Getline = [&](RetType R, RangeInt Max) {
 return Summary(ArgTypes{Irrelevant, Irrelevant, Irrelevant}, RetType{R},
NoEvalCall)
@@ -1223,19 +1206,45 @@
  0U, WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}})));
 
   // The getc() family of functions that returns either a char or an EOF.
-addToFunctionSummaryMap("getc", Getc());
-addToFunctionSummaryMap("fgetc", Getc());
+  addToFunctionSummaryMap(
+  {"getc", "fgetc"}, Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
+  Summary(NoEvalCall)
+  .Case({ReturnValueCondition(WithinRange,
+  {{EOFv, EOFv}, {0, UCharRangeMax}})}));
   addToFunctionSummaryMap(
   "getchar", Summary(ArgTypes{}, RetType{IntTy}, NoEvalCall)