How to reference a pointer to structure inside another structure in C

Question

I have an assignment in C that basically asks for some sort of interface/database for a supposed animal shelter, and we were given these 2 structures:

typedef struct age
{
int years, months;
}age;

typedef struct pet
{
int id;
char* sex;
char* breed;
age* pet_age;
}pet;

The interface has to have several functions, like adding a new pet (in our case dogs specifically), removing based on ID, searching for all pets of the same breed and changing the name of a breed entirely, and it all has to be done dynamically using a pet* array as well as the malloc and realloc functions. The entries have to be written in a file and also read from it, but that's something I'll figure out after I figure out how to handle the functions regarding my dynamic array first.

To get to the point, I am having trouble understanding how to scan/reference an instance's pet_age. I've tried it a myriad different ways but I don't understand what's wrong, really. The program crashes/exits after I scan the months element.

Here is the insertion function I have implemented thus far. While not correct, the main source file still compiles.

void addPet(pet *p){
    if(i=1){            //First time activation check.
    p=malloc(k*sizeof(p));
    if(!p){
        printf("\nUnable to allocate memory...");
        exit(0);
    }
    }
    p[i].sex = malloc(sizeof(char)*1);
    p[i].breed = malloc(sizeof(char)*20);
    p[i].pet_age =malloc(sizeof(int)*2);
    
    p[i].id = i;    //Autogenerated ID
    printf("\n%d\n", p[i].id);
    
    printf("Insert pet's breed:");  //Scan breed
    scanf("%s", p[i].breed);
    
    printf("Insert pet's sex:");    //Scan sex
    scanf("%s", p[i].sex);
    
    printf("Insert pet's age in years:");   //Scan years
    scanf("%d", p[i].pet_age->years);
    printf("\n%d\n", p[i].pet_age->years);
    
    printf("Insert pet's age in months:");  //Scan months
    scanf("%d", p[i].pet_age->months);
    printf("\n%d\n", p[i].pet_age->months);
    
    i++;    //Incrementing counter
    
    if(i==k){
    k+=10;
    p=realloc(p, k*sizeof *p);  //Size check
    }
}

For now there is a basic initialization in the event that this is the first insertion. Then I allocate memory for each element of the structure (to the best of my understanding), and scan every element with a scanf (I pasted some printf checks to see what was actually scanned). Then at the end I increment the i counter, followed by a size check to allocate 10 more places for the array in the event that i==k.

For the sake of continuity, here is my main function as well (basic menu and all):

int i=1; //Counter
int k=10;   //Default max entries

int main(int argc, char *argv[]) {
    FILE *fp;
    int choice;
    pet *petarray;
    
    //Menu that lists every option.
    while(1){   //Endless loop that ends only if you choose to exit through the 5th option.
    printf("\n\n Menu:");
        printf("\n=========");
        printf("\n1. Insert information for a new pet.");
        printf("\n2. Delete a pet record based on pet's ID.");
        printf("\n3. Search a pet record based on pet's breed.");
        printf("\n4. Update pet's breed name.");
        printf("\n5. Exit.\n\n");
        scanf("%d", &choice);
    switch(choice)
    {
        case 1:
            addPet(petarray);
            break;
        case 2:
            break;
        case 3:
            break;
        case 4:
            break;
        case 5:
            printf("Exiting program...");
            exit(0);
        }   
}
    return 0;
}

Apologies if this seems amateur, I'm quite the rookie and still learning. Thanks in advance.

Answer 1

It compiles, but don't you get a long list of warnings? If you don't, you should turn on warnings.

But let's have a look.

void addPet(pet *p)
{
    if (i = 1)

You are not comparing (global!) i to 1 here. You are assigning to it. This if statement can only take the true path because of that. When you assign to i, the result of the assignment is the value you assign, so you are testing if (1) here. You want to take this path only if i is 1, I guess, so you should use if (i == 1).

    { //First time activation check.
        p = malloc(k * sizeof(p));

Well, it is every time, but we have fixed that now. What do you want p to be, here? An array of k pets? That is not what you are allocating memory for. You are allocating space for k times sizeof(p) and since p is a pet *p, that means you are allocating space for k pointers to pets. Not pets. That, of course, is a problem since p is a pointer to pet and not a pet **. You have most likely allocated too little memory here.

This, unfortunately, is usually not something you will get a warning about. You can give malloc() any size, and it will give you that amount of memory. If you asked for the wrong amount, you get the wrong amount. I think you wanted malloc(k * sizeof *p) here. That allocates space for k of the kind of objects p points to, and that means you can use p as an array of k of that type. You do it the right way when you realloc() later, so this is probably just a quick mistake, but it can easily destroy everything at runtime.

    p[i].sex = malloc(sizeof(char) * 1);
    p[i].breed = malloc(sizeof(char) * 20);

Two issues here. First, are you sure that p has an entry i? If you fixed the allocation above, then the p you allocated the first time has room for k pets, but this could be any p we have called the function with, so we don't know about this one at all. There is absolutely no guarantee that it is valid to access p[i]. Your reliance on the two global variables will generally make this very dodgy; you simply cannot assume that the function is called with the specific pointer you allocated memory for a bit earlier.

Second, for the string allocation, there are a few red flags as well. sizeof(char) is always 1, so you don't need it. It isn't wrong, really, it just looks odd. And are you absolutely sure that you are allocating enough memory? For p[i].sex I find it highly unlikely. You are getting space for exactly one char. If you only want one char, then that is fine, but you you should probably declare sex a char instead of a char *. If you plan to put a string in p[i].sex, then it will have to be the empty string and nothing longer, because you have only room for the '\0' terminal in a buffer of length 1.

With

   p[i].pet_age = malloc(sizeof(int) * 2);

it might technically work, but I don't think the standard guarantees it. You are allocating space for a struct age, and that struct holds two int. They will align the right way, so there shouldn't be any padding, and therefore it should work, but it is flaky as hell.

If you want to allocate space for a struct, then do that. malloc(sizeof(struct age)) gets the job done. Even better, gets the type from the variable you are allocating space for:

    p[i].pet_age = malloc(sizeof *(p[i].pet_age));

If p[i].pet_age is a struct age *, then *(p[i].pet_age) is a struct age, and it is the size of that we want.

Then we read in the data.

    printf("Insert pet's breed:"); //Scan breed
    scanf("%s", p[i].breed);

Here we can have a buffer overflow.

    printf("Insert pet's sex:"); //Scan sex
    scanf("%s", p[i].sex);

Here we are guaranteed one, because we need to write the terminal zero into sex after we put the data there.

    printf("Insert pet's age in years:"); //Scan years
    scanf("%d", p[i].pet_age->years);
    printf("\n%d\n", p[i].pet_age->years);

    printf("Insert pet's age in months:"); //Scan months
    scanf("%d", p[i].pet_age->months);
    printf("\n%d\n", p[i].pet_age->months);

Since scanf needs to store the data it reads somewhere, it needs a pointer to where it should put it. You are providing integers. (Your compiler definitely should have warned you here). You should use &p[i].pet_age->years to store an integer in p[i].pet_age->years, and the same for months.

Then we get to what I think is probably the worst error in the code.

    if (i == k)
    {
        k += 10;
        p = realloc(p, k * sizeof *p); //Size check
    }

I'm not going to comment on the global variables again, but rather the local variable. This realloc potentially destroys the memory that p pointed at. I don't care that it can return NULL and you don't check; I doubt that this is happening in your program, but someone called addPet with a pointer, and they have no way of knowing if that pointer is valid again after calling. They have to consider it lost. It won't be freed if addPet() doesn't free it (and it doesn't), and they cannot safely do it themselves. The new memory you allocate doesn't get back to the caller in any way. Assigning to the local variable in addPet() doesn't affect any caller's variable. This realloc() is dangerous. The caller will absolutely lose the existing memory and has no way of obtaining the new memory.

Any of these issues can be the cause of your current problem; the others can be the cause of future problems.