Will this leak memory?

I have this function:

char* pascalsToMillibars(uint32_t pascals)
{
    #define MBAR_STRLEN 15
    
    char *str = (char*) malloc(sizeof(char) * MBAR_STRLEN);

    uint8_t offset;

    uint16_t millibars  = pascals / 100;
    uint16_t  decimals  = pascals % 100;

    offset = sprintf (str, "%4u%c%02u%s",
        millibars,
        '.',
        decimals,
        " mbar");

    *(str + offset) = '\0'; // redundant?

    return str;
}

and I call it every second like this:

  char *absolute_mbar = pascalsToMillibars (barometer.readPressure());
  Serial.print(F("Absolute pressure: "));
  Serial.print(absolute_mbar);
  Serial.write(10);
  free(absolute_mbar);

It's working as intended, but using malloc() always makes me nervous about memory leaks over time. Is everything I did alright, or may I run into problems down the road?

One more thing: is the line marked "redundant" really redundant, or should I keep it? In other words, is the null character in a double-quoted string preserved when I use sprintf() like I just did?

It should. The preferred method is 'new' and 'delete'. Do you really have to use dynamic memory for this? Why not a static text buffer?

it's not the recommended choice, one tend to avoid using explicitly new and delete.


it's also a bad practice to allocate the memory and then hand over the pointer to something else. The best practice is to let the caller provide the storage.

regarding the null terminator, sprintf() will add that for you. I would suggest snprintf() just to be on the safe side and never overflow

Even if you don't have the STL to help? I also noticed that the caller has to do the free(). That is really dangerous.

if @PieterP comes around, he will tell you

Usage of new should be limited to low-level code only, cf C++ Core Guidelines: Avoid calling new and delete explicitly

1 Like

Okay, that's fine. Just wondering how it will solve the OP problem right now.

Specifically, I.11: Never transfer ownership by a raw pointer (T*) or reference (T&). I.e. never return malloc'ed memory using a raw pointer.

I would return a String if dynamic allocation is absolutely necessary (or if it's on a non-embedded system and I'm lazy).
In this case, and in general if you don't need dynamic allocation, and if you are not in a multithreaded environment and the function doesn't have to be reentrant (e.g. never called in an ISR), I'd probably just use a static local char array and return a pointer to it.

1 Like

I'm aware of the danger of forgetting to call free(), or calling it more than once on the same pointer, or did you have some other kind of danger in mind?

No, dynamic allocation is not necessary in my case. I could as well use a static local array, like you say, or even a global array for that matter. I was just wondering if my code, the way it is now, can turn around and bite me or not.

As you said, either make the buffer static within the function and return a const char*

inline  const char* pascalsToMillibars(uint32_t pascals) {
  const size_t MBAR_STRLEN = 15;
  static char str[MBAR_STRLEN];
  snprintf (str, MBAR_STRLEN, "%4lu.%02lu mbar", pascals / 100ul, pascals % 100ul);
  return str;
}

void setup() {
  Serial.begin(115200); Serial.println();
  Serial.println(pascalsToMillibars(101325ul));
}

void loop() {}

or as suggested, let the caller provide the buffer and be responsible for the memory

inline char* pascalsToMillibars(char* str, const size_t len, uint32_t pascals) {
  snprintf (str, len, "%4lu.%02lu mbar", pascals / 100ul, pascals % 100ul);
  return str;
}

void setup() {
  const size_t MBAR_STRLEN = 15;
  char str[MBAR_STRLEN];
  Serial.begin(115200); Serial.println();
  Serial.println(pascalsToMillibars(str, MBAR_STRLEN, 101325ul));
}

void loop() {}

Even if you never forget to call free() in every possible branch of your calling code, it's impolite towards your users to impose the burden of cleaning up your memory onto the caller.

If you use a container like String or an owning pointer like std::unique_ptr, you simply don't have to worry about that, the compiler will always clean it up for you, in all possible scenarios, early returns, exceptions, etc. This lowers the cognitive load and allows you to focus on the actual logic and structure of the program rather than memory management.

All of that is of course ignoring the problematic aspects of using dynamic allocation on a microcontroller.

My feeling is, if the end result is not different, and I have both a safe and risky method of accomplishing it, it's obvious that I should choose the safe way.

Unless it's a school assignment to see how well I can mitigate necessary risks...

Also consider how convoluted your calling is, compared with:

  Serial.print(pascalsToMillibars (barometer.readPressure()) );

that would result from using a static string.

I assume OP does not just want to print and needs the string in memory . Otherwise you just do

Serial.print(barometer.readPressure()/100.0, 2); // 1 mbar = 1 hPa
Serial.println(F(" mbar"));

Yes, this is certainly enough for debugging purposes. Actually, it all started with something along these lines. However, I began to feel the need for a more general-purpose function, something I can use to feed a display, to log to EEPROM and whatnot. Also, the sprintf() approach lets me do some padding, so different values with different numbers of digits will line up on the decimal point end be easier to read.

There were some interesting points in all the answers I got, but one thing is still unclear to me. Can I get away with using my code the way it is, or should I rather forget about malloc() and its dangers and do it some safer way, no matter what?

You can get away with malloc and free with the risks of poking holes in the heap depending what the rest of the code does (how other dynamic memory allocation work).

but why would you keep a poor design...

if you want to play with dynamic memory, then build in a local buffer and return a String instance built from that buffer

Forget about malloc for this purpose, and especially forget about returning its result as a raw pointer. Most of the C++ Core Guidelines are nuanced and suggestions, however, I.11: Never transfer ownership by a raw pointer (T*) or reference (T&) is firm: never use a raw pointer to transfer ownership. There are better alternatives (containers, smart pointers, or in the worst case things like gsl::owner), use them. This has two advantages: 1. your code is safer and won't leak, and 2. if you stick to this convention it's clear to users of your code (and future maintainers) that they should never call free() on any pointer returned from a function, again making the code easier to reason about.
If some functions in your code base return pointers that should be free()'d and other functions return pointers that shouldn't be free()'d, you rely on the users to read the documentation and do the right thing. This makes your code harder to use, and they will inevitably get it wrong.

...myself included in 6 months, I presume. OK, I'll go and code in something safer and easier to maintain.

You seem to have known size at compile time, there is no reason to have dynamic allocation

That's correct, I have a buffer of fixed size and no real need for dynamic allocation. However I noticed that the malloc() version saves some space in memory, presumably because I free() the memory as soon as I am done with the pointer (unless I'm wrong about how memory works in my particular case).

When I take the safer approach and use static arrays, the memory usage goes up depending on how many separate instances I invoke within my caller function, because with each call one global, however hidden, variable is created and maintained in memory.

Am I reasoning right about my original approach saving some memory?

You judge the size of your saving by compile message when uploading?