memory allocation eror in C

Question

I have been working on the following code for a while and there is a problem when I tried to allocate/deallocate memory for a struct and its elements. Any insights into the problem would be greatly appreciated thanks. Looking at the error message, I believe the problem lies in the fact that I tried to free an element that I had not properly allocated memory for but it is not obvious to me while looking at the code. I also tried code where I did not individually allocate memory for each element of the struct but that did not work as well.

typedef struct {
  char *cnet;
  char *email;
  char *fname;
  char *lname;
  char *tel;
} vcard;

vcard *vcard_new(char *cnet, char *email, char *fname, char *lname, char *tel)
{
  vcard* new = (vcard*)malloc(sizeof(vcard));
  printf("%lu\n", sizeof(new->tel) );
  new->cnet = malloc(sizeof(new->cnet));
  new->email = malloc(sizeof(new->email));
  new->fname = malloc(sizeof(new->fname));
  new->lname = malloc(sizeof(new->lname));
  new->tel = malloc(sizeof(new->tel));
  new->cnet = cnet;
  new->email = email;
  new->fname = fname;
  new->lname = lname;
  new->tel = tel;
  return new;
}

/* vcard_free : free vcard and the strings it points to
 */
void vcard_free(vcard *c)
{
  free(c->cnet);
  free(c->email);
  free(c->fname);
  free(c->lname);
  free(c->tel);
  free(c);
  return;
}

Reames

sizeof(new->cnet) = size of a pointer, and I seriously suspect that is not intentional. Rinse/repeat for every allocation in that function Not that it matters much, since you leak all of those allocations later in the same function. Assignment operators are not how strings are copied in C; it's how values are copied, and in this case, they're the values of pointers. A good C text and the section on strings, followed by the section on dynamic memory, will help immensely. – Thalassic

This is a C question, but if you ever want to compile your code with a C++ compliler, I would rethink about naming a variable new :-). – Quinnquinol

Answer 1

Your entire memory allocation is erroneous. Here are some pointers.

1. You allocate memory for only one char *, which is not what is intended.

   • Lesser memory allocation, possibility of boundary overrun.

2. Then, you overwrite the allocated memory by assigning the parameters to the same variables which held the pointer to allocated memories.

   • You end up causing memory leak.
3. You try to free the pointers which are not returned by malloc() and family.
   • Invalid attempt to free(), causes undefined behavior
4. You have used wrong format specifier in printf()
   • sizeof yields a result of type size_t, you must use %zu to print the result.

---

Solutions:

1. Allocate enough memory to store the expected content, like in predefined sizes

    #define CNETSIZ 32
    #define EMAILSIZ 64
    . . . . . 
    new->cnet = malloc(CNETSIZ);
    new->email = malloc(EMAILSIZ);
   

   or, based on the length of the input string, like

    new->cnet = malloc(strlen(cnet)+1);  //+1 for the space to null-terminator
   

2. Inside the vcard_new() function, use strcpy() to copy the content from the function parameters, like

   strcpy(new->cnet, cnet);
   strcpy(new->email, email);
   

Notary

Recommended: avoid use of the English word 'pointer' for something other than what it means in C. Choose from the many available synonyms... – Commissar

@Commissar That's punny. :) – Notary

Answer 2

You're just allocating enough memory to hold another char* instead of the actual string. And then you overwrite the pointer with the pointer passed. Later you try to free that pointer, which means that it will try to free the memory of which you took a pointer and passed it to the function. Instead you should do this:

vcard *vcard_new(char *cnet, char *email, char *fname, char *lname, char *tel)
{
  vcard* new = (vcard*)malloc(sizeof(vcard));
  new->cnet = malloc(strlen(cnet)+1);
  strcpy(new->cnet, cnet);
  ...
}

Or, if strdup is available on your system:

vcard *vcard_new(char *cnet, char *email, char *fname, char *lname, char *tel)
{
  vcard* new = (vcard*)malloc(sizeof(vcard));
  new->cnet = strdup(cnet);
  ...
}

Unspent

Thank you very much for your suggestion. It all works now. – Reames

Answer 3

Using new->cnet = malloc(sizeof(new->cnet)) you are allocating an amount of memory for cnet string equals to the pointer size and not to the actual string length.

Chthonian