CODEWITHSUNDEEP
cmemory-managementmalloc
johan
johan

Reputation: 1983

Memory allocation in a C program

I have the code block below, I deleted all unnecessary parts, just left the problematic part. My purpose to fetch the time in a specific format that I need(YYYYMMDDHHmm) by fetch_time function. In order to return char array, I used malloc. However, the program crashes if I run the code below like a minute. When I monitor run-time of the code by a debugging tool, the memory location that p1 points increases e.g 0x72120 for the first iteration, 0x72150 for the second iteration, and so on. So, I suspect it fails because of the memory problem. I wonder what do I do wrong and how can I solve the problem?

Btw, I guess I can solve the problem by defining a global char array and assining the time info in the sub-function. I would like to learn my mistake in the malloc usage and learn the solution. Thank you.

int main(int argc,char *argv[]){
    char timedate2[13];
    char *p1 = malloc(strlen(timedate2)+1);
    if(!p1){exit(1);}

    while(1){
        p1=fetch_time();
    }
}

char *fetch_time() {
    char *p;
    time_t rawtime;
    struct tm * timeinfo;
    char buffer [13];

    time ( &rawtime );
    timeinfo = localtime ( &rawtime );
    strftime (buffer,13,"%04Y%02m%02d%02k%02M",timeinfo);
    p = (char *)malloc(sizeof(buffer));
    strcpy(p, buffer);
    return p;
}

Upvotes: 0

Views: 1814

Answers (7)

Denis Novikov
Denis Novikov

Reputation: 143

Don't You forget to free the allocated memory? The code worked for 13 minutes and did not crashed.

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

char* fetch_time();

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

        char timedate2[13];
        char *p1 = malloc(strlen(timedate2)+1);
        if(!p1){exit(1);}
        free(p1);

        while(1){
               p1=fetch_time();
               free(p1);
        }
    }

    char* fetch_time() {
        char *p;
        time_t rawtime;
        struct tm * timeinfo;
        char buffer [13];

        time ( &rawtime );
        timeinfo = localtime ( &rawtime );
        strftime (buffer,13,"%04Y%02m%02d%02k%02M",timeinfo);
        p = (char *)malloc(sizeof(buffer));
        strcpy(p, buffer);

        return p;
    }

I see one more issue in the lines below.

char timedate2[13];
char *p1 = malloc(strlen(timedate2)+1);

'strlen' function searches for the first symbol '\0' in the given array of chars. But in this case the content of the array 'timedate2' is undefined and the symbol '\0' may be found anywhere.

Upvotes: 1

MYMNeo
MYMNeo

Reputation: 836

Modify your function as below

while(1){
   fetch_time(p1, strlen(timedate2) + 1);
  }
}
void fetch_time(char * time_str, int format_len)
    time_t rawtime;
    struct tm * timeinfo;
    char buffer [13];

    time ( &rawtime );
    timeinfo = localtime ( &rawtime );
    strftime (buffer,13,"%04Y%02m%02d%02k%02M",timeinfo);
    strncpy(p, buffer, format_len);
}

to avoid leak memory and other unexpected errors.

Upvotes: 1

Manik Sidana
Manik Sidana

Reputation: 2163

There is a big MEMORY LEAK in your program.
First of all, you are allocating memory to p1 in main().You then have an infinite loop for the statement p1=fetch_time();
In function fetch_time(), you are allocating memory to p and copying buffer. This address is returned to main(). You now lose the memory address for p1 which you allocated in main.
This loop runs infinite no of times thus it would reach a stage when the memory reaches its maximum limit.
FAULT 1:Memory allocated to p1 not freed.
FAULT 2: Function fetch_time keeps on allocating memory to p in each call and does not frees it. FAULT 3: Calling malloc(strlen(timedate2)+1);on an uninitialized string

Upvotes: 1

Giovanni Funchal
Giovanni Funchal

Reputation: 9200

The code looks correct per see. There's one catch thought. When you call malloc, you are requesting some amount of memory in which you store your date string. Everytime you call fetch_time you will request some (other) piece of memory with a new string. You never re-use the same piece of memory and eventually run out of memory. To fix that, you need to free every piece of memory you requested which will allow the system to eventually reuse it again.

while(1) {
    p1=fetch_time();
    // do something with p1
    free(p1);
}

Also, please fix the indentation.

If you find similar problems in the future, consider using a tool like valgrind, it will help you a lot (google it up!).

Upvotes: 2

John3136
John3136

Reputation: 29266

char timedate2[13];
/* timedate2 is not initialized, so strlen is unpredictable at best*/
char *p1 = malloc(strlen(timedate2)+1);

You allocate p1 then never use it, but overwrite it with he call to fetch_time() - this is a memory leak.

fetch_time() seems ok, but it returns malloced memory which is never freed [memory leak] when you do this in a tight loop (e.g. while(1)), it will very quickly allocate all the available memory, and then boom

Upvotes: 2

A. K.
A. K.

Reputation: 38264

why are you reassigning p1(already pointing to allocated memory).

Also, calling fetch_time() indefinitely will cause heap overflow.

What you can do is pass the p1(pointer) as argument to the fetch_time() and copy the timeinfo into the already allocated memory.

At last, remember to free the memory allocated by your program.

Upvotes: 0

Daniel Fischer
Daniel Fischer

Reputation: 183978

char timedate2[13];
char *p1 = malloc(strlen(timedate2)+1);

timedate2 is uninitialised, so there's no way to guess (reliably) how much you try to allocate, and there's the possibility that the strlen call will access invalid memory and cause a segfault. Did you mean to allocate

char *p1 = malloc(sizeof timedate2 + 1);

?

But the main problem is that fetch_time returns a newly allocated buffer each time it is called, and that is never freed.

Upvotes: 2

Related Questions