Memory leaks in function - please help with the code

Projects Programming
1

Hi,
I have an issue with my sketch which receive data using MySensors library, and parse it for my needs. After a 8-24 hours (depending on data receiving frequency) of proper working my arduino freezes. could you please help me with the following code? I think I could have memory leaks in one of the functions which actually performs the received data parsing. The sketch works fine if the function call is commented out, and works bad if the call is enabled. So this is the function:

void parseReading(char * receivedReadings, int sensorId) {
  char *strtokIndx;
  strtokIndx = (char*)malloc(25);
  char tempDate[17] = {0};

  //temp
  strtokIndx = strtok(receivedReadings,"/");
  dtostrf(static_cast<float>(atoi(strtokIndx))/10., 6, 1, readings[sensorId].temp);
  
  //hum
  strtokIndx = strtok(NULL, "/");
  dtostrf(static_cast<float>(atoi(strtokIndx))/10., 5, 1, readings[sensorId].hum);
  strcat(readings[sensorId].hum,"%");
  
  //date and time
  strtokIndx = strtok(NULL, "/");
  strncpy(tempDate,strtokIndx,4);
  strcat(tempDate, "-");
  strncat(tempDate,strtokIndx+4,2);
  strcat(tempDate, "-");
  strncat(tempDate,strtokIndx+6,2);
  strcat(tempDate, " ");
  strncat(tempDate,strtokIndx+8,2);
  strcat(tempDate, ":");
  strcat(tempDate,strtokIndx+10);
  strcpy(readings[sensorId].date,tempDate);

  free(strtokIndx);
}

The call is:

parseReading(receivedMsg,sensor);

where:
receivedMsg is:
char receivedMsg[25] = {0};
declared in the same scope when the call is performed and
sensor
is an uint8_t
(yeah I know that uint8_t and int are different, noticed this when writing the post but I do not think it could cause my issue).
The received string is in the following format:
123/456/201903061130
and after the parse I expect data stored in the following struct:

struct reading {
  char header[11] = {0};
  char temp[7] = {0};
  char hum[7] = {0};
  char date[17] = {0};
};
reading readings[4];

which is a global variable. And the data looks like this (based on above example):
temperature is 12.3
humidity is 45.6
date is 2019-03-06 11:30

I can post a complete sketch, however it is quite long and do not think it has an error somewhere else. I am not very good at programming in C/C++ and think I messed up with memory allocation and freeing it at the end of the function.

2

Why use malloc at all here? Just define a large enough local buffer in the function.

I suggest that you check very carefully that 25 characters is enough, I wonder if there's a buffer overflow causing your problems.

3

benski:

...

char strtokIndx;
 strtokIndx = (char)malloc(25);
...
 strtokIndx = strtok(receivedReadings,"/");
...
 free(strtokIndx);
}

You need to free the original pointer and not the original pointer + x.
I agree with wildbill, you could just use a local buffer and save the malloc. // what am I thinking... :frowning:

strtok() returns a pointer to inside the modified receivedReadings; you don't need a buffer at all.

4

You are dynamically allocating memory (that you don't need), and storing the address of that memory in a variable. You IMMEDIATELY overwrite that address with the value returned by strtok(). You now have NO way of freeing the memory you allocated. Then, you try to free memory that you didn't allocate.

No wonder you have a memory leak.

5

Thank you all for the reply. OK so I do not need strtokIndx = (char*)malloc(25); Then I do not need free() function. Should I then modify the code as follows:

...
char *strtokIndx = strtok(receivedReadings,"/");
...

and remove malloc and free? Should I then delete the strtokIndx at the end?

I programmed c/c++ at the university (11+ years ago) and forgot the dynamic memory allocation principles so sorry for potentially dumb questions.

6

benski:
and remove malloc and free? Should I then delete the strtokIndx at the end?

No. strtokIndx is a pointer into receivedReadings, so don't try to free or delete it.

7

Looks like it works fine now. 4+ days and no crash. Thank you all for your help.