Allocating memory for a string inside a struct

Question

I know this question has been around, but I found answers a bit foggy, long or/and confusing; so I am going to specifically refer to my code in order to fully get the point.

so i got this struct:

typedef struct album {
  unsigned int year;
  char *artist;
  char *title;
  char **songs;
  int songs_c;
} album ;

the following functions :

struct album* init_album(char *artist, char *album, unsigned int year){
  struct album *a;
  a= malloc( sizeof(struct album) );
  a->artist = malloc( strlen(artist) + 1);
  strncpy(a->artist, artist, strlen(artist));
  a->title = malloc( strlen(album) +  1);
  strncpy(a->title, album, strlen(album));
  a->year = year;
  return a;
}

void add_song(struct album *a, char *song){
  int index = a->songs_c;
  if (index == 0){
    a->songs = malloc( strlen(song) );
  } else a->songs[index] = malloc( strlen(song)+1 );

  strncpy(a->songs[index], song, strlen(song));
  a->songs_c= a->songs_c+1;
}

And a main function:

int main(void){
  char *name;
  char artist[20] = "The doors";
  char album[20] = "People are strange";
  int year = 1979;

  struct album *a;
  struct album **albums;

  albums = malloc( sizeof(struct album));

  albums[0] = init_album((char *)"hihi", (char *)"hoho", 1988);

  albums[1] = init_album((char *)"hihi1", (char *)"hoho1", 1911);

  printf("%s, %s, %d\n", albums[0]->artist, albums[0]->title, albums[0]->year);
  printf("%s, %s, %d\n", albums[1]->artist, albums[1]->title, albums[1]->year);

  char song[] = "song 1\0";

  add_song(albums[1], song);

  free(albums[0]);
  free(albums[1]);
}

Segmentation fault when issuing strncpy to add a song in "add_song()".

What am i doing critically wrong? as heard a bunch of times, there is no "correct" way of implementation in c, as long as it works and it's not buggy, it's ok, but as a beginner would be great to get some caution feedback or advices about using memory allocation along with complex data structures.

Thanks a bunch! /s

Answer 1

if (index == 0) {
    a->songs = malloc( strlen(song) );
} else a->songs[index] = malloc( strlen(song)+1 );

This is not a good idea. You must song x via a->songs[x], so you need to allocate a->songs as (char**)malloc(sizeof(char*)*numsongs). When there is just one song, you should still put it into the sub-pointer.

One reason you're segfaulting is because the above doesn't have a +1 for the NUL like you have everywhere else... Another is that you didn't add the +1 to the strncpy length, so nothing actually gets terminated.