Change the value of pointed to by a void pointer

Question

I'm not sure how I can store and modify an int using the following stucts in C. I don't think the values that I insert into a student struct are being stored. The student grade must stay void* and I need to be able to access/modify the grade at anytime until exiting.

typedef struct student{
    void *grade;
} student;

typedef struct school{
    student **students;
    size_t school_size;
}school;

student *student_create(int x){
    student *new_student = malloc(sizeof(student));
    new_student->grade = malloc(sizeof(int));
    new_student->grade = &x;
    return new_student;
}

school *school_create(size_t school_szie) {
    school *s = (school*)malloc(sizeof(school));
    s->students = (student**)malloc(sizeof(student*) * school_szie);

    for (int i = 0; i < school_szie; ++i) {
        s->students[i] = NULL;
    }
    return s;
}

void school_insert(school *s, int count, int index){
    if(s->students[index] == NULL){
        student *new_s = student_create(count);
        s->students[index] = new_s;
    }
}

void student_inc(school *s, int index){
    *(int *)s->students[index]->grade = *(int *)s->students[index]->grade + 1;
}

int main(){

    int a = 10;
    void *ptr = &a;

    printf("%d\n", *(int *)ptr);
    *(int *)ptr = *(int*)ptr + 1;
    printf("%d\n", *(int *)ptr);

    school *s1 = school_create(5);
    
    school_insert(s1, 2, 1); // school name, grade, index
    school_insert(s1, 4, 0);
    school_insert(s1, 7, 3);
    
    printf("%d\n", *(int *)s1->students[1]->grade);
    printf("%d\n", *(int *)s1->students[0]->grade);
    printf("%d\n", *(int *)s1->students[3]->grade);

    return 0;
}

I'm getting the following output,although the intended output of the last three lines should be 2, 4, 7.

You have a memory leak in `student_create()` and set grade to the address of a local variable that becomes invalid when the function returns... — Shawn, Sep 08 '21 at 08:30
In `school_create` you have the assignment `new_student->grade = &x` which will make `new_student->grade` point to the ***local*** variable `x`. The life-time of `x` will end when the function ends, making the pointer invalid. Any dereference of this pointer afterward will lead to *undefined behavior*. — Some programmer dude, Sep 08 '21 at 08:30
Think about what these two lines do: `new_student->grade = malloc(sizeof(int)); new_student->grade = &x;` The first assigns the pointer to space to store an int. The second overwrites that pointer with the address of a local variable that will be invalid as soon as the function ends. It also create a memory leak. Perhaps you want to copy the value of `x` into the space allocated for it by the first line? — Retired Ninja, Sep 08 '21 at 08:30
I'm guessing what you really want is something like `*(int *)new_student->grade = x`? — Some programmer dude, Sep 08 '21 at 08:32
Also in `school _insert` you forgot `s->school_size = school_szie;`. — Jabberwocky, Sep 08 '21 at 08:34
Having `void *grade;` in `struct student` looks like very, very poor (if not worse) design to me. Is it part of some assignment? Tell us more. It should be `int grade;`. Also `struct student` should contain at least a name or some student identifier. — Jabberwocky, Sep 08 '21 at 08:37
By the way, there has been [some discussion about casting the result of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) and the consensus seems to be *not* to cast. — Some programmer dude, Sep 08 '21 at 08:41

Jabberwocky · Accepted Answer · 2021-09-08T09:03:07.657

There are several issues. Look at the commented lines.

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

typedef struct student {
  void* grade;
} student;

typedef struct school {
  student** students;
  size_t school_size;
}school;

student* student_create(int x) {
  student* new_student = malloc(sizeof(student));
  new_student->grade = malloc(sizeof(int));
  *(int*)(new_student->grade) = x;            // changed
  return new_student;
}

school* school_create(size_t school_szie) {
  school* s = (school*)malloc(sizeof(school));
  s->students = (student**)malloc(sizeof(student*) * school_szie);

  for (size_t i = 0; i < school_szie; ++i) {
    s->students[i] = NULL;
  }

  s->school_size = school_szie;               // you forgot this
  return s;
}

void school_insert(school* s, int count, int index) {
  if (s->students[index] == NULL) {
    student* new_s = student_create(count);
    s->students[index] = new_s;
  }
}

void student_inc(school* s, int index) {
  *(int*)s->students[index]->grade = *(int*)s->students[index]->grade + 1;
}

int main() {

  int a = 10;
  void* ptr = &a;

  printf("%d\n", *(int*)ptr);
  *(int*)ptr = *(int*)ptr + 1;
  printf("%d\n", *(int*)ptr);

  school* s1 = school_create(5);

  school_insert(s1, 2, 1); // school name, grade, index
  school_insert(s1, 4, 0);
  school_insert(s1, 7, 3);

  printf("%d\n", *(int*)(s1->students[1]->grade));   // changed
  printf("%d\n", *(int*)(s1->students[0]->grade));   // changed
  printf("%d\n", *(int*)(s1->students[3]->grade));   // changed

  return 0;
}

This doesn't make sens for two reasons:

new_student->grade = malloc(sizeof(int));
new_student->grade = &x;

new_student->grade = &x overwrites new_student->grade that has been allocated on the previous line
new_student->grade = &x puts the pointer to the local variable x into new_student->grade, but this local variable will cease to exist once the student_create function is over.

You want: *(int*)(new_student->grade) = x;. You've done it right with your tests using a and ptr at the beginning of main.

You forgot to assign the school size in school_create

 s->school_size = school_szie;               // you forgot this

This is wrong:

printf("%d\n", s1->students[1]->grade);

s1->students[1]->grade is the pointer to the grade, but you want the grade itself. Here you pretend s1->students[1]->grade is actually an int, and printf prints this as if it was an int. This is undefined bahaviour, the value printed can be any garbage value.

Therefore you need *(int *)s1->students[1]->grade. (int *) casts the pointer to void to a pointer to int, and the * dereferences the pointer to int.

Finally you should use size_t isntead of int in this for loop:

for (size_t i = 0; i < school_szie; ++i)

school_szie being a size_t.

This last problem however has no impact on the program output here.

That being said, be aware that having void *grade in struct student looks like very, very poor (if not worse) design to me. It should be int grade. You want to store a grade.

Also struct student should probably contain at least a student name or some student identifier.

score 0 · Answer 2 · answered Sep 08 '21 at 08:38

I compiled your program with gcc -g main.c -o main and ran it with valgrind ./main to find memory issues quickly. Try it!

Valgrind complains about conditional jumps depending on uninitialized values for the last 3 printf statements. That means the 3 grades aren't initialized.

Though it took some digging to find the problem. It's in:

student *student_create(int x){
    student *new_student = malloc(sizeof(student));
    new_student->grade = malloc(sizeof(int));
    new_student->grade = &x;
    return new_student;
}

The new_student->grade = &x; line is setting grade to be a pointer to the argument x, which will be completely invalid when the function returns! You really want *(int*)new_student->grade = x, which writes x to the newly-allocated memory location.

score 0 · Answer 3 · answered Sep 08 '21 at 08:40

Well this doesn't make much sense:

new_student->grade = malloc(sizeof(int));
new_student->grade = &x;

Memory is allocated and assigned to new_student->grade, then that is promptly overwritten by a pointer to a parameter. So you have now a memory leak (you can never again free the allocated memory) and undefined behaviour as that pointer to a local variable is no longer valid after the function returns.

What you want to do is allocate an integer, assign it a value, and then assign the pointer to it to new_student->grade.

int *grade = malloc(sizeof *grade);
if (!grade) {
   // handle the error properly (not shown here)
   return NULL;
}
*grade = x;
new_student->grade = grade;

Change the value of pointed to by a void pointer

3 Answers3