Re: [PATCH] Use malloca instead alloca

[Petr Baudis]

  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?

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

  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.

>   /* 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?

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

> /* 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();\
>   }\
> }

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.

You really should wrap the freea() block in do { ... } while (0)

                                Petr "Pasky" Baudis