Thread: Do I have memory leaks?

I wrote this program:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>


struct sensor {
  unsigned long long int           address;
  float                            current;
  int                              pressure_units;
};


static struct sensor **sensors;
static int sensors_count;


struct sensor *createSensor(unsigned long long int address, float current, int pressure_units)
{
  struct sensor *temp = malloc(sizeof(struct sensor));
  // (*temp).address
  temp->address = address;
  temp->current = current;
  temp->pressure_units = pressure_units;
  return temp;
}


void checkSerialHart(int sensorId)
{
    sensors = realloc(sensors, (sensors_count+1)*sizeof(*sensors));
    if (sensors == NULL)
        exit(EXIT_FAILURE);
    sensors[sensors_count] = createSensor(10000*sensorId,sensorId+3.0,sensorId);
    sensors_count++;
}


void pollHart(void)
{
	int i,j;
	
	sensors_count = 0;
	for(i=0,j=0;i<3;i++)
    {
        // test case where sensor n doesn't respond
        if(i != 1)
        {
            checkSerialHart(j);
            ++j;
        }
    }
}


void sendToIridium()
{
	int i;
	char wholeMsg[300];


    
    for (i = 0; i < sensors_count; i++)
    {
        sprintf(wholeMsg + strlen(wholeMsg),"address: %llu, current: %f, pressure: %c\n",
        sensors[i]->address, sensors[i]->current, sensors[i]->pressure_units);
    }
    printf("\n%s\n",wholeMsg);
}


int main(int argc, char *argv[])
{


    while(1)
    {
		pollHart();
		sendToIridium();
		sleep(3);
    }
}

Does it contain any memory leaks? I'm not sure if the malloc call in the createSensor function is a memory leak, because it returns a pointer and there is no way to free that memory before the pointer is returned.

I can't quite put my finger on it - but you seem to confuse the memory allocated by malloc() with the pointer to that allocated memory. There's a certain fuzziness there.

They are 2 seperate but linked things. You are allocating memory in RAM by using malloc() - the pointer isn't the memory - it simply holds the address where that memory starts. As such it's the only indicator where the memory actually is. If you lose the indicator - you no longer know where that memory is. So you can neither use it nor free it.

Originally Posted by anduril462

I've suggested this to you before at least once, but I feel like there have been multiple times. VALGRIND. Google it. Install it. Use it.

This is result of valgrind:

Code:

==17724== Memcheck, a memory error detector
==17724== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==17724== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==17724== Command: ./point-leak
==17724== 
--17724-- ./point-leak:
--17724-- dSYM directory is missing; consider using --dsymutil=yes
==17724== Conditional jump or move depends on uninitialised value(s)
==17724==    at 0xE7B9: strlen (mc_replace_strmem.c:412)
==17724==    by 0x100000CD4: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724== 
==17724== Conditional jump or move depends on uninitialised value(s)
==17724==    at 0xE7B9: strlen (mc_replace_strmem.c:412)
==17724==    by 0x100000CF7: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724== 


address: 0, current: 3.000000, pressure: address: 10000, current: 4.000000, pressure: 




address: 0, current: 3.000000, pressure: address: 10000, current: 4.000000, pressure: 


==17724== Conditional jump or move depends on uninitialised value(s)
==17724==    at 0xE7C7: strlen (mc_replace_strmem.c:412)
==17724==    by 0x100000CD4: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724== 
==17724== Conditional jump or move depends on uninitialised value(s)
==17724==    at 0xE7C7: strlen (mc_replace_strmem.c:412)
==17724==    by 0x100000CF7: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724== 
==17724== Conditional jump or move depends on uninitialised value(s)
==17724==    at 0xE7B9: strlen (mc_replace_strmem.c:412)
==17724==    by 0x1778C2: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17618D: vfprintf_l (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17F2CF: printf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x100000D85: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724== 
==17724== Conditional jump or move depends on uninitialised value(s)
==17724==    at 0xE7C7: strlen (mc_replace_strmem.c:412)
==17724==    by 0x1778C2: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17618D: vfprintf_l (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17F2CF: printf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x100000D85: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724== 


==17724== Conditional jump or move depends on uninitialised value(s)
==17724==    at 0x1B1EC8: memchr (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x1ABFB8: __sfvwrite (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17A946: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17618D: vfprintf_l (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17F2CF: printf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x100000D85: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724== 
==17724== Syscall param write(buf) points to uninitialised byte(s)
==17724==    at 0x2C61BA: write$NOCANCEL (in /usr/lib/system/libsystem_kernel.dylib)
==17724==    by 0x18059D: __sflush (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x1ABF6C: __sfvwrite (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17A946: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17618D: vfprintf_l (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17F2CF: printf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x100000D85: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724==  Address 0x1000043d0 is 0 bytes inside a block of size 4,096 alloc'd
==17724==    at 0xD810: malloc (vg_replace_malloc.c:295)
==17724==    by 0x17F3F7: __smakebuf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x175D19: __swsetup (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x1766C3: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17618D: vfprintf_l (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x17F2CF: printf (in /usr/lib/system/libsystem_c.dylib)
==17724==    by 0x100000D85: sendToIridium (in ./point-leak)
==17724==    by 0x100000DC8: main (in ./point-leak)
==17724==

From that, I cannot gather if there is a leak or not.

Since sensors is a pointer to a pointer, the i-th index of sensors points to *temp which points to the allocated memory on RAM. Hence, I can free that memory using the following:

Code:

void sendToIridium()
{
    int i;
    char wholeMsg[300];
 
 
     
    for (i = 0; i < sensors_count; i++)
    {
        sprintf(wholeMsg + strlen(wholeMsg),"address: %llu, current: %f, pressure: %c\n",
        sensors[i]->address, sensors[i]->current, sensors[i]->pressure_units);
    }
    printf("\n%s\n",wholeMsg);
   for (i = 0; i < sensors_count; i++)
   {
	  free(sensors[i]);	
   }
}

Second point is I cannot free sensors (which is a pointer to a pointer). Even though I use realloc on it, I cannot free it. Otherwise I get the following error:

malloc: *** error for object 0x10d500910: pointer being realloc'd was not allocated
*** set a breakpoint in malloc_error_break to debug

when calling free on sensors:

Code:

int main(int argc, char *argv[])
{
 
 
    while(1)
    {
        pollHart();
        sendToIridium();
		free(sensors);
        sleep(3);
    }
}

Thus, while I can free *sensors, I cannot free **sensors, which is the pointer to pointer.

I can't tell what options you ran valgrind with. The leak checker runs in summary mode by default, so you should have seen something like

Code:

==12190== HEAP SUMMARY:
==12190==     in use at exit: 104 bytes in 7 blocks
==12190==   total heap usage: 13 allocs, 6 frees, 141 bytes allocated
==12190==
==12190== LEAK SUMMARY:
==12190==    definitely lost: 64 bytes in 4 blocks
==12190==    indirectly lost: 0 bytes in 0 blocks
==12190==      possibly lost: 0 bytes in 0 blocks
==12190==    still reachable: 40 bytes in 3 blocks
==12190==         suppressed: 0 bytes in 0 blocks
==12190== Rerun with --leak-check=full to see details of leaked memory

Which instructs you to use the --leak-check=full option, which would give you a backtrace of the functions that lead to a malloc without a free

Code:

==12191== 64 bytes in 4 blocks are definitely lost in loss record 3 of 3
==12191==    at 0x4006D69: malloc (vg_replace_malloc.c:236)
==12191==    by 0x80485B1: createSensor (foo.c:19)
==12191==    by 0x804865F: checkSerialHart (foo.c:33)
==12191==    by 0x80486A5: pollHart (foo.c:48)
==12191==    by 0x80487B9: main (foo.c:77)

So yes you leak memory. You leak the malloc call in createSensor.

---

That conditional jump issue on line 64 is the sprintf(wholeMsg + strlen(wholeMsg), ...). That needs to be fixed, it's undefined behavior. You're relying on wholeMsg having a null character in the first position, which it might not, since it's an uninitialized automatic variable. That could technically be the cause of your problem, but it's probably not.

---

Adding your new free code, I get:

Code:

$ valgrind --leak-check=full ./foo
==12228== Memcheck, a memory error detector
==12228== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==12228== Using Valgrind-3.6.1 and LibVEX; rerun with -h for copyright info
==12228== Command: ./foo
==12228==
==12228== Conditional jump or move depends on uninitialised value(s)
==12228==    at 0x8048765: sendToIridium (foo.c:64)
==12228==    by 0x804881C: main (foo.c:82)
==12228==


address: 0, current: 3.000000, pressure: address: 10000, current: 4.000000, pressure:


==12228== Invalid free() / delete / delete[]
==12228==    at 0x4006E53: realloc (vg_replace_malloc.c:525)
==12228==    by 0x804862E: checkSerialHart (foo.c:30)
==12228==    by 0x80486D5: pollHart (foo.c:48)
==12228==    by 0x8048817: main (foo.c:81)
==12228==  Address 0x40270e0 is 0 bytes inside a block of size 8 free'd
==12228==    at 0x4005ECD: free (vg_replace_malloc.c:366)
==12228==    by 0x8048829: main (foo.c:83)
==12228==
==12228==
==12228== HEAP SUMMARY:
==12228==     in use at exit: 0 bytes in 0 blocks
==12228==   total heap usage: 6 allocs, 6 frees, 57 bytes allocated
==12228==
==12228== All heap blocks were freed -- no leaks are possible
==12228==
==12228== For counts of detected and suppressed errors, rerun with: -v
==12228== Use --track-origins=yes to see where uninitialised values come from
==12228== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 39 from 8)

That tells me there is an issue with the realloc, but only on the second iteration (note the first successful print of wholeMsg). That means, you screw something up in/after the first free. When you free sensors, it still contains the old memory address from realloc, but the memory there is no longer valid. You then pass that address to realloc, but that address is no longer an alloc'ed address, yet it seems you still try to free sensors[i] and sensors at that address, which have already been freed. Unfortunately, I have to run and can't finish this up.

NOTE: I seriously think you don't need any memory allocation in this program at all. Declare an array with the max number of sensors you'll have, and simply use sensor_count to tell you how many spots in the array are in use. It is functionally equivalent to what you're doing, wastes very little memory (3 sensors), and is free from these problems you're having.

Yeah I removed dynamic memory allocation and used array instead and I get same results but without memory leaks. The lesson is avoid dynamic memory at all costs, except when reading from serial cable or network.

OP, the secret to programming is, listen. And I mean, really listen. Valgrind is telling you exactly what you should fix. Stop using uninitialized variables and remember that for every malloc(), there must be a free() at that address.