Freeing memory in C

Question

I'm having problem with this small program:

UPDATED (As per some requests, I've included everything here so to make clear what I'm doing. Sorry for it being too long): Student.h file:

typedef struct Student {
  char *name;
  int age;
  char *major;
    char *toString;
} *Student;

extern Student newStudent(char *name, int age, char *major);

Student.c file: c

har *studentToString(Student s);
static void error(char *s) {
  fprintf(stderr,"%s:%d %s\n",__FILE__,__LINE__,s);
  exit(1);
}

extern Student newStudent(char *name, int age, char *major) {
  Student s;
    if (!(s=(Student)malloc(sizeof(*s)))){
    error("out of memory");
    }
  s->name=name;
  s->age=age;
  s->major=major;
    s->toString = studentToString(s);
  return s;
}

char *studentToString(Student s) {
  const int size=3;
  char age[size+1];
  snprintf(age,size,"%d",s->age);

  char *line=newString();
  line=catString(line,"<");
  line=catString(line,s->name);
  line=catString(line," ");
  line=catString(line,age);
  line=catString(line," ");
  line=catString(line,s->major);
  line=catString(line,">");
  return line;
}

Students.c file:

static void error(char *s) {
  fprintf(stderr,"%s:%d %s\n",__FILE__,__LINE__,s);
  exit(1);
}

static StudentList alloc(StudentList students, Student student) {

  StudentList p;
    if (!(p=(StudentList)malloc(sizeof(*p)))){
    error("out of memory");}
  p->student=student;
  p->students=students;
  return p;
}

extern Students newStudents() {
  Students p;
    if (!(p=(Students)malloc(sizeof(*p)))){
    error("out of memory");
    }
  p->cursor=0;
  p->students=0;
  return p;
}

extern void addStudent(Students students, Student student) {
  StudentList p=students->students;
  if (!p) {
    students->students=alloc(0,student);

    return;
  }
while (p->students)
     p=p->students;
  p->students=alloc(0,student); 
    }    

extern void initStudent(Students students) {
  students->cursor=students->students;
}

extern Student currStudent(Students students) {
  StudentList cursor=students->cursor;
  if (!cursor)
    return 0;
  return cursor->student;
}

extern void nextStudent(Students students) {
  students->cursor=students->cursor->students;
}

And my main method:

int main() {
  Students students=newStudents();
  addStudent(students,newStudent("Julie",22,"CS"));
  addStudent(students,newStudent("Trevor",32,"EE"));

  for (initStudent(students);
       currStudent(students);
       nextStudent(students)) {
      char *line=currStudent(students)->toString;
    printf("%s\n",line);
      free(currStudent(students));
    free(line);
  }

    free(students->students);
    free(students);
  return 0;
}

I'm using valgrind to check memory leaks, and it is popping following error:

8 bytes in 1 blocks are definitely lost in loss record 1 of 1
==9520==    at 0x40054E5: malloc (vg_replace_malloc.c:149)
==9520==    by 0x8048908: alloc (Students.c:13)
==9520==    by 0x80489EB: addStudent (Students.c:42)
==9520==    by 0x804882E: main (StudentList.c:10)

I understand that I need to free the memory allocated for p in alloc function, but where should I call free(p)? Or is there something else I'm doing wrong? Please help!

Answer 1

I think you have a problem in alloc.

If you want a pointer it should be

StudentList *p;
p = (StudentList*)malloc(sizeof(StudentList));

Answer 2

I don't ever use sizeof with a variable if I am using malloc(). You should be using malloc(n * sizeof(StudentList)). With that said...

You've got some major concerns. First of all, you don't tell us what Students and StudentList are specifically defined to be. You pass 0 in some cases when I think you meant NULL -- never use 0 when you mean NULL. And if malloc() fails -- meaning, it returned NULL--then you don't call free() on the result. Never, ever free NULL.

You should only free a block of memory that was (a) successfully allocated, and (b) when you don't need it anymore. That doesn't seem to enter into your code here.

There are other issues as well (you assume students in addStudent is non-NULL when you initialize p), but they don't quite address your question concerning the usage of free().

Answer 3

What is this doing:

if (!(p=(StudentList)malloc(sizeof(*p)))){
    free(p);

So, if p can't be allocated, free it?

Answer 4

The question is what do you do when you are finished with a Student or a StudentList and don't need it any more. That's the point where you should call free() for all the allocated things in that structure.

Probably you would want a freeStudents function that walks through a list of students and frees all the Students and all the StudentList items in it. Then you call that function whenever you want to get rid of a list of students.

Answer 5

I am assuming that StudentList is typedefined to be some type of pointer to Student. This being the case, this line in your alloc function is a problem. p=(StudentList)malloc(sizeof(*p))

You are attempting to dereference the pointer before it has been assigned and that is not good.

Answer 6

You should free the memory when you no longer need to use the objects you've allocated.

What are StudentList and Student? It looks to me like your data structure is constructed improperly. Why is your StudentList storing a pointer to another StudentList? You should probably create a StudentList using malloc and return the pointer to that using your newStudents function, and rather than returning a new StudentList struct with a copy of the old one in addStudent, simply modify the structure you created originally.

So it would go something like this:

StudentList *newStudentList() {
    //malloc a new StudentList and return the pointer
}
void freeStudentList(StudentList *list) {
    //free the list pointer and all its child resources (i.e. the students in the list)
}
Student *newStudent(const char *name, etc) {
    //malloc a new Student and return the pointer
}
void addStudentToList(StudentList *list, Student *student) {
    //modify the StudentList which has been passed in by adding the student to it.
}

You could consolidate newStudent into addStudent if you don't plan on using them separately, and you may need a freeStudent and removeStudentFromList function if you plan on doing those things separately from freeStudentList as well. You should look at Google for examples of dynamic data structures in C (here is the first result on Google, but there are many others).

Answer 7

You really need to define what your types are. Could you edit your post to include the definitions of Student, StudentList and Students?

Also,

StudentList p;
p = (StudentList) malloc(sizeof(*p);

Unless StudentList is a typedef for a pointer, I'm not sure how that's compiling.

We also have the line:

StudentList p=students->students

With p being defined as a Students. Then Students must be a typedef for a pointer as well.

---

Also, what I think is your issue in the end, is that when you try to insert a student in the linked list, you end up losing any existing list. What would be interesting for you to try would be to insert 3 students into the list, and then attempt to print the list.

Answer 8

Sorry, this is tangential, but you could really do a lot to make your code more readable.

You make a struct and then redefine its type to be a pointer to it. Yikes, that's just asking for trouble when it comes to maintainability. It's usually a bad idea to hide the fact that pointers are pointers, because when people see something like this:

Students newStudents()
{
  Students p;
  // ...
  return p;
}

convention forces us to assume that you're returning a struct that was allocated on the stack, which would be obviously incorrect. (Edit: Not necessarily "obviously incorrect", but a wasteful copy.)

Things get even hairier when you add your malloc...

Students p;
if (!(p=(Students)malloc(sizeof(*p))))
{
  error("out of memory");
}

For one thing, as mentioned, people assume that with no *, Students is a full structure on the stack. This will make anyone that sees "sizeof(*p)" do a double take. It's not obvious what you're doing.

And while squishing assignments and comparisons into one if statement is perfectly valid C and C++, it's usually not the most readable solution. Much improved:

Students* newStudents ()
{
  Students* p = (Students*) malloc (sizeof (Students));
  if (p == NULL)
  {
     // ...
  }
  // ...
  return p;
}

And people enjoy pointing out that casting the return value of malloc isn't necessary in C, but it is in C++.

As for your leak, well... valgrind didn't report your catString usage, but it's still pretty sketchy since you're hiding the memory usage. Using snprintf is a better, more idiomatic way to create the string you want.

The leak valgrind is reporting: it looks like you're just freeing the first "students" node in your list. You need to traverse it and free them all, probably like this:

Students p = students;
while (p)
{
  Students next = p->students;
  free (p);
  p = next;
}