6
\$\begingroup\$

As we all know, the syntax of allocating memory is a bit clunky in C. The recommended way is:

 int *p;
 int n=10;
 p = malloc(n*sizeof *p);

You can use sizeof (int) instead of sizeof *p but it is bad practice.

I made a solution to this with a macro:

#define ALLOC(p,n) do{ *p=malloc(n*sizeof **p); } while(0)

This get called this way:

 int *p;
 int n=10;
 ALLOC(&p, n);

The reason I want to pass the address of p rather than p, is to offer a bit of protection. When you call a function with foo(x), then foo can never change the value of x. If you're not passing the address, the argument will not change. (Sure there are ways around that too.)

Any comments?

\$\endgroup\$
  • \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 3 → 2. \$\endgroup\$ – 200_success Jan 26 at 18:10
  • 4
    \$\begingroup\$ I think the basic problem here is that your opinion of the "clunkiness" of C memory allocation is not shared by many experienced C programmers. (To me, it seems elegantly simple.) So by hiding what you're actually doing in a macro, you just introduce possible confusion. \$\endgroup\$ – jamesqf Jan 27 at 5:18
17
\$\begingroup\$

p = malloc(n*sizeof *p);

This is dangerous if n gets large, because the multiplication could overflow. After the overflow, too little memory has been allocated but code will continue without detecting the error.

This is especially dangerous if n comes from untrusted source, such as some file format or remote user. Then it gives attacker easy way to overwrite parts of memory with exploit code.

The easy safe solution is to use calloc() which will detect the overflow (at least on most common libc implementations). If you really need to use malloc() for some reason, you can check for overflow separately.

\$\endgroup\$
  • \$\begingroup\$ Indeed, this is an often overlooked but very important point. I think that any calloc implementation that does not return a null pointer on overflow would not be conforming. \$\endgroup\$ – jamesdlin Jan 26 at 23:38
  • 1
    \$\begingroup\$ @jamesdlin Yes. Still, one should not ignore that calloc() has to zero the memory too. \$\endgroup\$ – Deduplicator Jan 26 at 23:43
8
\$\begingroup\$

You've taken an expression statement and unnecessarily made a do/while statement out of it. You need parentheses around your macro parameters. You don't need to pass in the pointer you're assigning to as a pointer.

Put all that together and you end up with:

#define ALLOC(p, n)  ((p) = malloc((n) * sizeof *(p)))

This puts fewer restrictions on how it is used, and allows things like

struct monster *AddMonsters(monster *monsters, int which_monster, int how_many)
{
    int difficulty_add = ExtraMonsters();
    return ALLOC(monsters + which_monster, how_many + difficulty_add);
}

Yes, you could use monsters[which_monster], add the extra before using the macro, and put a separate return statement, but all that would be unnecessary restrictions on its use.

\$\endgroup\$
  • 1
    \$\begingroup\$ The do-while is just something I always do with macros. A bit redundant here, but it does not hurt. Parenthesis I forgot. Thanks for that. But I do have a good reason for passing it as a pointer. See my edit. \$\endgroup\$ – klutt Jan 26 at 17:18
  • 6
    \$\begingroup\$ @Broman "Does not hurt"? Sure it does. You're unnecessarily making it illegal to use your macro where an expression is expected, such as with if (ALLOC(...)). \$\endgroup\$ – jamesdlin Jan 26 at 23:24
  • 1
    \$\begingroup\$ I would prefer the do-while, personally. The body technically works as an expression, but it’s still an assignment. Those can be surprising in expression position, especially behind a macro. \$\endgroup\$ – gntskn Jan 27 at 1:44
4
\$\begingroup\$

There are a couple questionable things here, first since ALLOC() is a macro you don't need to pass the address of p. If ALLOC() were a function call then you would want to pass the address of p.

Second, for an array I would probably use calloc() rather than malloc().

\$\endgroup\$
  • 2
    \$\begingroup\$ The reason for passing the address is to make it clear that the argument will be modified. \$\endgroup\$ – klutt Jan 26 at 17:54
2
\$\begingroup\$

First of all, remove the unnecessary parts, which can cause errors and make the code harder to read.

#define ALLOC(p,n) p=malloc(n*sizeof *p)

You can make your code more readable with describing names. After months it will be quite hard to understand even your own code.

#define ALLOC(pointer, size) pointer = malloc(size * sizeof *pointer)

Parenthesizes are important! The following call: ALLOC(pointer, size + 1); would be equal with pointer = malloc(size + 1 * sizeof *pointer);, which clearly is a bug.

#define ALLOC(pointer, size) (pointer) = malloc((size) * sizeof(*pointer))

Use calloc instead of malloc, because of security reasons.

#define ALLOC(pointer, size) (pointer) = calloc((size), sizeof(*pointer))

Lastly, do not use function-like macros, instead a function would be a better choice. https://rules.sonarsource.com/c/tag/preprocessor/RSPEC-960

\$\endgroup\$
  • \$\begingroup\$ "because of security reasons" is very vague, and arguably untrue. It's certainly not enough to justify an obvious pessimisation. \$\endgroup\$ – Toby Speight Feb 4 at 12:48
  • \$\begingroup\$ @jpa explained it quite well, so I did not went into details, but I focused on mentioning different, possible error sources. \$\endgroup\$ – Norbert Incze Feb 4 at 14:38
  • \$\begingroup\$ Describing names are good, but for such a simple function as this I'd say it's abundantly clear. And I would like to use a function instead, but for my purpose, it is not possible. Besides, the link you gave does not illustrate an issue that could arise in this code. \$\endgroup\$ – klutt May 13 at 21:38
  • \$\begingroup\$ Your opinion is totally understandable, the function is simple, the code is not safety critical or anything like that. But I still recommend to use describing names and try to improve, refactor each part of your code whenever you can. You will improve by practicing it on these little functions. After months it helps a lot, even at these functions, when you look at their names and you totally understand what it is doing. I do not even mention when others have to look at your code. Remember, we read code way more times than it was wrote/refactored. \$\endgroup\$ – Norbert Incze May 13 at 21:54
2
\$\begingroup\$

Based on your question, a similar question I asked yesterday, and some of the answers, I did a version that tries to have the benefits of all of them.

Disclaimer: Code is GCC specific (although probably works in similar compilers such as Clang).

Usage:

int *p;

/* Want >int p[47];< */
if (mallocs(&p, 47))
        goto err;
...
free(p);

Properties:

  • Avoid having to type sizeof
  • Can be used inside an if
  • Returns an int error code
  • Check for invalid input pointer: mallocs(NULL, 47) == EINVAL
  • Check for negative nmemb: mallocs(&p, -47) == -EOVERFLOW (and p set to NULL)
  • Check for overflow due to high nmemb: mallocs(&p, TOO_HIGH) == EOVERFLOW (and p set to NULL)
  • Check for malloc error: mallocs(&p, 47) == ENOMEM (and p set to NULL) (correct input, but malloc fails for some reson)
  • Sets errno on any failure (malloc shall set errno on failure, so if this is going to be in a library, it would make sense to set errno on failure too)
  • I don't need a pointer to a pointer given that this is a macro, but I think a user that doesn't know this would probably be happier passing a pointer to a pointer, and keep thinking it is a function :) I would have to read the code if some tells me that (what looks like) a function call modifies a pointer without a pointer to it.
  • Prevents double evaluation of arguments
  • Removes the possibility that someone may cast the result of malloc

Code: (Edited: This code has problems; at the end of the answer is the fixed one)

#include <errno.h>
#include <stddef.h>
#include <stdint.h>
#include <stdlib.h>


/*
 * int  mallocs(type **restrict p, ptrdiff_t nmemb);
 */
#define mallocs(ptr, nmemb)     (                                       \
{                                                                       \
        ptrdiff_t   nmemb_  = (nmemb);                                  \
        __auto_type ptr_    = (ptr);                                    \
        int         err_;                                               \
                                                                        \
        err_    = 0;                                                    \
        if (ptr_ == NULL) {                                             \
                errno   = EINVAL;                                       \
                err_    = EINVAL;                                       \
                goto ret_;                                              \
        }                                                               \
        if (nmemb_ < 0) {                                               \
                *ptr_   = NULL;                                         \
                errno   = EOVERFLOW;                                    \
                err_    = -EOVERFLOW;                                   \
                goto ret_;                                              \
        }                                                               \
        if (nmemb_ > (PTRDIFF_MAX / (ptrdiff_t)sizeof(**ptr_)) {        \
                *ptr_   = NULL;                                         \
                errno   = EOVERFLOW;                                    \
                err_    = EOVERFLOW;                                    \
                goto ret_;                                              \
        }                                                               \
                                                                        \
        *ptr_   = malloc(sizeof(**ptr_) * nmemb_);                      \
        if (*ptr_ == NULL)                                              \
                err_    = ENOMEM;                                       \
ret_:                                                                   \
        err_;                                                           \
}                                                                       \
)

I named it mallocs (malloc safe)


EDIT:

After finding that the code above can only be called once in a function, I improved it making use of an inline. Now there is the possibility to call the function or the macro, depending on your preferences (both can be called multiple times):

#include <errno.h>
#include <stddef.h>
#include <stdint.h>
#include <stdlib.h>


#define mallocs(ptr, nmemb) (                               \
{                                                           \
        __auto_type     ptr_    = (ptr);                    \
                                                            \
        *ptr_   = mallocarray((nmemb), sizeof(**ptr_));     \
                                                            \
        !(*ptr_);                                           \
}                                                           \
)


inline
void    *mallocarray(ptrdiff_t nmemb, size_t size);


inline
void    *mallocarray(ptrdiff_t nmemb, size_t size)
{

    if (nmemb < 0)
        goto ovf;
    if (nmemb > (PTRDIFF_MAX / (ptrdiff_t)size))
        goto ovf;

    return  malloc(size * nmemb);
ovf:
    errno   = EOVERFLOW;
    return  NULL;
}

I named mallocarray() after the BSD extension reallocarray()

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.