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.

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**. — jpm
– jpm, Commented Nov 9, 2012 at 17:35
Doing a scanf("%s",invitingBufferOverflow); is just as terrible (or worse) as gets(invitingBufferOverflow) — Happy Green Kid Naps
– Happy Green Kid Naps, Commented Nov 9, 2012 at 19:34

score 8 · Accepted Answer · 2019-07-03 02:33:48Z

8

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);
}

...

edited Jul 3, 2019 at 2:33

answered Nov 9, 2012 at 17:52

user1157391

Sign up to request clarification or add additional context in comments.

1 Comment

jpm Over a year ago

+1 for the management functions. It's definitely preferable to having the alloc/dealloc logic strewn about client functions.

Mike · Accepted Answer · 2012-11-09 17:50:09Z

1

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.

answered Nov 9, 2012 at 17:50

Mike

49.9k30 gold badges119 silver badges181 bronze badges

1 Comment

Jcmoney1010 Over a year ago

hmmmm..I guess what I'm trying to do is assign memory to each one without doing it statically. I'm assuming that using malloc everytime I take in user input, doesn't doesn't do this? lifes hard being a noob lol.

jpm · Accepted Answer · 2012-11-09 17:50:25Z

0

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**.

answered Nov 9, 2012 at 17:50

jpm

3,15519 silver badges24 bronze badges

Comments

William Morris · Accepted Answer · 2012-11-09 19:25:14Z

0

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;

edited Nov 9, 2012 at 19:25

answered Nov 9, 2012 at 17:38

William Morris

3,7142 gold badges26 silver badges25 bronze badges

3 Comments

Jcmoney1010 Over a year ago

This is what I had in my original phonebook app, but after reading up on malloc I thought I would be able to make all these into char pointers and then use a buffer in the malloc function.

Aubin Over a year ago

This is not the OP ask for. It's a way to avoid problems not to solve it.

Jcmoney1010 Over a year ago

I think I might have misrepresented what I wanted to do in the original post. When the user enters information for a friend, I want the pointer to allocated appropriate space and the friend information be stored in this allocated space. i've read snippits other places that mention using a buffer array as an argument to scanf, but I'm just having a putting this all together

Collectives™ on Stack Overflow

Using malloc with structures in c

4 Answers 4

1 Comment

1 Comment

Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

1 Comment

Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related