[PATCH] D17688: Fix missed leak from MSVC specific allocationfunctions

Tue Mar 1 15:46:28 PST 2016

> 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).

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. 

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.

> By "here", do you mean in this review thread, or as part of this section of code?

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.

I'll need to separately implement the aligned versions, because they shouldn't be mixed with the normal versions.

sent from my (stupid) windows phone

-----Original Message-----
From: "Aaron Ballman" <aaron.ballman at gmail.com>
Sent: ‎3/‎1/‎2016 3:33 PM
To: "<Alexander G. Riccio>" <alexander at riccio.com>
Cc: "reviews+D17688+public+c9d6a65abf2d2bf2 at reviews.llvm.org" <reviews+D17688+public+c9d6a65abf2d2bf2 at reviews.llvm.org>; "Devin Coughlin" <dcoughlin at apple.com>; "Anna Zaks" <zaks.anna at gmail.com>; "cfe-commits" <cfe-commits at lists.llvm.org>
Subject: Re: [PATCH] D17688: Fix missed leak from MSVC specific allocationfunctions

On Tue, Mar 1, 2016 at 1:42 PM, <Alexander G. Riccio>
<alexander at riccio.com> wrote:
> I'd quite happily add them... but can I do it in another patch? I think I
> could be more thorough that way.

I'm not certain I understand the reasoning, but I also don't have
strong feelings on whether it's this patch or another. 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).

> For the same reason, can we list all the microsoft memory allocating
> routines here? There are a thousand routines we might want to add, and then
> a few others (like _dupenv_s, _malloca, and _expand) which are especially
> important to be able to analyze (because they have really tricky APIs), but
> because of their kooky APIs, they're harder to implement checkers for.

By "here", do you mean in this review thread, or as part of this
section of code?

~Aaron

>
> Sincerely,
> Alexander Riccio
> --
> "Change the world or go home."
> about.me/ariccio
>
> If left to my own devices, I will build more.
> ⁂
>
> On Tue, Mar 1, 2016 at 8:38 AM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> On Tue, Mar 1, 2016 at 2:16 AM, Alexander Riccio <alexander at riccio.com>
>> wrote:
>> > ariccio updated this revision to Diff 49456.
>> > ariccio added a comment.
>> >
>> > Nit addressed.
>> >
>> >
>> > http://reviews.llvm.org/D17688
>> >
>> > Files:
>> >   llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> >
>> > Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> > ===================================================================
>> > --- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> > +++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> > @@ -169,11 +169,12 @@
>> >  {
>> >  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.
>> > @@ -231,10 +232,11 @@
>> >    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;
>> > @@ -540,9 +542,15 @@
>> >    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");
>>
>> What about: _mbsdup, _strdup_dbg, _wcsdup_dbg, _aligned_realloc, and
>> the rest? If we're going to add these (which I really support), it
>> would be good to make a comprehensive sweep for the Win32 additions
>> and add them all.
>>
>> ~Aaron
>>
>> >  }
>> >
>> >  bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext
>> > &C) const {
>> > @@ -585,7 +593,8 @@
>> >      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;
>> >      }
>> >
>> > @@ -600,7 +609,7 @@
>> >      }
>> >
>> >      if (Family == AF_Alloca && CheckAlloc) {
>> > -      if (FunI == II_alloca)
>> > +      if (FunI == II_alloca || FunI == II_win_alloca)
>> >          return true;
>> >      }
>> >    }
>> > @@ -789,11 +798,12 @@
>> >        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);
>> >
>> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160301/1de8954d/attachment-0001.html>