<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body><div><div style="font-family: Calibri,sans-serif; font-size: 11pt;">> I just don't understand why some functions made it in this patch and not others (notably, why the lack of _mbsdup, which is documented on the same page as others you are adding).<br><br>It's partly because of the better wrapper class that was mentioned (in a discussion that I'm having trouble finding), and partly because I highly prefer locking in small changes rather than repeatedly growing patches. <br><br>Personally, the more I grow a patch, the less likely it is that I'll ever actually get it accepted. I think this has a bit to do with using svn instead of git.<br><br>> By "here", do you mean in this review thread, or as part of this section of code?<br><br>Whoops. I meant in this thread or an email, but I don't actually remembering why I asked this in the first place. I'll just whack some of the easiest ones in, and we'll go from there.<br><br>I'll need to separately implement the aligned versions, because they shouldn't be mixed with the normal versions.<br><br><br>sent from my (stupid) windows phone</div></div><div dir="ltr"><hr><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">From: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;"><a href="mailto:aaron.ballman@gmail.com">Aaron Ballman</a></span><br><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">Sent: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;">3/1/2016 3:33 PM</span><br><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">To: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;"><a href="mailto:alexander@riccio.com"><Alexander G. Riccio></a></span><br><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">Cc: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;"><a href="mailto:reviews+D17688+public+c9d6a65abf2d2bf2@reviews.llvm.org">reviews+D17688+public+c9d6a65abf2d2bf2@reviews.llvm.org</a>; <a href="mailto:dcoughlin@apple.com">Devin Coughlin</a>; <a href="mailto:zaks.anna@gmail.com">Anna Zaks</a>; <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits</a></span><br><span style="font-family: Calibri,sans-serif; font-size: 11pt; font-weight: bold;">Subject: </span><span style="font-family: Calibri,sans-serif; font-size: 11pt;">Re: [PATCH] D17688: Fix missed leak from MSVC specific allocationfunctions</span><br><br></div>On Tue, Mar 1, 2016 at 1:42 PM, <Alexander G. Riccio><br><alexander@riccio.com> wrote:<br>> I'd quite happily add them... but can I do it in another patch? I think I<br>> could be more thorough that way.<br><br>I'm not certain I understand the reasoning, but I also don't have<br>strong feelings on whether it's this patch or another. I just don't<br>understand why some functions made it in this patch and not others<br>(notably, why the lack of _mbsdup, which is documented on the same<br>page as others you are adding).<br><br>> For the same reason, can we list all the microsoft memory allocating<br>> routines here? There are a thousand routines we might want to add, and then<br>> a few others (like _dupenv_s, _malloca, and _expand) which are especially<br>> important to be able to analyze (because they have really tricky APIs), but<br>> because of their kooky APIs, they're harder to implement checkers for.<br><br>By "here", do you mean in this review thread, or as part of this<br>section of code?<br><br>~Aaron<br><br>><br>> Sincerely,<br>> Alexander Riccio<br>> --<br>> "Change the world or go home."<br>> about.me/ariccio<br>><br>> If left to my own devices, I will build more.<br>> ⁂<br>><br>> On Tue, Mar 1, 2016 at 8:38 AM, Aaron Ballman <aaron.ballman@gmail.com><br>> wrote:<br>>><br>>> On Tue, Mar 1, 2016 at 2:16 AM, Alexander Riccio <alexander@riccio.com><br>>> wrote:<br>>> > ariccio updated this revision to Diff 49456.<br>>> > ariccio added a comment.<br>>> ><br>>> > Nit addressed.<br>>> ><br>>> ><br>>> > http://reviews.llvm.org/D17688<br>>> ><br>>> > Files:<br>>> > llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp<br>>> ><br>>> > Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp<br>>> > ===================================================================<br>>> > --- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp<br>>> > +++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp<br>>> > @@ -169,11 +169,12 @@<br>>> > {<br>>> > public:<br>>> > MallocChecker()<br>>> > - : II_alloca(nullptr), II_malloc(nullptr), II_free(nullptr),<br>>> > - II_realloc(nullptr), II_calloc(nullptr), II_valloc(nullptr),<br>>> > - II_reallocf(nullptr), II_strndup(nullptr), II_strdup(nullptr),<br>>> > - II_kmalloc(nullptr), II_if_nameindex(nullptr),<br>>> > - II_if_freenameindex(nullptr) {}<br>>> > + : II_alloca(nullptr), II_win_alloca(nullptr), II_malloc(nullptr),<br>>> > + II_free(nullptr), II_realloc(nullptr), II_calloc(nullptr),<br>>> > + II_valloc(nullptr), II_reallocf(nullptr), II_strndup(nullptr),<br>>> > + II_strdup(nullptr), II_win_strdup(nullptr),<br>>> > II_kmalloc(nullptr),<br>>> > + II_if_nameindex(nullptr), II_if_freenameindex(nullptr),<br>>> > + II_wcsdup(nullptr), II_win_wcsdup(nullptr) {}<br>>> ><br>>> > /// In pessimistic mode, the checker assumes that it does not know<br>>> > which<br>>> > /// functions might free the memory.<br>>> > @@ -231,10 +232,11 @@<br>>> > mutable std::unique_ptr<BugType> BT_MismatchedDealloc;<br>>> > mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds];<br>>> > mutable std::unique_ptr<BugType><br>>> > BT_UseZerroAllocated[CK_NumCheckKinds];<br>>> > - mutable IdentifierInfo *II_alloca, *II_malloc, *II_free, *II_realloc,<br>>> > - *II_calloc, *II_valloc, *II_reallocf,<br>>> > *II_strndup,<br>>> > - *II_strdup, *II_kmalloc, *II_if_nameindex,<br>>> > - *II_if_freenameindex;<br>>> > + mutable IdentifierInfo *II_alloca, *II_win_alloca, *II_malloc,<br>>> > *II_free,<br>>> > + *II_realloc, *II_calloc, *II_valloc,<br>>> > *II_reallocf,<br>>> > + *II_strndup, *II_strdup, *II_win_strdup,<br>>> > *II_kmalloc,<br>>> > + *II_if_nameindex, *II_if_freenameindex,<br>>> > *II_wcsdup,<br>>> > + *II_win_wcsdup;<br>>> > mutable Optional<uint64_t> KernelZeroFlagVal;<br>>> ><br>>> > void initIdentifierInfo(ASTContext &C) const;<br>>> > @@ -540,9 +542,15 @@<br>>> > II_valloc = &Ctx.Idents.get("valloc");<br>>> > II_strdup = &Ctx.Idents.get("strdup");<br>>> > II_strndup = &Ctx.Idents.get("strndup");<br>>> > + II_wcsdup = &Ctx.Idents.get("wcsdup");<br>>> > II_kmalloc = &Ctx.Idents.get("kmalloc");<br>>> > II_if_nameindex = &Ctx.Idents.get("if_nameindex");<br>>> > II_if_freenameindex = &Ctx.Idents.get("if_freenameindex");<br>>> > +<br>>> > + //MSVC uses `_`-prefixed instead, so we check for them too.<br>>> > + II_win_strdup = &Ctx.Idents.get("_strdup");<br>>> > + II_win_wcsdup = &Ctx.Idents.get("_wcsdup");<br>>> > + II_win_alloca = &Ctx.Idents.get("_alloca");<br>>><br>>> What about: _mbsdup, _strdup_dbg, _wcsdup_dbg, _aligned_realloc, and<br>>> the rest? If we're going to add these (which I really support), it<br>>> would be good to make a comprehensive sweep for the Win32 additions<br>>> and add them all.<br>>><br>>> ~Aaron<br>>><br>>> > }<br>>> ><br>>> > bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext<br>>> > &C) const {<br>>> > @@ -585,7 +593,8 @@<br>>> > if (Family == AF_Malloc && CheckAlloc) {<br>>> > if (FunI == II_malloc || FunI == II_realloc || FunI ==<br>>> > II_reallocf ||<br>>> > FunI == II_calloc || FunI == II_valloc || FunI == II_strdup<br>>> > ||<br>>> > - FunI == II_strndup || FunI == II_kmalloc)<br>>> > + FunI == II_win_strdup || FunI == II_strndup || FunI ==<br>>> > II_wcsdup ||<br>>> > + FunI == II_win_wcsdup || FunI == II_kmalloc)<br>>> > return true;<br>>> > }<br>>> ><br>>> > @@ -600,7 +609,7 @@<br>>> > }<br>>> ><br>>> > if (Family == AF_Alloca && CheckAlloc) {<br>>> > - if (FunI == II_alloca)<br>>> > + if (FunI == II_alloca || FunI == II_win_alloca)<br>>> > return true;<br>>> > }<br>>> > }<br>>> > @@ -789,11 +798,12 @@<br>>> > State = ProcessZeroAllocation(C, CE, 1, State);<br>>> > } else if (FunI == II_free) {<br>>> > State = FreeMemAux(C, CE, State, 0, false,<br>>> > ReleasedAllocatedMemory);<br>>> > - } else if (FunI == II_strdup) {<br>>> > + } else if (FunI == II_strdup || FunI == II_win_strdup ||<br>>> > + FunI == II_wcsdup || FunI == II_win_wcsdup) {<br>>> > State = MallocUpdateRefState(C, CE, State);<br>>> > } else if (FunI == II_strndup) {<br>>> > State = MallocUpdateRefState(C, CE, State);<br>>> > - } else if (FunI == II_alloca) {<br>>> > + } else if (FunI == II_alloca || FunI == II_win_alloca) {<br>>> > State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,<br>>> > AF_Alloca);<br>>> > State = ProcessZeroAllocation(C, CE, 0, State);<br>>> ><br>>> ><br>><br>><br></body></html>