[cfe-commits] r138937 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/malloc.c

[Xu Zhongxing]

Okay. I am fine with returning a non-null pointer when the size is 0. 
 
------------------ Original ------------------
From:  "Jordy Rose"<jediknil at belkadan.com>;
Date:  Fri, Sep 2, 2011 10:17 AM
To:  "Xu Zhongxing"<xuzhongxing at foxmail.com>; 
Cc:  "cfe-commits"<cfe-commits at cs.uiuc.edu>; 
Subject:  Re: [cfe-commits] r138937 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/malloc.c

 
I think that's just defensive programming in case the allocation fails. It's not possible for n to be 0 in this function.


On Sep 1, 2011, at 18:37, Xu Zhongxing wrote:

> I find some real code assumes the NULL return value. For example, 
> 
> SXML_CHAR* strcat_alloc(SXML_CHAR** src1, const SXML_CHAR* src2)
> {
> 	SXML_CHAR* cat;
> 	int n;
> 
> 	if (src1 == NULL || *src1 == src2) return NULL; /* Do not concatenate '*src1' with itself */
> 
> 	/* Concatenate a NULL or empty string */
> 	if (src2 == NULL || *src2 == NULC) return *src1;
> 
> 	n = (*src1 == NULL ? 0 : sx_strlen(*src1)) + sx_strlen(src2) + 1;
> 	cat = (SXML_CHAR*)__realloc(*src1, n*sizeof(SXML_CHAR));
> 	if (cat == NULL) return NULL;
> 	if (*src1 == NULL) *cat = NULC;
> 	*src1 = cat;
> 	sx_strcat(*src1, src2);
> 
> 	return *src1;
> }
>  
> It compares 'cat' with 'NULL'.
> 
> ------------------ Original ------------------
> From:  "Jordy Rose"<jediknil at belkadan.com>;
> Date:  Fri, Sep 2, 2011 09:31 AM
> To:  "Xu Zhongxing"<xuzhongxing at foxmail.com>;
> Cc:  "cfe-commits"<cfe-commits at cs.uiuc.edu>;
> Subject:  Re: [cfe-commits] r138937 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/malloc.c
>  
> If we want to check the current OS, that's one thing, but I don't think we can make this assumption everywhere. Even the man page you linked leaves open the possibility of being non-null on Linux systems.
> 
> Jordy
> 
> 
> On Sep 1, 2011, at 18:25, Xu Zhongxing wrote:
> 
> > http://www.kernel.org/doc/man-pages/online/pages/man3/free.3.html
> > 
> > It seems Linux has different definition from Mac OS X.
> >  
> >  
> > ------------------ Original ------------------
> > From:  "Jordy Rose"<jediknil at belkadan.com>;
> > Date:  Thu, Sep 1, 2011 01:58 PM
> > To:  "Zhongxing Xu"<xuzhongxing at foxmail.com>;
> > Cc:  "cfe-commits"<cfe-commits at cs.uiuc.edu>;
> > Subject:  Re: [cfe-commits] r138937 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/malloc.c
> >  
> > I'm not sure about this...it's unspecified in the C standard and not true on Mac OS X (at least not in Lion):
> > 
> > "If size is zero and ptr is not NULL, a new, minimum sized object is allocated and the original object is freed."
> > 
> > But we definitely don't need both transitions, and UndefinedVal is probably wrong. It's only /dereferencing/ the pointer that's bad.
> > 
> > Jordy
> > 
> > 
> > On Aug 31, 2011, at 21:53, Zhongxing Xu wrote:
> > 
> > > Author: zhongxingxu
> > > Date: Wed Aug 31 23:53:59 2011
> > > New Revision: 138937
> > > 
> > > URL: http://llvm.org/viewvc/llvm-project?rev=138937&view=rev
> > > Log:
> > > If size was equal to 0, either NULL or a pointer suitable to be passed to 
> > > free() is returned by realloc(). Most code expect NULL.
> > > 
> > > And we only need to transfer one final ProgramState.
> > > 
> > > Modified:
> > >    cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> > >    cfe/trunk/test/Analysis/malloc.c
> > > 
> > > Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=138937&r1=138936&r2=138937&view=diff
> > > ==============================================================================
> > > --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
> > > +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Wed Aug 31 23:53:59 2011
> > > @@ -538,11 +538,8 @@
> > >       if (const ProgramState *stateFree = 
> > >           FreeMemAux(C, CE, stateSizeZero, 0, false)) {
> > > 
> > > -        // Add the state transition to set input pointer argument to be free.
> > > -        C.addTransition(stateFree);
> > > -
> > > -        // Bind the return value to UndefinedVal because it is now free.
> > > -        C.addTransition(stateFree->BindExpr(CE, UndefinedVal(), true));
> > > +        // Bind the return value to NULL because it is now free.
> > > +        C.addTransition(stateFree->BindExpr(CE, svalBuilder.makeNull(), true));
> > >       }
> > >     if (const ProgramState *stateSizeNotZero = stateNotEqual->assume(SizeZero,false))
> > >       if (const ProgramState *stateFree = FreeMemAux(C, CE, stateSizeNotZero,
> > > 
> > > Modified: cfe/trunk/test/Analysis/malloc.c
> > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.c?rev=138937&r1=138936&r2=138937&view=diff
> > > ==============================================================================
> > > --- cfe/trunk/test/Analysis/malloc.c (original)
> > > +++ cfe/trunk/test/Analysis/malloc.c Wed Aug 31 23:53:59 2011
> > > @@ -41,7 +41,7 @@
> > > 
> > > void f2_realloc_1() {
> > >   int *p = malloc(12);
> > > -  int *q = realloc(p,0); // expected-warning{{Assigned value is garbage or undefined}}
> > > +  int *q = realloc(p,0); // no-warning
> > > }
> > > 
> > > // ownership attributes tests
> > > 
> > > 
> > > _______________________________________________
> > > cfe-commits mailing list
> > > cfe-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > 
> > 
> 
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110902/c66f3845/attachment.html>