Fix missed leak from MSVC specific allocation functions

[ariccio]

Summary

I've found & fixed a leak that Clang misses when compiling on Windows.

The leak was found by SARD #149071, mem1-bad.c. Clang misses it because MSVC uses _strdup instead of strdup. I've added an IdentifierInfo* for _strdup, and now Clang catches it.

As a drive by fix, I added the wide character strdup variants (wcsdup, _wcsdup) and the MSVC version of alloca (_alloca).

No new check-all failures. (Did somebody improve lit's output?) Adding reviewers...

[zaks.anna]

The two underscores in the names are hard to read. Maybe we should just prefix them with 'win'?

[ariccio]

In D17688#363835, @zaks.anna wrote:

The two underscores in the names are hard to read. Maybe we should just prefix them with 'win'?

I like that better, will change.

[ariccio]

(This is mostly for my own reference)
BTW, there are a few other non-MS-only functions in the C standard library that allocate memory that needs to be free()d:

1. getcwd (and _getcwd/_wgetcwd)

And some MS-specific functions that I'm surprised are actually MS-only:

1. _getdcwd/_wgetdcwd
2. _dupenv_s/_wdupenv_s
3. _fullpath/_wfullpath

[ariccio]

Changed underscore prefixed variable names to win prefixed variable names.

[zaks.anna]

Nit: Could you change the prefix from "win" to "win_"?

[ariccio]

Nit addressed.

[ariccio]

Is this patch all clear to go?

I hope I don't sound too pushy - I just don't want to lose momentum here.

[zaks.anna]

I suggest to update the malloc regression test with these.

[ariccio]

In D17688#365880, @zaks.anna wrote:

I suggest to update the malloc regression test with these.

Eh? There's a malloc regression test?

[zaks.anna]

Of cause, we have regression tests for (almost) everything:
ls ./clang/test/Analysis/malloc*

[ariccio]

In D17688#365951, @zaks.anna wrote:

ls ./clang/test/Analysis/malloc*

Ah, ok. Would it be ok if (for _strdup & _alloca) I just do this at the beginning:

#if defined(_WIN32)

#define strdup _strdup
#define alloca _alloca

#endif //defined(_WIN32)

[ariccio]

In D17688#366011, @ariccio wrote:

In D17688#365951, @zaks.anna wrote:

ls ./clang/test/Analysis/malloc*

Ah, ok. Would it be ok if (for _strdup & _alloca) I just do this at the beginning:

#if defined(_WIN32)

#define strdup _strdup
#define alloca _alloca

#endif //defined(_WIN32)

Nevermind. I'm uploading a proper patch in the next few minutes.

[ariccio]

Added newly checked variants to the malloc checks.

(Running the check-all suite now, will update with results)

[zaks.anna]

LGTM. Thanks!

I can commit this in your behalf.

[ariccio]

Hmm, check-all produced a number of issues, which I think stem from the following warning:

File C:\LLVM\llvm\tools\clang\test\Analysis\malloc.c Line 16: unknown type name 'wchar_t'

(with a bunch of instances of that)

...so how do I declare/define wchar_t properly?

[zaks.anna]

Generate a preprocessed file and keep copying the definitions from there until you reach the types compiler knows about.

[zaks.anna]

Please, do not submit patches for review before they pass all tests.

[ariccio]

In D17688#366883, @zaks.anna wrote:

Please, do not submit patches for review before they pass all tests.

Whoops, sorry. Should I been a bit clearer that check-all hadn't finished when I updated the diff, or should I not update the diff at all until check-all finishes?

[zaks.anna]

The second: you should not update the diff. When a patch is uploaded, the assumption is that all tests pass:)

[ariccio]

Hmm. This seems to be because wchar_t is enabled by -fms-compatibility.

@aaron.ballman do you have any idea how to define wchar_t without -fms-compatibility?

[ariccio]

Alrighty. This final version of the patch causes no test failures (vs the unmodified build*).

*An unrelated test normally fails on my machine.

[zaks.anna]

[ariccio]

In D17688#368330, @zaks.anna wrote:

?

[ariccio]

In D17688#366780, @zaks.anna wrote:

LGTM. Thanks!

I can commit this in your behalf.

Oh, and yeah, I don't have privs.

llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

class MallocChecker : public Checker<check::DeadSymbols,
                                      check::PreStmt<CXXDeleteExpr>,
                                      check::PostStmt<BlockExpr>,
                                      check::PostObjCMessage,
                                      check::Location,
                                      eval::Assume>
 {
 public:
   MallocChecker()
-      : II_alloca(nullptr), II_malloc(nullptr), II_free(nullptr),
-        II_realloc(nullptr), II_calloc(nullptr), II_valloc(nullptr),
-        II_reallocf(nullptr), II_strndup(nullptr), II_strdup(nullptr),
-        II_kmalloc(nullptr), II_if_nameindex(nullptr),
-        II_if_freenameindex(nullptr) {}
+      : II_alloca(nullptr), II_win_alloca(nullptr), II_malloc(nullptr),
+        II_free(nullptr), II_realloc(nullptr), II_calloc(nullptr),
+        II_valloc(nullptr), II_reallocf(nullptr), II_strndup(nullptr),
+        II_strdup(nullptr), II_win_strdup(nullptr), II_kmalloc(nullptr),
+        II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
+        II_wcsdup(nullptr), II_win_wcsdup(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
   enum CheckKind {
     CK_MallocChecker,
     CK_NewDeleteChecker,
     CK_NewDeleteLeaksChecker,
     CK_MismatchedDeallocatorChecker,
private:
   mutable std::unique_ptr<BugType> BT_DoubleDelete;
   mutable std::unique_ptr<BugType> BT_Leak[CK_NumCheckKinds];
   mutable std::unique_ptr<BugType> BT_UseFree[CK_NumCheckKinds];
   mutable std::unique_ptr<BugType> BT_BadFree[CK_NumCheckKinds];
   mutable std::unique_ptr<BugType> BT_FreeAlloca[CK_NumCheckKinds];
   mutable std::unique_ptr<BugType> BT_MismatchedDealloc;
   mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr<BugType> BT_UseZerroAllocated[CK_NumCheckKinds];
-  mutable IdentifierInfo *II_alloca, *II_malloc, *II_free, *II_realloc,
-                         *II_calloc, *II_valloc, *II_reallocf, *II_strndup,
-                         *II_strdup, *II_kmalloc, *II_if_nameindex,
-                         *II_if_freenameindex;
+  mutable IdentifierInfo *II_alloca, *II_win_alloca, *II_malloc, *II_free,
+                         *II_realloc, *II_calloc, *II_valloc, *II_reallocf,
+                         *II_strndup, *II_strdup, *II_win_strdup, *II_kmalloc,
+                         *II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
+                         *II_win_wcsdup;
   mutable Optional<uint64_t> KernelZeroFlagVal;
 
   void initIdentifierInfo(ASTContext &C) const;
 
   /// \brief Determine family of a deallocation expression.
   AllocationFamily getAllocationFamily(CheckerContext &C, const Stmt *S) const;
 
   /// \brief Print names of allocators and deallocators.
void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const {
   II_malloc = &Ctx.Idents.get("malloc");
   II_free = &Ctx.Idents.get("free");
   II_realloc = &Ctx.Idents.get("realloc");
   II_reallocf = &Ctx.Idents.get("reallocf");
   II_calloc = &Ctx.Idents.get("calloc");
   II_valloc = &Ctx.Idents.get("valloc");
   II_strdup = &Ctx.Idents.get("strdup");
   II_strndup = &Ctx.Idents.get("strndup");
+  II_wcsdup = &Ctx.Idents.get("wcsdup");
   II_kmalloc = &Ctx.Idents.get("kmalloc");
   II_if_nameindex = &Ctx.Idents.get("if_nameindex");
   II_if_freenameindex = &Ctx.Idents.get("if_freenameindex");
+  
+  //MSVC uses `_`-prefixed instead, so we check for them too.
+  II_win_strdup = &Ctx.Idents.get("_strdup");
+  II_win_wcsdup = &Ctx.Idents.get("_wcsdup");
+  II_win_alloca = &Ctx.Idents.get("_alloca");
 }
 
 bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const {
   if (isCMemFunction(FD, C, AF_Malloc, MemoryOperationKind::MOK_Any))
     return true;
 
   if (isCMemFunction(FD, C, AF_IfNameIndex, MemoryOperationKind::MOK_Any))
     return true;
if (FD->getKind() == Decl::Function) {
     if (Family == AF_Malloc && CheckFree) {
       if (FunI == II_free || FunI == II_realloc || FunI == II_reallocf)
         return true;
     }
 
     if (Family == AF_Malloc && CheckAlloc) {
       if (FunI == II_malloc || FunI == II_realloc || FunI == II_reallocf ||
           FunI == II_calloc || FunI == II_valloc || FunI == II_strdup ||
-          FunI == II_strndup || FunI == II_kmalloc)
+          FunI == II_win_strdup || FunI == II_strndup || FunI == II_wcsdup ||
+          FunI == II_win_wcsdup || FunI == II_kmalloc)
         return true;
     }
 
     if (Family == AF_IfNameIndex && CheckFree) {
       if (FunI == II_if_freenameindex)
         return true;
     }
 
     if (Family == AF_IfNameIndex && CheckAlloc) {
       if (FunI == II_if_nameindex)
         return true;
     }
 
     if (Family == AF_Alloca && CheckAlloc) {
-      if (FunI == II_alloca)
+      if (FunI == II_alloca || FunI == II_win_alloca)
         return true;
     }
   }
 
   if (Family != AF_Malloc)
     return false;
 
   if (IsOptimistic && FD->hasAttrs()) {
if (FunI == II_malloc) {
       State = ReallocMem(C, CE, true, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
     } else if (FunI == II_calloc) {
       State = CallocMem(C, CE, State);
       State = ProcessZeroAllocation(C, CE, 0, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
     } else if (FunI == II_free) {
       State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
-    } else if (FunI == II_strdup) {
+    } else if (FunI == II_strdup || FunI == II_win_strdup ||
+               FunI == II_wcsdup || FunI == II_win_wcsdup) {
       State = MallocUpdateRefState(C, CE, State);
     } else if (FunI == II_strndup) {
       State = MallocUpdateRefState(C, CE, State);
-    } else if (FunI == II_alloca) {
+    } else if (FunI == II_alloca || FunI == II_win_alloca) {
       State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
                            AF_Alloca);
       State = ProcessZeroAllocation(C, CE, 0, State);
     } else if (isStandardNewDelete(FD, C.getASTContext())) {
       // Process direct calls to operator new/new[]/delete/delete[] functions
       // as distinct from new/new[]/delete/delete[] expressions that are
       // processed by the checkPostStmt callbacks for CXXNewExpr and
       // CXXDeleteExpr.

llvm/tools/clang/test/Analysis/malloc.c

 // RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
 
 #include "Inputs/system-header-simulator.h"
 
 void clang_analyzer_eval(int);
 
+// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
+// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
+// the builtin type: "Using the typedef version can cause portability
+// problems", but we're ok here because we're not actually running anything.
+// Also of note is this cryptic warning: "The wchar_t type is not supported
+// when you compile C code".
+//
+// See the docs for more:
+// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
+#if !defined(_WCHAR_T_DEFINED)
+// "Microsoft implements wchar_t as a two-byte unsigned value"
+typedef unsigned short wchar_t;
+#define _WCHAR_T_DEFINED
+#endif // !defined(_WCHAR_T_DEFINED)
+
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void *alloca(size_t);
 void *valloc(size_t);
 void free(void *);
 void *realloc(void *ptr, size_t size);
 void *reallocf(void *ptr, size_t size);
 void *calloc(size_t nmemb, size_t size);
 char *strdup(const char *s);
+wchar_t *wcsdup(const wchar_t *s);
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 
+// Windows variants
+char *_strdup(const char *strSource);
+wchar_t *_wcsdup(const wchar_t *strSource);
+void *_alloca(size_t size);
+
 void myfoo(int *p);
 void myfooint(int p);
 char *fooRetPtr();
 
 void f1() {
   int *p = malloc(12);
   return; // expected-warning{{Potential leak of memory pointed to by 'p'}}
 }
if (p) {
     char x = *q; // expected-warning {{Potential leak of memory pointed to by 'q'}}
   }
 }
 
 void allocaTest() {
   int *p = alloca(sizeof(int));
 } // no warn
 
+void winAllocaTest() {
+  int *p = _alloca(sizeof(int));
+} // no warn
+
 void allocaBuiltinTest() {
   int *p = __builtin_alloca(sizeof(int));
 } // no warn
 
 int *realloctest1() {
   int *q = malloc(12);
   q = realloc(q, 20);
   return q; // no warning - returning the allocated value
void CheckUseZeroAllocatedNoWarn1() {
   int *p = malloc(0);
   free(p); // no warning
 }
 
 void CheckUseZeroAllocatedNoWarn2() {
   int *p = alloca(0); // no warning
 }
 
+void CheckUseZeroWinAllocatedNoWarn2() {
+  int *p = _alloca(0); // no warning
+}
+
+
 void CheckUseZeroAllocatedNoWarn3() {
   int *p = malloc(0);
   int *q = realloc(p, 8); // no warning
   free(q);
 }
 
 void CheckUseZeroAllocatedNoWarn4() {
   int *p = realloc(0, 8);
   *p = 1; // no warning
   free(p);
 }
 
 void CheckUseZeroAllocated1() {
   int *p = malloc(0);
   *p = 1; // expected-warning {{Use of zero-allocated memory}}
   free(p);
 }
 
 char CheckUseZeroAllocated2() {
   char *p = alloca(0);
   return *p; // expected-warning {{Use of zero-allocated memory}}
 }
 
+char CheckUseZeroWinAllocated2() {
+  char *p = _alloca(0);
+  return *p; // expected-warning {{Use of zero-allocated memory}}
+}
+
 void UseZeroAllocated(int *p) {
   if (p)
     *p = 7; // expected-warning {{Use of zero-allocated memory}}
 }
 void CheckUseZeroAllocated3() {
   int *p = malloc(0);
   UseZeroAllocated(p);
 }

 }
 
 // Test various allocation/deallocation functions.
 void testStrdup(const char *s, unsigned validIndex) {
   char *s2 = strdup(s);
   s2[validIndex + 1] = 'b';
 } // expected-warning {{Potential leak of memory pointed to by}}
 
+void testWinStrdup(const char *s, unsigned validIndex) {
+  char *s2 = _strdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWcsdup(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = wcsdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWcsdup(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = _wcsdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
 int testStrndup(const char *s, unsigned validIndex, unsigned size) {
   char *s2 = strndup(s, size);
   s2 [validIndex + 1] = 'b';
   if (s2[validIndex] != 'a')
     return 0;
   else
     return 1;// expected-warning {{Potential leak of memory pointed to by}}
 }
 
 void testStrdupContentIsDefined(const char *s, unsigned validIndex) {
   char *s2 = strdup(s);
   char result = s2[1];// no warning
   free(s2);
 }
 
+void testWinStrdupContentIsDefined(const char *s, unsigned validIndex) {
+  char *s2 = _strdup(s);
+  char result = s2[1];// no warning
+  free(s2);
+}
+
+void testWcsdupContentIsDefined(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = wcsdup(s);
+  wchar_t result = s2[1];// no warning
+  free(s2);
+}
+
+void testWinWcsdupContentIsDefined(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = _wcsdup(s);
+  wchar_t result = s2[1];// no warning
+  free(s2);
+}
+
 // ----------------------------------------------------------------------------
 // Test the system library functions to which the pointer can escape.
 // This tests false positive suppression.
 
 // For now, we assume memory passed to pthread_specific escapes.
 // TODO: We could check that if a new pthread binding is set, the existing
 // binding must be freed; otherwise, a memory leak can occur.
 void testPthereadSpecificEscape(pthread_key_t key) {
void *test(void *ptr) {
   return newPtr;
 }
 
 
 char *testLeakWithinReturn(char *str) {
   return strdup(strdup(str)); // expected-warning{{leak}}
 }
 
+char *testWinLeakWithinReturn(char *str) {
+  return _strdup(_strdup(str)); // expected-warning{{leak}}
+}
+
+wchar_t *testWinWideLeakWithinReturn(wchar_t *str) {
+  return _wcsdup(_wcsdup(str)); // expected-warning{{leak}}
+}
+
 void passConstPtr(const char * ptr);
 
 void testPassConstPointer() {
   char * string = malloc(sizeof(char)*10);
   passConstPtr(string);
   return; // expected-warning {{leak}}
 }