[analyzer] Improves the logic of GenericTaintChecker identifying stdin.

[MTC]

Summary

GenericTaintChecker can't recognize stdin in some cases. The reason is that if (PtrTy->getPointeeType() == C.getASTContext().getFILEType() does not hold when stdin is encountered.

My platform is ubuntu16.04 64bit, gcc 5.4.0, glibc 2.23. The definition of stdin is as follows:

__BEGIN_NAMESPACE_STD
/* The opaque type of streams.  This is the definition used elsewhere.  */
typedef struct _IO_FILE FILE;
___END_NAMESPACE_STD

  ...

/* The opaque type of streams.  This is the definition used elsewhere.  */
typedef struct _IO_FILE __FILE;   

  ...

/* Standard streams.  */
extern struct _IO_FILE *stdin;      /* Standard input stream.  */
extern struct _IO_FILE *stdout;     /* Standard output stream.  */
extern struct _IO_FILE *stderr;     /* Standard error output stream.  */

The type of stdin is as follows AST:

ElaboratedType 0xc911170'struct _IO_FILE'sugar
`-RecordType 0xc911150'struct _IO_FILE'
 `-CXXRecord 0xc923ff0'_IO_FILE'

C.getASTContext().GetFILEType() is as follows AST:

TypedefType 0xc932710 'FILE' sugar
|-Typedef 0xc9111c0 'FILE'
`-ElaboratedType 0xc911170 'struct _IO_FILE' sugar
  `-RecordType 0xc911150 'struct _IO_FILE'
      `-CXXRecord 0xc923ff0 '_IO_FILE'

So I think it's better to use getCanonicalType().

[zaks.anna]

Fix looks good. Could you add a test case? The analyzer tests define their own stdin and types, so you could add an alternate definition or change the existing one so that it goes through a typedef.

Thanks!

[MTC]

1. taint-tester.c has some tests about stdin, so I only modified the parts of stdin in Inputs/system-header-simulator.h.
2. The C standard does not specify the implementation of FILE, so I continue to use typedef _FILE FILEto define the FILE type.

[zaks.anna]

Thanks! Do you have commit access or should we commit on your behalf?

[MTC]

I do not have commit access and hope someone can commit it on my behalf. Thanks a lot!

[NoQ]

Maybe we could also test both declaration variants, i.e.:

// RUN: %clang_analyze_cc1                  -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify 
// RUN: %clang_analyze_cc1 -DFILE_IS_STRUCT -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify

#ifdef FILE_IS_STRUCT
extern struct _FILE *stdin;
#else
extern FILE *stdin;
#endif

// put tests here

[MTC]

In D39159#910466, @NoQ wrote:

Maybe we could also test both declaration variants, i.e.:

Thanks for your suggestion, Noq. Yes, it's a choice.

But stdin can only have one declaration in system-header-simulator.h, extern FILE stdin or extern _FILE stdin and testing these two declaration in one test file is not compatible with system-header-simulator.h. In either form, getCanonicalType () can handle it. So I don't know whether it's reasonable to test both declaration variants at the same time?

[MTC]

Update the taint-generic.c to test both stdin declaration variants.

[MTC]

@NoQ, Very sorry, I've forgotten about this patch, it has now been updated.

[a.sidorin]

LGTM.

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

bool GenericTaintChecker::isStdin(const Expr *E, CheckerContext &C) {
 
   // This region corresponds to a declaration, find out if it's a global/extern
   // variable named stdin with the proper type.
   if (const VarDecl *D = dyn_cast_or_null<VarDecl>(DeclReg->getDecl())) {
     D = D->getCanonicalDecl();
     if ((D->getName().find("stdin") != StringRef::npos) && D->isExternC())
         if (const PointerType * PtrTy =
               dyn_cast<PointerType>(D->getType().getTypePtr()))
-          if (PtrTy->getPointeeType() == C.getASTContext().getFILEType())
+          if (PtrTy->getPointeeType().getCanonicalType() ==
+              C.getASTContext().getFILEType().getCanonicalType())
             return true;
   }
   return false;
 }
 
 static bool getPrintfFormatArgumentNum(const CallExpr *CE,
                                        const CheckerContext &C,
                                        unsigned int &ArgNum) {

test/Analysis/taint-generic.c

 // RUN: %clang_analyze_cc1  -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -Wno-format-security -verify %s
+// RUN: %clang_analyze_cc1  -DFILE_IS_STRUCT -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -Wno-format-security -verify %s
 
 int scanf(const char *restrict format, ...);
 int getchar(void);
 
 typedef struct _FILE FILE;
+#ifdef FILE_IS_STRUCT
+extern struct _FILE *stdin;
+#else
 extern FILE *stdin;
+#endif
+
 int fscanf(FILE *restrict stream, const char *restrict format, ...);
 int sprintf(char *str, const char *format, ...);
 void setproctitle(const char *fmt, ...);
 typedef __typeof(sizeof(int)) size_t;
 
 // Define string functions. Use builtin for some of them. They all default to
 // the processing in the taint checker.
 #define strcpy(dest, src) \