Memory allocation for char pointer in a struct-CodePudding

I'm trying to teach C to myself and I'm struggling with what it looks like a very basic thing. I have a struct with a char pointer and I want to dynamically allocate memory and free it once done. What I understand is that I need to allocate memory for both the struct and the char pointer but it looks like I'm missing something because I'm getting a Segmentation fault on the sprintf line.

As an exercise to understand and use malloc and free, I'm trying to build an easy program that takes an int as input and outputs a table with the factorials of all the numbers from 0 to input. Each table entry has its index (int), the result of the factorial (long long int) and the result but in an array of chars format (this is the problematic one).

Here's what I have until now:

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

#define LIMIT 20

struct entry
{                               /* Definition of each table entry */
  int n;
  long long int lli;
  char *str;
};

void shout(char *m)
{
  fprintf (stderr, "%s\n", m);
  exit (0);
}

int main (int argc, char *argv[])
{
  int n;
  int i;
  struct entry *fact_table;

  if (argc != 2)
    shout("wrong parameters");

  n = atoi (argv[1]);
  if (n < 0)
    shout("n too small");
  if (n > LIMIT)
    shout("n too big");

  // Allocate memory for struct
  fact_table = (struct entry*) malloc((n 1) * sizeof(struct entry));
  
  if (fact_table == NULL) {
    shout("malloc");
  }
  // Compute factorials
  fact_table[0].n = 0;
  fact_table[0].lli = fact_table[1].n = fact_table[1].lli = 1;  // 0! = 1! = 1
  fact_table[0].str = fact_table[1].str = "1";
  
  for (i=2; i<=n; i  ) {
    fact_table[i].n = i;
    fact_table[i].lli = i * fact_table[i-1].lli; // N! = N*(N-1)!
    int digits = log10(fact_table[i].lli)   1; // get number of digits of the result to allocate memory in consequence
    fact_table[i].str = malloc((digits   1)*sizeof(char));
    if (fact_table[i].str = NULL) {
        shout("malloc");
    }
    sprintf(fact_table[i].str, "%lld", fact_table[i].lli); // convert to string. ERROR HERE
  }

  // print factorial table
  for (i= 0; i<=n; i  )
  {
      printf ("%d %lld %s\n", fact_table[i].n, fact_table[i].lli, fact_table[i].str);
  }

  // Free memory
  for (i=0; i<=n; i  ) {
    free(fact_table[i].str);
  }
  free(fact_table);

  return 0;
}

I'm probably missing something very obvious and making a silly mistake when allocating the memory for the fact_table[i].str, but I'm struggling to make it work.

CodePudding user response：

In addition to the comments about freeing the memory for n >= 2, if you look at your test after the malloc:

    if (fact_table[i].str = NULL) {

You are setting your pointer to NULL. Instead you should write:

    fact_table[i].str = malloc((digits   1)*sizeof(char));
    if ( fact_table[i].str == NULL) {
        shout("malloc");
    }

Just some little recommendation here. This mistake generates a warning on most compiler if you compile using -Wall:

   fact.c: In function ‘main’:
fact.c:53:9: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if (fact_table[i].str = NULL) {
         ^~~~~~~~~~

Also, in order to make sure the compiler will complain if you forget a =, you can compare the values of a variable and of a constant placing the constant on the left: if you write if (NULL = fact_table[i].str), your compiler will complain and notify you with the error.

CodePudding user response：

I reproduced this issue on my setup. We need to fix two issues before program runs ok.

change 'fact_table[i].str = NULL' to 'fact_table[i].str == NULL'. Your previous command directly set 'str' to NULL, which is not expected. This is the reason you met segment error.
In Free memory part, you need to start from index 2. You cannot free memory which doesnot belong to you.

Below code can work well in my setup. Wish this can help you.

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

#define LIMIT 20

struct entry
{                               /* Definition of each table entry */
  int n;
  long long int lli;
  char *str;
};

void shout(char *m)
{
  fprintf (stderr, "%s\n", m);
  exit (0);
}

int main (int argc, char *argv[])
{
  int n;
  int i;
  struct entry *fact_table;

  if (argc != 2)
    shout("wrong parameters");

  n = atoi (argv[1]);
  if (n < 0)
    shout("n too small");
  if (n > LIMIT)
    shout("n too big");

  // Allocate memory for struct
  fact_table = (struct entry*) malloc((n 1) * sizeof(struct entry));

  if (fact_table == NULL) {
    shout("malloc");
  }
  // Compute factorials
  fact_table[0].n = 0;
  fact_table[0].lli = fact_table[1].n = fact_table[1].lli = 1;  // 0! = 1! = 1
  fact_table[0].str = fact_table[1].str = "1";

  for (i=2; i<=n; i  ) {
    fact_table[i].n = i;
    fact_table[i].lli = i * fact_table[i-1].lli; // N! = N*(N-1)!
    int digits = log10(fact_table[i].lli)   1; // get number of digits of the result to allocate memory in consequence
    fact_table[i].str = malloc((digits   1)*sizeof(char));
    if (fact_table[i].str == NULL) {
        shout("malloc");
    }
    sprintf(fact_table[i].str, "%lld", fact_table[i].lli); // convert to string. ERROR HERE
  }

  // print factorial table
  for (i= 0; i<=n; i  )
  {
      printf ("%d %lld %s\n", fact_table[i].n, fact_table[i].lli, fact_table[i].str);
  }

  // Free memory
  for (i=2; i<=n; i  ) {
    free(fact_table[i].str);
  }
  free(fact_table);

  return 0;
}