4

I'm trying to implement a secure-free function that erases the allocated memory, frees it and then also sets the pointer to the allocated region to NULL so the pointer cannot be reused after-free and cannot be double-freed with the same function. To achieve this I'm using a pointer-to-pointer parameter, which allows me to overwrite the pointer to the allocated memory.

The issue is GCC complaining about incompatible pointer types ("but it works on my machine"); I did not expect such a warning. My understanding was that any pointer can be implicitly cast to void*, thus I'm guessing also the address of a pointer could be cast to void**.

In the meantime I rewrote secure_free() as a macro, which solves the warning, but I would like to know why the compiler is complaining.

File securefree.c

#include <stdlib.h>
#include <stdint.h>
#include <string.h>

#define STRING_BUFFER_LEN 10

/**
 * Securely erases a heap-allocated memory section, frees it and sets its
 * pointer to NULL to avoid use-after-free and double-free.
 */
static void secure_free(void** p_p_data, size_t length_in_bytes)
{
    if (p_p_data == NULL || *p_p_data == NULL)
    { return; }
    memset(*p_p_data, 0, length_in_bytes);
    free(*p_p_data);
    *p_p_data = NULL;
}

int main(void)
{
    // Allocate some data
    char* my_string = calloc(STRING_BUFFER_LEN, sizeof(char));
    if (my_string == NULL) { return 1; }
    // Use the allocated space in some way
    my_string[0] = 'a';
    my_string[1] = 'b';
    // Free using the dedicated function
    secure_free(&my_string, STRING_BUFFER_LEN);
    return 0;
}

Compiling with GCC (Rev6, Built by MSYS2 project, 10.2.0):

$ gcc securefree.c -o securefree
securefree.c: In function 'main':
securefree.c:29:17: warning: passing argument 1 of 'secure_free' from incompatible pointer type [-Wincompatible-pointer-types]
   29 |     secure_free(&my_string, STRING_BUFFER_LEN);
      |                 ^~~~~~~~~~
      |                 |
      |                 char **
securefree.c:11:32: note: expected 'void **' but argument is of type 'char **'
   11 | static void secure_free(void** p_p_data, size_t length_in_bytes)
      |                         ~~~~~~~^~~~~~~~

EDIT: the macro version looks like this

#define secure_free_macro(ptr, len) if ((ptr) != NULL) { \
        memset((ptr), 0, (len)); free(ptr); (ptr) = NULL; }
Matjaž
  • 292
  • 6
  • 14

2 Answers2

3

What you're trying to do cannot be done portably, because different pointer types can have different representations; and to assign the null pointer to the value, you must cast the pointer-to-pointer first to a pointer to the effective type of the actual pointer variable - which is impossible.

What you can do however is use a macro, it is as good as any, and much simpler to use:

#define secure_free(x) (free(x), (x) = 0)

This works without &.

  • AFAIK one can always set a `void*` variable to NULL: it's literally setting a pointer data type (thus an address) to zero. This is portable because it compiles to different pointer sizes on different machines, that's what `void*` is for - or am I completely misunderstanding your answer? – Matjaž Apr 02 '21 at 11:23
  • @Matjaž: You can set any pointer object to a null pointer. But that is not what happens if you have a `void *` parameter `p` that has been passed from, say, an `int **` argument and attempt to set it to null using `* (void **) p = 0;`. The address passed by parameter `p` is the address of a `int *`, not of a `void *`, and those are different types and may have different representations and sizes in memory. `* (void **) p = 0;` will attempt to set the bytes of a `void *` in memory to a null pointer, but that might not work for the bytes of an `int *`. – Eric Postpischil Apr 02 '21 at 11:32
  • That's news to me. Could you give me an example of where pointers on the same machine have different sizes or memory representations? – Matjaž Apr 02 '21 at 11:34
  • 1
    @Matjaž, see https://port70.net/~nsz/c/c11/n1570.html#6.2.5p28. It looks that only `void*` and pointer to character are interchangeable. Moreover, `*(void**)p=0` may violate strict aliasing rules but it can be workaround with `memcpy`. – tstanisl Apr 02 '21 at 11:41
  • 1
    @Matjaž: See the question [Are there any platforms where pointers to different types have different sizes?](https://stackoverflow.com/questions/916051/are-there-any-platforms-where-pointers-to-different-types-have-different-sizes). – Eric Postpischil Apr 02 '21 at 11:42
  • 1
    @Matjaž: Although such machines are largely archaic, there is a consequence in modern compilers: Different pointer types are generally incompatible as defined by the C standard, and accessing one pointer type as another violates the aliasing rule in C 2018 6.5 7, even if the sizes and representations are the same. That makes the behavior of code that does this not defined by the C standard, and that means compiler optimization may transform it in ways that break your program. Code that uses the wrong type in this way should not be used absent an additional guarantee from the compiler. – Eric Postpischil Apr 02 '21 at 11:43
2

C lets any pointer be implicitly cast to void* as an explicit exception. Note, that void and char are not compatible types. Thus void*, char*, void** and char** are not compatible as well. That is why compiler emits a warning.

To bypass this issue change the function signature to use void*:

void secure_free(void* ptr, size_t length_in_bytes) {
   void **p_p_data = (void**)ptr;
   ...
}

To add protection that the argument is a pointer to a pointer one could use a macro:

#define secure_free(x,s) ((void)sizeof **(x), secure_free((x), (s)))
  • Expression **(x) will not compile is x were not a pointer to a pointer.
  • sizeof prevent evaluation of x in **(x) to avoid side effects
  • (void) silence the compiler about complaining on unused value
  • comma operator (X,Y), return only value of the Y, which is return value of secure_free(...)
  • using the same name for macro as for function allows to expand secure_free as macro only if it used as a function. This allows to use secure_free as a pointer to a function

Extra note. In the code

    memset(*p_p_data, 0, length_in_bytes);
    free(*p_p_data);

The compiler will likely optimize out memset(). I suggest casting to volatile void * to force the compiler to generate clearing code.

Edit

Additionally, one may have clear the content with a loop because memset discards volatile qualifier.

tstanisl
  • 13,520
  • 2
  • 25
  • 40
  • This would work, but I lose type safety: the function caller can now pass the pointer instead of the address of the pointer without any compiler warnings: `secure_free(my_string)` and `secure_free(&my_string)` both compile without warnings when only the second is correct. – Matjaž Apr 02 '21 at 09:58
  • Thanks! You explained the reason of the compiler warning and proposed a solution. However if I have to write a function and then wrap it in a macro, I would just use the macro-version `secure_free_macro()` for simplicity. About the volatile cast: it's a good suggestion, but ` warning: passing argument 1 of 'memset' discards 'volatile' qualifier` PS: downvote is not mine. – Matjaž Apr 02 '21 at 11:37
  • 2
    When `secure_free` is passed the address of a pointer type other than a pointer to `void` or a character type, and this pointer is converted to `void **` by `void **p_p_data = (void **) ptr;`, then `*p_p_data = NULL;` has undefined behavior because it violates C 2018 6.5 7, which says that an object shall be accessed (which includes writing) only with a type compatible with its effective type or certain other types. The type of `*p_p_data` is `void *`, but the object it points to is an incompatible type, such as `int *`, so the rule is violated. So this code should not be used. – Eric Postpischil Apr 02 '21 at 11:38
  • Bonus points for referring to the C standard – Matjaž Apr 02 '21 at 11:40
  • @EricPostpischil, it guess the issue of strict aliasing can be bypassed by `memcpy(ptr, &(void*){0}, sizeof(void*))`. But still C standard does not guarantee it the original pointer would be correctly set to NULL. – tstanisl Apr 02 '21 at 11:45
  • there is no guarantee that any object pointer would not be smaller than `void *`, nor is there any guarantee that the void's null pattern is the one that is suitable for the target type – Antti Haapala -- Слава Україні Apr 06 '21 at 06:46
  • @AnttiHaapala, yes but now it's in *Implementation Defined* regime rather than *UB*. Moreover, one can add a check for `sizeof(x) == sizeof(void*)` in the `secure_free` macro above. This should cover all practical applications except legacy platforms. However, there is still a problem if `NULLs` of different types have the same representation. – tstanisl Apr 06 '21 at 11:27
  • @tstanisl well, that is true. Implementation-defined behaviour is much better than UB :D – Antti Haapala -- Слава Україні Apr 06 '21 at 12:11