Allocating memory for a string inside a struct

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 = mallo开发者_开发知识库c( 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

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.

The problem is strncpy() won't null-terminate the string for you:

a->artist = malloc( strlen(artist) + 1);
strncpy(a->artist, artist, strlen(artist)); // null terminator is not placed

since you tell it the buffer only has space for the string itself. The solution here is to just use strcpy() - you know for sure that the buffer is large enough.

Also this:

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

will only free the structures, but not strings pointed to from those structures and you've got a memory leak.

In my opinion, what you do critically wrong is not using proper tools :-)

One problem is the following line:

a->songs = malloc( strlen(song) );

You allocate an amount of bytes equal to the length of the first song, but you want an array of char pointers. This might work by dumb luck, it the first song title has more characters than the number of bytes required for the number of char pointers used.

But it would be better to do

a->songs = calloc(max_number_of_songs, sizeof(char*));

or even expand the 'songs' array dynamically and realloc when needed.

By the way, you never intialize songs_c to anything, which means you might not have allocated songs at all.

Further, you allocate albums with

albums = malloc( sizeof(struct album));

Again, this might work by dumb luck since the size of two pointers might be less than the size of struct album, but I think you really meant

albums = calloc(2, sizeof(struct album *));

All of these problems should be caught by either static code analysis or runtime analysis tools.

In the initalbum function songs_c variable for album[1] is not initialized. This will have a garbage value.

In the function add_song because index is not initialized, it is causing sEGV.

Seriously consider replacing this:

a->artist = malloc(strlen(artist) + 1);
strncpy(a->artist, artist, strlen(artist));

with this:

a->artist = my_strdup(artist);

Where:

char * my_strdup(const char *s)
{
  char *out = NULL;

  if(s != NULL)
  {
      const size_t len = strlen(s);
      if((out = malloc(len + 1)) != NULL)
         memcpy(out, s, len + 1);
  }
  return out;  
}

I do think it's obvious that the latter is clearer. It's also better functionality-wise, since strncpy() has horrible semantics and really should be avoided, in my opinion. Further, my solution is quite likely faster. If your system has strdup() you could use that directly, but it's not 100% portable since it's not well-standardized. Of course, you should do the replacement for all the places where you need to copy a string into dynamically-allocated memory.