Re: [PATCH] Use malloca instead alloca

[Ondřej Bílka]

On Sat, Dec 29, 2012 at 03:29:36PM +0100, Petr Baudis wrote:
>   Hi!
> 
>   I applaud any effort that absolves us of the current unholy pointer
> flags we need to keep track of now.
> 
> On Sat, Dec 29, 2012 at 11:33:15AM +0100, Ondřej Bílka wrote:
> > /* Safe automatic memory allocation.
> >    Copyright (C) 2012 Free Software Foundation, Inc.
> > 
> >    This program is free software; you can redistribute it and/or modify
> >    it under the terms of the GNU General Public License as published by
> >    the Free Software Foundation; either version 2, or (at your option)
> >    any later version.
> > 
> >    This program is distributed in the hope that it will be useful,
> >    but WITHOUT ANY WARRANTY; without even the implied warranty of
> >    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >    GNU General Public License for more details.
> > 
> >    You should have received a copy of the GNU General Public License
> >    along with this program; if not, see <http://www.gnu.org/licenses/>.  */
> > 
> > #ifndef _MALLOCA_H
> > #define _MALLOCA_H
> > 
> > /* AIX requires this to be the first thing in the file.  */
> > #ifndef __GNUC__
> > # if HAVE_ALLOCA_H || defined _LIBC
> > #  include <alloca.h>
> > # else
> > #  ifdef _AIX
> > #pragma alloca
> > #  else
> > #   ifndef alloca /* predefined by HP cc +Olibcalls */
> > char *alloca ();
> > #   endif
> 
> Does this matter if we go ahead with using ({...}) just a few lines
> below?

I kept this from argp/argp-help.c and was not sure if its necessary.

> 
> > #  endif
> > # endif
> > #endif
> > 
> > #include <stddef.h>
> > #include <stdlib.h>
> > #include <stdio.h>
> > #include <stdint.h>
> > 
> > #ifdef __cplusplus
> > extern "C" {
> > #endif
> > 
> > #define _ALLOCA_MC 0x2439a2431bca4812L
> > #define _MALLOC_MC 0x1bca48122439a243L
> 
>   Does the 'L' have any effect? I'm not sure if it isn't actually doing
> any harm on 32-bit platforms... If anything, I'd use 'ULL', but maybe no
> suffix should be necessary.
OK
> 
>   I assume uint64_t is used to preserve alignment? This question doesn't
> appear as trivial to me, as there are some significant considerations
> behind MALLOC_ALIGNMENT; malloca() returns less aligned pointers than
> malloc() on 64-bit platforms, which maybe matters just for exotic cases
> like long double which probably won't be an issue in glibc, but this
> should be documented somewhere.
> 
I forgot that alloca alignment is 16byte. Remaining bytes could be
pointer to guard at end. I wait with this as it would make code
uglier than already is.

> >   /* malloca(N) is a safe variant of alloca(N).  It allocates N bytes of
> >      memory allocated on the stack or heap for large requests.
> >      It must be freed using freea() before
> >      the function returns.  Upon failure, it returns NULL.  */
> > 
> > #if 1
> 
> #if 1?
Planned add alloca support detection. 
> 
> > #define malloca(n) ({\
> >   size_t  __n__ = n;\
> >   void  * __r__ = NULL;\
> >   if (__n__ <= 4096)\
> >     {\
> >       __r__ = alloca (__n__ + sizeof (uint64_t));\
> >       if (__r__)\
> >         {\
> >           *((uint64_t *)__r__) = _ALLOCA_MC;\
> >           __r__ += sizeof (uint64_t);\
> >         }\
> >     }\
> >   if (!__r__)\
> >     {\
> >       __r__ = malloc (__n__ + sizeof (uint64_t));\
> >       if (__r__)\
> >         {\
> >           *((uint64_t *)__r__) = _MALLOC_MC;\
> >           __r__ += sizeof (uint64_t);\
> >         }\
> >     }\
> >   __r__;\
> > })
> 
> So it is still unsafe to call malloca() in a loop. This is also
> something that should be documented. Or can't we do better? I think even
> having an alternative 2-parameter call would be worth considering so
> that users that need this can use a local variable to keep track of
> stack usage, as per include/alloca.h.
I do not know how to portably do that. 

For platform specific way best solution is to read where bottom of stack is
and allocate only when at least say 32768 bytes are left.
> 
> > /* If desired we could detect more corruption by 
> >    adding constant to end of alloca'd array. */
> > 
> > #define freea(r) {\
> > void *__r__ = r;\
> > if (__r__)\
> >   {\
> >     __r__ -= sizeof (uint64_t);\
> >     if  (    *((uint64_t *)__r__) == _MALLOC_MC)\
> >       free (__r__);\
> >     else if (*((uint64_t *)__r__) != _ALLOCA_MC)\
> >       __abort_freea();\
> >   }\
> > }
> 
> You really should wrap the freea() block in do { ... } while (0)
Or possibly as I dont manipulate stack here static inline function.


> I personally find it much more readable to have " \" instead of "\" at
> line ends; maybe GNU style even requires having the \s aligned at col 78.
ok
> 
> 
>                               Petr "Pasky" Baudis

-- 

terrorist activities