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_winalloca(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_winstrdup(nullptr), II_kmalloc(nullptr),
+        II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
+        II_wcsdup(nullptr), II_winwcsdup(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_winalloca, *II_malloc, *II_free,
+                         *II_realloc, *II_calloc, *II_valloc, *II_reallocf,
+                         *II_strndup, *II_strdup, *II_winstrdup, *II_kmalloc,
+                         *II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
+                         *II_winwcsdup;
   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_winstrdup = &Ctx.Idents.get("_strdup");
+  II_winwcsdup = &Ctx.Idents.get("_wcsdup");
+  II_winalloca = &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_winstrdup || FunI == II_strndup || FunI == II_wcsdup ||
+          FunI == II_winwcsdup || 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_winalloca)
         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_winstrdup ||
+               FunI == II_wcsdup || FunI == II_winwcsdup) {
       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_winalloca) {
       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.