4

In the following function designed to free some memory allocated to a pointer of type Maze with Maze being a struct I defined in another file.

I am getting the compiler error nonnull argument 'maze' compared to NULL This is just a warning but I am constrained to leave the warning turned on.

Here is the code for the function:

void free_maze(Maze *maze) {
    if (maze == NULL) {
        return;
    }

    free(maze);
    return;
}

As I understood, this is the correct way to check if a pointer to a struct is NULL. What am I doing wrong here?

Colin Harrison
  • 184
  • 2
  • 18
  • 4
    is your function called from another function which has a "nonnull" directive? BTW you can safely `free` `maze` even if it's NULL. – Jean-François Fabre Oct 23 '17 at 17:42
  • doesn't calling free to memory that has already been freed result in undefined behaviour? Either way, I am supposed to check for the constraints of this. – Colin Harrison Oct 23 '17 at 17:42
  • 5
    `free(NULL)` has no effect. and we would need a [mcve], as the code alone doesn't issue any warning. – Jean-François Fabre Oct 23 '17 at 17:43
  • Right but "Otherwise, or if free(ptr) has already been called before, undefined behavior occurs" - the concern here is that it may have already been called. source: https://linux.die.net/man/3/free – Colin Harrison Oct 23 '17 at 17:45
  • I think you are compiling it with a c++ compiler... – Eugene Sh. Oct 23 '17 at 17:46
  • Exactly what are you trying to achieve by checking if maze is null? – klutt Oct 23 '17 at 17:47
  • 1
    doubtful as this is on a school system set up to compile C99 code. I ran `gcc -v` and got v 6.1.0 – Colin Harrison Oct 23 '17 at 17:47
  • 2
    This code won't protect against a double free. Calling `free_maze(my_maze); free_maze(my_maze);` can demonstrate this since `my_maze` is not set to `NULL`. – dbush Oct 23 '17 at 17:47
  • 3
    See [Is this a good way to free memory in C?](https://stackoverflow.com/q/34651234/1275169) for a related discussion. "the concern here is that it may have already been called." - but your `free_maze` function doesn't address that at all. It only check if `maze` is NULL and `free(NULL);` is totally fine. – P.P Oct 23 '17 at 17:48
  • 1
    @EugeneSh. I'm not saying that, just that the code won't do what OP expects. Although, assigning the parameter to a temp variable and comparing that against `NULL` might work. I'm not sure how to generate this warning, though. – dbush Oct 23 '17 at 17:52
  • 1
    @ColinHarrison If you want to make sure your code is well-behaved regarding memory usage, run it though [valgrind](http://valgrind.org). – dbush Oct 23 '17 at 17:53
  • @dbush I have just read about the [`nonnull` attribute](http://www.keil.com/support/man/docs/armcc/armcc_chr1359124976631.htm) and think I understand it now. Apparently it is probably written somewhere. – Eugene Sh. Oct 23 '17 at 17:53
  • 2
    The text you added in your edit makes no sense. Checking it `maze` is `NULL` doesn't check if `free_maze(maze);` has been called before or not. – ikegami Oct 23 '17 at 17:53
  • 1
    https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/UD5627IC7GE43RAN6MPTJEP2VJTF7SUG/ – Taha Paksu Oct 23 '17 at 17:55
  • 2
    Jean-François suggested that the warning might arise from the function being called from another that is endowed with a GNU `nonnull` attribute. I don't quite see how that would work, but I could believe that another in-scope declaration of *this* function attaches a `nonnull` attribute to parameter `maze`. – John Bollinger Oct 23 '17 at 17:55
  • and the full conversation is here: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/VA2QCXJGXVLH43327TRR5UM2BP52DWIC/ – Taha Paksu Oct 23 '17 at 17:56
  • @JohnBollinger yes, probably. – Jean-François Fabre Oct 23 '17 at 17:57
  • 1
    A bit of an aside, after calling `free(maze);`, your next line should be `maze = NULL;`. This will prevent your code from accidentally accessing freed memory as well as allow you a way to check if the memory has been freed (i.e. test for NULL). Since `maze` is still in scope after the `free_maze` function executes, there is always that danger. – Nik Oct 23 '17 at 17:58
  • 2
    That's not a bad idea, @Nik, and others advocate the same, but do note that its effectiveness is limited to the scope if the variable holding the pointer that gets freed. Applying it to the OP's function would be useless, because the only other thing the function does is return, so its `maze` parameter goes out of scope immediately anyway. The caller's copy would be unaffected. – John Bollinger Oct 23 '17 at 18:50
  • Great eye @JohnB, completely missed that. To NULL the original pointer, it must be passed by reference. i.e. `void free_maze(Maze **maze);`, or after calling `free_maze` – Nik Oct 23 '17 at 18:52
  • it is ok to pass a NULL pointer to `free()`, so no need for the check – user3629249 Oct 24 '17 at 18:09

1 Answers1

11

The reason of the warning is probably that the declaration of free_maze looks similar to this:

extern void free_maze (Maze *maze)
        __attribute__((nonnull));

__attribute__((nonnull)); is a GCC-specific extension.

So the declaration says that NULL should never be passed to free_maze. The compiler will try to detect violations of this constraint and warn you. Since you are not supposed to pass NULL, it makes little sense to check for it.

Even without an attribute, you don't need to check because free(NULL) is guaranteed to be safe.

Regarding your edit: a call

free(maze);

doesn't change maze in the scope of the caller. If you accidentally call free once again with the same pointer, the second call will not be with NULL but with a dangling pointer, leading to undefined behaviour. Just don't do double-free.

n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243
  • I didn't write the maze.h so sure enough I checked and this is exactly the case. Thanks for a straightforward answer with no sass. – Colin Harrison Oct 23 '17 at 17:58
  • I understood it wasn't a re createable example, but I just admitted that was the mistake I was making. I thought I did, but I made a mistake. I came here because obviously I didn't know much about memory allocation. – Colin Harrison Oct 23 '17 at 18:02
  • 1
    I've the same issue and IMO this warnings is nonesense. The __attribute__((nonnull)); is good to avoid wrong usage. The compiler can detect hardcoded NULL values and warn you, but there is no guarantee that at runtime the argument will never by NULL. For this reason it is important to check pointer arguments before dereferencing to get robust code. So throwing a warning for the comparison against NULL just makes no sense. For this reason I see no other way than disabling this warning using -Wno-nonnull-compare. – Hans Dampf Mar 14 '18 at 08:29
  • @HansDampf *The compiler may also choose to make optimizations based on the knowledge that certain function arguments will not be null.* The compiler interprets the attribute as programmer's promise that the argument will never be null. If you are checking for null, you are not trusting your own promise. Doubleplusungood. – n. m. could be an AI Mar 14 '18 at 08:56