Skip to content

mismatched delete #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
FStefanni opened this issue Jul 19, 2021 · 6 comments · Fixed by #56
Closed

mismatched delete #53

FStefanni opened this issue Jul 19, 2021 · 6 comments · Fixed by #56

Comments

@FStefanni
Copy link

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.

@PaulZC
Copy link
Collaborator

PaulZC commented Jul 19, 2021

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
Copy link
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
Copy link
Collaborator

PaulZC commented Aug 7, 2021

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

@PaulZC PaulZC closed this as completed Aug 7, 2021
@FStefanni
Copy link
Author

FStefanni commented Aug 9, 2021

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
Copy link
Collaborator

PaulZC commented Aug 9, 2021

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

@PaulZC PaulZC reopened this Aug 9, 2021
PaulZC added a commit that referenced this issue Aug 9, 2021
@PaulZC PaulZC linked a pull request Aug 9, 2021 that will close this issue
@PaulZC PaulZC closed this as completed in #56 Aug 9, 2021
@FStefanni
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants