Dynamically allocated memory in structure in c

Question

I'm trying to create two lists, pros and cons and then print them. But I can't figure out what am I doing wrong.

I tried to debug the program with gdb online and I found out that the error is in function fgets().

#include <stdio.h>
#include <string.h>
typedef struct list{
    char ** reason;

} list;
void printMenu();
void printList(list * myList, int len1);


int main(void)
{
    int keepGoing = 0;
    int choice = 0;
    int i = 0;
    int j = 0;
    list * pros;
    list * cons;

    while (!keepGoing){

        printMenu(); 
        scanf("%d", &choice);

        pros = (list*)malloc(sizeof(list));
        cons = (list*)malloc(sizeof(list));
        switch (choice){
        case 1:
            i++;
            printf("Enter a reason to add to list PRO: ");
            pros = (list*)realloc(pros, i*sizeof(list));
            fgets(pros->reason[i], 50, stdin);
            pros->reason[strcspn(pros->reason[i], "\n")] = 0;
            break;
        case 2:
            j++;
            cons = (list*)realloc(cons->reason, j*sizeof(list));
            printf("Enter a reason to add to list CON: ");
            fgets(cons->reason[j], 50, stdin);
            cons->reason[strcspn(cons->reason[j], "\n")] = 0;
            break;
        case 3:
            printf("PROS:\n");
            printList(pros, i);
            printf("CONS:\n");
            printList(cons, j);

            break;
        case 4:
            keepGoing = 1;
            break;
        default:
            printf("Invalid value.");
            keepGoing = 1;
        }
    }

    free(pros);
    free(cons);

    getchar();
    return 0;
}

void printList(list * reasons, int len1){
    int i = 0;
    for (i = 0; i < len1; i++){
        printf("%s\n", reasons->reason[i]);
    }
}
void printMenu(){
    printf("Choose option:\n");
    printf("1 - Add PRO reason\n");
    printf("2 - Add CON reason\n");
    printf("3 - Print reasons\n");
    printf("4 - Exit\n");
}

Answer 1

There is no need to allocate these dynamically: list * pros; list * cons;. Code like pros = (list*)realloc(pros, i*sizeof(list)); doesn't make any sense.

Instead, declare them as plain variables. list pros.

What you instead need to allocate dynamically is the member pros.reason. You need to allocate an array of pointers that it points to, and then you need to allocate the individual arrays.

Answer 2

You have a problem in

  fgets(pros->reason[i], 50, stdin);

as the memory you want to use is not valid. pros->reason does not point to a valid memory, so you cannot dereference it, this causes undefined behavior.

Before you can index-into pros->reason, you need to make pros->reason point to a valid memory location.

After that, you need to make pros->reason[i]s also to point to valid memory if you want them to be used as the destination of fgets().

---

Apart from this issue, you have another issue which makes this code nonsense, that is calling malloc() on every iteration of the loop. You need to call malloc() only once, to get a pointer (to memory) allocated by memory allocator function and then, use realloc() inside the loop to adjust that to the required memory.

Answer 3

There are many issues. Previous comments and answers do still apply.

Here is a clean solution.

• The list structure is now self contained, no need to track the number of strings in separate variables
• added selfcontained AddString function
• no more unnecessary mallocs
• all allocated memory is freed correctly
• some logic errors removed (inverted logic of keepGoing)

There is still room for improvement. Especially there is no error checking for the memory allocation functions.

---

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

typedef struct list {
  int size;       // number of strings
  int chunksize;  // current of chunk
  char ** reason;
} list;

void printMenu();
void printList(list * reasons);
void freeList(list * l);
void AddString(list *l, const char *string);

int main(void)
{
  int keepGoing = 1;
  int choice = 0;

  list pros = { 0 };  // = {0} initializes all fields to 0
  list cons = { 0 };

  while (keepGoing) {

    printMenu();
    scanf("%d", &choice);

    char input[50];
    fgets(input, sizeof(input), stdin);  // absorb \n from scanf

    switch (choice) {
    case 1:
      printf("Enter a reason to add to list PRO: ");
      fgets(input, sizeof(input), stdin);
      AddString(&pros, input);    // Add string to pros
      break;
    case 2:
      printf("Enter a reason to add to list CONS: ");
      fgets(input, sizeof(input), stdin);
      AddString(&cons, input);    // Add string to cons
      break;
    case 3:
      printf("PROS:\n");
      printList(&pros);
      printf("CONS:\n");
      printList(&cons);
      break;
    case 4:
      keepGoing = 0;
      break;
    default:
      printf("Invalid value.");
      keepGoing = 1;
    }
  }

  freeList(&pros);
  freeList(&cons);

  getchar();
  return 0;
}


#define CHUNKSIZE 10

void AddString(list *l, const char *string)
{
  if (l->size == l->chunksize)
  {
    // resize the reason pointer every CHUNKSIZE entries
    l->chunksize = (l->chunksize + CHUNKSIZE);

    // Initially l->reason is NULL and it's OK to realloc a NULL pointer
    l->reason = realloc(l->reason, sizeof(char**) * l->chunksize);
  }

  // allocate memory for string (+1 for NUL terminator)
  l->reason[l->size] = malloc(strlen(string) + 1);

  // copy the string to newly allocated memory
  strcpy(l->reason[l->size], string);

  // increase number of strings
  l->size++;
}

void freeList(list * l) {
  for (int i = 0; i < l->size; i++) {
    // free string
    free(l->reason[i]);
  }

  // free the list of pointers
  free(l->reason);
}

void printList(list * l) {
  for (int i = 0; i < l->size; i++) {
    printf("%s\n", l->reason[i]);
  }
}

void printMenu() {
  printf("Choose option:\n");
  printf("1 - Add PRO reason\n");
  printf("2 - Add CON reason\n");
  printf("3 - Print reasons\n");
  printf("4 - Exit\n");
}