[PATCH] D17688: Fix missed leak from MSVC specific allocation functions
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 1 12:33:14 PST 2016
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);
>> >
>> >
>
>
More information about the cfe-commits
mailing list