0

I'm getting this warning while using a piece of code written like below:

//Macro
#define FREEIF(p)            if (p) { free_mem((void*)p); (p) = 0; }

//free_mem function
int free_mem(void *mem_ptr)
{
    if (mem_ptr != NULL)
    {
        free(mem_ptr);
    }
    mem_ptr = NULL;
    return 0;
}


//Use of Macro in my .c file with above declaration and definition of macro.
....
....

{
 FREEIF(temp_ptr);
}

If I add a check for "temp_ptr" e.g if (temp_ptr) {FREEIF(temp_ptr);} before calling MACRO, I don't get this warning.

As I'm already checking the "temp_ptr" inside MACRO. I am wondering why I get this warning.

Any insight?

user3860869
  • 121
  • 1
  • 1
  • 6
  • 2
    FYI You don't need to check if what to pass to `free()` is `NULL` because `free(NULL);` is defined to do nothing. – MikeCAT Aug 06 '21 at 13:25
  • 1
    And there's no value in assigning `NULL` to the pointer inside `free_mem` before leaving the function, that variable runs out of scope anyway. Matter changes if you pass a double pointer (`void** ptr`) and assign `NULL` to `*ptr`. – Aconcagua Aug 06 '21 at 13:28
  • 1
    You might just try `#define FREE(p) do { free(p); (p) = NULL; (void)(p); } while(0)` and skip `free_mem` entirely... – Aconcagua Aug 06 '21 at 13:33
  • You don't make clear in your question which line the message refers to. Your code contains two places that will/can trigger this warning. The expansion of `FREEIF(temp_ptr);` contains `temp_ptr = 0;`. Maybe your code does not use the value of `temp_ptr` after the assignment. Function `free_mem` contains an assignment to the local copy of the pointer in the function argument `mem_ptr` which is not used afterwards and also not passed to the calling function, so this assignment is useless. – Bodo Aug 06 '21 at 13:35
  • Like I mentioned above, the warning appears on line of .c file. When I'm calling FREEIF(temp_ptr); – user3860869 Aug 06 '21 at 13:37
  • In `FREEIF(p)`, that `p` should be wrapped in parentheses in this part: `free_mem((void*)p)` like so: `free_mem((void*)(p))`. There is no need for the type cast to `void*` so it should be changed to `free_mem((p))` but then there is no need for the extra pair of parentheses because the existing pair of parentheses for the function call suffice, so it should be changed to `free_mem(p)`. – Ian Abbott Aug 06 '21 at 13:38

2 Answers2

2

Arguments in C are passed by value. This means the values passed as arguments are copied to the arguments visible inside the function and modifying the arguments from the function don't have anye effect to the original values passed.

Therefore, the line mem_ptr = NULL; is meaningless (at least it seems meaningless assuming that no undefined behavior like out-of-bounds read or dereferencing invalidated pointers) because mem_ptr is local to the function and the value is not read at all after the assignment.

On the other hand, (p) = 0; may not be meaningless because p is not declared in the macro, so it will refer what is declared before the macro and that may be read after the invocation of the macro.

MikeCAT
  • 73,922
  • 11
  • 45
  • 70
2

Regarding the error, rule 2.2 is about not having any "dead code" in your program.

In the function, mem_ptr = NULL; sets the local variable mem_ptr to null, not the one passed. So that code line does nothing. This is the reason for the error and it's a common beginner FAQ, see Dynamic memory access only works inside function for details.

In the function-like macro, the passed pointer would however get changed by (p) = 0;. But if you aren't using the pointer after setting it to null, it's still regarded as "dead code" since the assignment is then strictly speaking pointless (although good practice). We can't tell since you didn't post the calling code nor the actual pointer declaration.

But there's some far more serious big picture issues here:

  • Using MISRA-C and dynamic memory allocation at the same time is nonsensical. They are pretty much mutually exclusive. Embedded systems in general don't use dynamic allocation, especially not bare metal/RTOS MCU applications where it simply doesn't make any sense.

    Dynamic allocation is particularly banned in mission-critical/safety-related software. This is not only banned by MISRA, but by any coding standard out there. It is also banned by generic safety standards like IEC 61508, ISO 26262, DO 178 etc.

  • Safety and MISRA aside, your macro is still nonsense since free() on a null pointer is a well-defined no-op. See the definition of free in C17 7.22.3.3:

    void free(void *ptr); /--/ If ptr is a null pointer, no action occurs.

    So all the macro achieves is to obfuscate the code and slow it down with an extra, pointless branch.

The correct solution here is to nuke this macro, then take a step back and consider what you are even doing with this project. Start with the requirements. Why do you need MISRA-C, does somebody in this project know what they are doing, if not - who should we hire to help with this project. And so on. You need at least one C veteran on the team for a project with MISRA-C or otherwise the project is doomed.

Lundin
  • 195,001
  • 40
  • 254
  • 396