Using malloc with structures in c

Question

So I'm trying add malloc to a phonebook application that I created, but since I'm kind of new to C I'm not sure if what I'm doing is correct. I've run into a small problem, but I've read through the beginner book that I have, and it doesn't go though as much detail as I would like, I can't tell by searching Google if I'm just completely wrong in how I set up the malloc or if there is something else I missed.

Basically what I've got are 4 arrays in my structure, First_Name, Last_name,home,cell. Each one of these have 2 functions, a function that gets the info from the user and a function that prints and adds the user info to the phonebook. What I've got right now is a small snipit of the original code that only adds the first name to the phonebook(so it's not the entire code) and in each function that gets the user input, I want to add the malloc function. Right now I've only got the first name and the first malloc set up, but the issue I have is that when I go to check the phonebook to see if the name was entered successfully, the program quits. If I take out the malloc, it works successfully.

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

#define BUFFER 50
    //Structure for contacts
typedef struct friends_contact {

    char *First_Name;
    char *Last_Name;
    char *home;
    char *cell;
} fr;

void menu(fr * friends, int *counter, int user_entry, int i);
void setFirst(fr *, int *, int i);
char getFirst(fr *, int i);
void add_contact(fr * friends, int *counter, int i);
void print_contact(fr * friends, int *counter, int i);

int main()
{

    int user_entry = 0;
    fr *friends;
    int counter = 0;
    int i = 0;
    menu(friends, &counter, user_entry, i);
    getch();
    return 0;
}

//Menu function
void menu(fr * friends, int *counter, int user_entry, int i)
{
    do {
        int result;

        printf("\nPhone Book Application\n");
        printf
            ("1) Add friend\n2) Delete friend\n3) Show a friend\n4)Showphonebook\n5)Exit\n");
        scanf("%d", &user_entry);

        if (user_entry == 1) {
            add_contact(friends, counter, i);
        }
        if (user_entry == 2) {

        }
        if (user_entry == 3) {

        }
        if (user_entry == 4) {
            print_contact(friends, counter, i);
        }
    } while (user_entry != 5);
}

void setFirst(fr * friends, int *counter, int i)
{
    // THE MALLOC FUNCTION!
    friends = (fr *) malloc(BUFFER * sizeof(fr));
    printf("Enter a first name \n");
    scanf("%s", friends[*counter].First_Name);
    if (friends != NULL) {

        free(friends);
    }
}

char getFirst(fr * friends, int pos)
{
    printf("%s ", friends[pos].First_Name);
    return *friends[pos].First_Name;
}

void add_contact(fr * friends, int *counter, int i)
{
    setFirst(friends, counter, i);
    (*counter)++;
}

void print_contact(fr * friends, int *counter, int i)
{
    for (i = 0; i < *counter; i++)
        if (strlen(friends[i].First_Name)) {
            getFirst(friends, i);
        }
}

Looking to give a big green check mark to whoever can help me out here.

Answer 1

You need to allocate memory both for the record as a whole and separately for each field. For example:

void string_realloc_and_copy (char **dest, const char *src)
{
  size_t len = strlen (src);
  *dest = realloc (*dest, len + 1);
  memcpy (*dest, src, len + 1);
}

typedef struct
{
  char *name;
  char *title;
} record;

record * record_new ()
{
  record *r = malloc (sizeof (record));
  r->name = NULL;
  r->title = NULL;
  return r;
}

void record_free (record *r)
{
  free (r->name);
  free (r->title);
  free (r);
}

void record_set_name (record *r, const char *name)
{
  string_realloc_and_copy (&r->name, name);
}

void record_set_title (record *r, const char *title)
{
  string_realloc_and_copy (&r->title, title);
}

Now to create a record and fill it with values read from the user:

record *r;
char buffer[100 + 1];

r = record_new ();

printf("Enter a first name \n");
if (scanf ("%100s", buffer) == 1) {
  record_set_name (r, buffer);
}

...

Answer 2

Your structure contains just pointers, not allocated memory. You would be better defining it to hold arrays into which you write names etc:

typedef struct friends_contact{

    char First_Name[20];
    char Last_Name[20];
    char home[20];
    char cell[20];
} fr;

Here I have made each field 20 characters long, but you can change that to suit.

---

Edit: yes of course you can use dynamic memory, but is it worth the bother? The advantage of dynamic strings is that they can be exactly the right size; you might save a few bytes and you guarantee being able to fit the names into the fields. But are there many names longer than 20 chars and would it matter to have to abbreviate a few? With malloc, there is a lot of fiddly allocation (each of which can fail) and freeing too, of course.

As a compromise one might make the phone numbers fixed size (they don't change) and the names dynamic; then allocate the names using strdup (which can also fail).

typedef struct friends_contact{

    char *First_Name;
    char *Last_Name;
    char home[12];
    char cell[12];
} fr;

Answer 3

There are several more things to consider here, but for a start consider the following.

In setFirst, you're freeing your friends buffer, in essence saying "I don't need this anymore." When you do this, that memory just goes away. If you're going to dynamically allocate structures for the caller, you either have to provide a separate deallocation function, or let your user know it's their responsibility to clean up that structure.

Also, you're only ever changing the local copy of the friends pointer. If you want to point the caller's pointer to a new buffer, you need to change the argument type to fr**.

Answer 4

Got some problems here:

void setFirst(fr*friends, int* counter, int i) {
   // THE MALLOC FUNCTION!
   friends=(fr*) malloc(BUFFER*sizeof(fr));  <-- This is not doing what you're thinking

sizeof(fr) is going to be the size required for 4 pointers to character. For example if you're on a 32-bit x86 platform it takes 4 bytes for a pointer to a char, thus:

sizeof(fr) == 4 x 4 == 16 bytes

So now you're malloc'ing 16*BUFFER or 16x50 = 800 bytes. This allows you to have an array of 50 'fr' structures.

fr * friend
        |
        +--------> FirstName*
            |      LastName*
            |      home*
            |      cell*
            +----> FirstName*
            |       LastName*
            |      home*
            |      cell*
            ...

So you've got the memory for 50 structures, but the contents of those structures still don't have memory. You need to assign memory to each member of the structure (and don't forget to free all those as well), or you could make them static members with arrays instead of pointers.

Second problem:

if(friends != NULL)  <-- if malloc was successful
{
     free(friends);  <-- release the memory

You just lost all your friends. :)
You do need to free the memory but at the end of the program or at the end of where you're using it. If you assign and then free right away, then the memory is gone and you can't access it anymore.