1

I am trying to use structures and pointers in C for the first time. And I don't really understand why I keep getting a segmentation fault, even though the program compiles perfectly. I am also little confused with memory allocation. Do we always use a cast in memory allocation every time?

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



#define HOW_MANY 7

char *names[HOW_MANY]= {"Adam", "James", "Matt", "Affleck", "Benedict", "Kayne",
    "Evans"};
int ages[HOW_MANY]= {22, 24, 46, 56, 21, 32, 30};

/* declare your struct for a person here */
typedef struct{
    char name[HOW_MANY];
    int age;
}person;


static void insert(person *people[], char *name, int age)
{
    static int nextfreeplace = 0;
    // Allocating memory here
    people = malloc(sizeof(person));

    if (people == NULL) {
        printf("Couldn't allocate memory");
        exit(-1);
    }

    strcpy((*people[nextfreeplace]).name,name);

    (*people[nextfreeplace]).age = age;

    nextfreeplace++;
}

int main(int argc, char **argv)
{
    person *people[HOW_MANY];

    for (int i = 0; i < HOW_MANY ; i++)
    {
        insert (people, names[i], ages[i]);
    }

    /* print the people array here*/
    for (int i = 0; i < HOW_MANY ; i++)
    {

        printf("The person's name is %s and the age is %d.\n",(*people[i]).name,(*people[i]).age);
    }

    for (int i = 0; i < HOW_MANY ; i++)
    {
        free(people[i]);
    }

    return 0;
}

Is this a proper way of allocating memory?

abelenky
  • 63,815
  • 23
  • 109
  • 159
Shamveel Ahammed
  • 266
  • 2
  • 3
  • 12

3 Answers3

1

Keep the same logic through your C code

Call insert providing a pointer to where you want to allocate memory, i.e. the address of the pointer location in the people array ; in the main function for loop:

insert (&people[i], names[i], ages[i]);

Thus in insert, 

static void insert(person **people, char *name, int age)
{
    *people = malloc(sizeof(person));

    if (*people == NULL) {
        printf("Couldn't allocate memory");
        exit(-1);
    }

    strcpy((*people)->name,name);

    (*people)->age = age;
}
Déjà vu
  • 28,223
  • 6
  • 72
  • 100
  • Good answer! My solution was going to be sending `i` and `people` but sending the address of the pointer to the person makes more sense. It might be worth mentioning why you got rid of `nextfreeplace`. In case the poster reads comments: since `main` is calling `insert` for each person, you don't have a need for your `insert` method to keep track of which person you are on. – Greg Schmit Nov 03 '16 at 01:33
0

Well, seems that you have 2 errors here:

  • one as pointed by Eugene, is the malloc, you should store the allocated memory in the proper place, an element of the array of pointers to "person" structure; you need:

    people[nextfreeplace] = malloc(sizeof(person));

  • the second, the name is too small, you should allocate more chars or truncate it... (Update: BLUEPIXY published the code while I was writing the reply... see his code, no need for me to write it again...). In short "Benedict" has 8 characters, so... you need at least 9 chars allocated for "name" in the struct (string terminator takes one character too).

    typedef struct{
  char name[9];  // 8 is already not good
  int age;
}person;

if you solve them, your code works well...

Prak
  • 302
  • 1
  • 9
0

I use two very important tools for getting visibility into memory problems. The first is compiler warnings. They're not on by default, you have to turn them on with -Wall. Your program doesn't have any warnings, good.

Second is to use a memory checker, something that will look for memory problems. This avoids having to do a lot of careful study. I use valgrind which shows memory violations along with the stack.

==90314== Conditional jump or move depends on uninitialised value(s)
==90314==    at 0x1013AD570: _platform_memmove$VARIANT$Nehalem (in /usr/lib/system/libsystem_platform.dylib)
==90314==    by 0x10112A421: stpcpy (in /usr/lib/system/libsystem_c.dylib)
==90314==    by 0x10119DBED: __strcpy_chk (in /usr/lib/system/libsystem_c.dylib)
==90314==    by 0x100000E9A: insert (test.c:31)
==90314==    by 0x100000D8A: main (test.c:44)
==90314== 
==90314== Use of uninitialised value of size 8
==90314==    at 0x1013AD5C0: _platform_memmove$VARIANT$Nehalem (in /usr/lib/system/libsystem_platform.dylib)
==90314==    by 0x10112A421: stpcpy (in /usr/lib/system/libsystem_c.dylib)
==90314==    by 0x10119DBED: __strcpy_chk (in /usr/lib/system/libsystem_c.dylib)
==90314==    by 0x100000E9A: insert (test.c:31)
==90314==    by 0x100000D8A: main (test.c:44)
==90314== 
==90314== Invalid write of size 1
==90314==    at 0x1013AD5C0: _platform_memmove$VARIANT$Nehalem (in /usr/lib/system/libsystem_platform.dylib)
==90314==    by 0x10112A421: stpcpy (in /usr/lib/system/libsystem_c.dylib)
==90314==    by 0x10119DBED: __strcpy_chk (in /usr/lib/system/libsystem_c.dylib)
==90314==    by 0x100000E9A: insert (test.c:31)
==90314==    by 0x100000D8A: main (test.c:44)
==90314==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==90314== 
==90314== 
==90314== Process terminating with default action of signal 11 (SIGSEGV)
==90314==  Access not within mapped region at address 0x0
==90314==    at 0x1013AD5C0: _platform_memmove$VARIANT$Nehalem (in /usr/lib/system/libsystem_platform.dylib)
==90314==    by 0x10112A421: stpcpy (in /usr/lib/system/libsystem_c.dylib)
==90314==    by 0x10119DBED: __strcpy_chk (in /usr/lib/system/libsystem_c.dylib)
==90314==    by 0x100000E9A: insert (test.c:31)
==90314==    by 0x100000D8A: main (test.c:44)

That indicates there's a problem with your call to strcpy.

strcpy((*people[nextfreeplace]).name,name);

One of the arguments is uninitialized, either name or *people[nextfreeplace]).name. name came from names so that's probably not it.

The problem is how people is allocated. people is a pointer to an array of person pointers. But you blow over that with memory for a single person.

people = malloc(sizeof(person));

C will let you do that without a warning because malloc returns a void * that will happily morph into whatever pointer type.

Instead you should allocate space for one person, and then put a pointer to that into people.

static void insert(person *people[], char *name, int age)
{
    static int nextfreeplace = 0;

    // Allocating memory here
    person *human = malloc(sizeof(person));
    if (human == NULL) {
        printf("Couldn't allocate memory");
        exit(-1);
    }

    strcpy(human->name, name);
    human->age = age;
    people[nextfreeplace] = human;

    nextfreeplace++;
}

That also points out that you should name your types like Person or Person_t to avoid conflicting with built in types and good variable names.

That reveals the next problem, and again it's strcpy. It's always strcpy.

==5111== Process terminating with default action of signal 6 (SIGABRT)
==5111==    at 0x101269F36: __pthread_sigmask (in /usr/lib/system/libsystem_kernel.dylib)
==5111==    by 0x10117876C: __abort (in /usr/lib/system/libsystem_c.dylib)
==5111==    by 0x1011786ED: abort (in /usr/lib/system/libsystem_c.dylib)
==5111==    by 0x101178855: abort_report_np (in /usr/lib/system/libsystem_c.dylib)
==5111==    by 0x10119EA0B: __chk_fail (in /usr/lib/system/libsystem_c.dylib)
==5111==    by 0x10119E9DB: __chk_fail_overflow (in /usr/lib/system/libsystem_c.dylib)
==5111==    by 0x10119EC28: __strcpy_chk (in /usr/lib/system/libsystem_c.dylib)
==5111==    by 0x100000E8F: insert (test.c:31)
==5111==    by 0x100000D8A: main (test.c:44)

strcpy is vulnerable to overflowing a buffer. Is there enough space allocated in person->name? Let's see...

typedef struct{
    char name[HOW_MANY];
    int age;
}person;

Nope. HOW_MANY is 7. The mistake there is thinking name is a list of names. Instead it's how long a name can be.

If we change that to something reasonable like 32, it works!

The person's name is Adam and the age is 22.
The person's name is James and the age is 24.
The person's name is Matt and the age is 46.
The person's name is Affleck and the age is 56.
The person's name is Benedict and the age is 21.
The person's name is Kayne and the age is 32.
The person's name is Evans and the age is 30.

Valgrind is happy, but because of the static allocations in your code it's vulnerable to buffer overflow. If one of those names was "Johnathan Jacob Jingleheimerschmit" you'd have a buffer overflow.

You have two choices. Statically allocate WAY more memory than you need, or use dynamic memory.

In general...

Avoid static memory allocations.

It's a really bad habit to get into. It invites all sorts of memory overflows. Get used to dynamically allocating and reallocating memory, or use structures that will grow naturally like linked lists and trees.

Use a string library.

Strings in C are just a nightmare. A general purpose C library like Gnome Lib provides functions for manipulating strings safely and their own improved String type.

Always write a new and destroy for your structs.

Allocating and deallocating memory for structures can get tricky. It's best to put that all into their own functions rather than relying on your code to get it right willy-nilly. Even if you think it's trivial, it might get not trivial later.

typedef struct {
    char *name;
    int age;
} Person_t;

/* So we don't have to check if a thing is null before freeing it */
static void free_if(void *thing) {
    if( thing != NULL ) {
        free(thing);
    }
}

static Person_t* Person_new() {
    /* calloc() is used here to 0 the structure so we don't use garbage */
    Person_t *person = calloc(1, sizeof(Person_t));
    if (person == NULL) {
        fprintf(stderr, "Couldn't allocate memory for Person_t: %s", strerror(errno));
        exit(-1);
    }

    return person;
}

static void Person_destroy(Person_t *person) {
    free_if(person->name);
    free(person);
}

Note that I've moved to using dynamically allocated memory for the name. Which brings us to the next point.

Write functions to deal with struct memory.

When you need to mess with allocating and copying things into your struct, make it a function that handles all that for you.

static void Person_set_name(Person_t *person, char *name) {
    free_if(person->name);
    person->name = malloc( (strlen(name) + 1) * sizeof(char) );
    strcpy( person->name, name );
}

Treat structs like objects, write methods for them.

If you're familiar with object-oriented programming in other languages you can see where this is all going. Treat a struct like an object. Write "methods" for it. Do all the work in those methods, not in the code using the struct.

This encapsulates all the work of managing the struct into pieces you can easily build, manage, and test.

Schwern
  • 153,029
  • 25
  • 195
  • 336