Skip to content

mismatched delete #53

@FStefanni

Description

@FStefanni

Subject of the issue

Hi,

in the code, there are mismatched deletes.
In C++ new must match a delete and new[] must match delete[].
In the code, there are only delete[], even for new.

For example, in file SparkFun_u-blox_GNSS_Arduino_Library.cpp:

  • moduleSWVersion
  • currentGeofenceParams
  • packetUBXNAVTIMELS

and basically, all struct-based data pointers have this issue.

This mismatch could lead the code to crash, since it goes into undefined behavior.

Regards.

Activity

PaulZC

PaulZC commented on Jul 19, 2021

@PaulZC
Collaborator

Hi Francesco (@FStefanni ),

Thanks for this. I suspect we haven't seen this issue before because on Arduino platforms the implementations of new and new[] are usually exactly the same. Likewise for delete and delete[]:

void * operator new(size_t size)
{
  return malloc(size);
}

void * operator new[](size_t size)
{
  return malloc(size);
}

void operator delete(void * ptr)
{
  free(ptr);
}

void operator delete[](void * ptr)
{
  free(ptr);
}

Best wishes,
Paul

FStefanni

FStefanni commented on Jul 20, 2021

@FStefanni
Author

Hi,

yes, very often also malloc and free can be mixed, since the actual implementation is the same (as in the snippet you posted).
But to be 100% compliant and portable, one must adhere to the standard.
It is quite annoying to have incompatible memory management methods, and I always hope C++ will fix this in the future... but this is another topic.

Regards.

PaulZC

PaulZC commented on Aug 7, 2021

@PaulZC
Collaborator

Hi Francesco (@FStefanni ),

I've looked into this and I believe I am using the new operator correctly, certainly as far as the compilers used by Arduino are concerned. Please see this link https://www.cplusplus.com/reference/new/operator%20new[]/ , specifically lines 13 and 18 of the example.

In many places, I do create a new array directly:

Both of these alternatives generate compilation errors:

payloadCfg = new[] uint8_t[payloadSize];

payloadCfg = new[payloadSize] uint8_t;

Likewise, where I use typedef struct to define how much memory should be allocated:

moduleSWVersion = new moduleSWVersion_t; //Allocate RAM for the main struct

Replacing this with:

moduleSWVersion = new[] moduleSWVersion_t;

also generates a compilation error.

I'm going to close this as I don't believe there is any further action I can take. Please reopen if you can provide code which compiles successfully on Arduino platforms.

Best wishes,
Paul

FStefanni

FStefanni commented on Aug 9, 2021

@FStefanni
Author

Hi,

sorry, maybe I was not clear.
The C++ standards mandates to match the various memory operators. So just to clarify with exampes...

Example 1: malloc/free

myVar1 = malloc(...);
...
free(myVar1);

Example 2: new/delete

myVar2 = new MyType();
...
delete myVar2;

Example 3: new[]/delete[]

myVar3 = new MyType[5];
...
delete [] myVar2;

Other combinations are not allowed. For example it is not possible to do something as:

myVar4 = new MyType();
delete [] myVar4;

If the memory function are mixed, the code will compile, without any warning, but it will be non standard.
More precisely, it will run into the so called "undefined behavior".

  • What does it means undefined behavior? To say in few words, let's call it a subtle bug (to have a deep understanding of what it is, please just search for it).
  • What if, by any chance, we run the code and it seems to work properly? It still is a non standard code, so we cannot consider it as correct. More precisely, it could run with a specific version of a compiler or library, but it could break to a newer one.

So the fix is just to match something as:

moduleSWVersion = new moduleSWVersion_t;

with

delete moduleSWVersion;
// not: delete[] moduleSWVersion;

Regards.

PS: it seems I cannot re-open the issue...

PaulZC

PaulZC commented on Aug 9, 2021

@PaulZC
Collaborator

Hi Francesco (@FStefanni ),

Thank you for the clarification!
As you suggest, I will try replacing delete[] with delete for those cases where I am creating storage for a struct.

Best wishes,
Paul

reopened this on Aug 9, 2021
linked a pull request that will close this issuev2.0.11 #56on Aug 9, 2021
FStefanni

FStefanni commented on Aug 9, 2021

@FStefanni
Author

Hi,

now it seems fine to me.
Thank you for the fix.

Regards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @PaulZC@FStefanni

      Issue actions

        mismatched delete · Issue #53 · sparkfun/SparkFun_u-blox_GNSS_Arduino_Library