Allocating memory for a string inside a struct?

This is not a good idea. You must song x via a->songsx 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.

Up vote 1 down vote favorite share g+ share fb share tw.

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->songsindex = malloc( strlen(song)+1 ); strncpy(a->songsindex, song, strlen(song)); a->songs_c= a->songs_c+1; } And a main function: int main(void){ char *name; char artist20 = "The doors"; char album20 = "People are strange"; int year = 1979; struct album *a; struct album **albums; albums = malloc( sizeof(struct album)); albums0 = init_album((char *)"hihi", (char *)"hoho", 1988); albums1 = init_album((char *)"hihi1", (char *)"hoho1", 1911); printf("%s, %s, %d\n", albums0->artist, albums0->title, albums0->year); printf("%s, %s, %d\n", albums1->artist, albums1->title, albums1->year); char song = "song 1\0"; add_song(albums1, song); free(albums0); free(albums1); } 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 c malloc struct link|improve this question asked Feb 24 '11 at 13:50Smokie565 62% accept rate.

Oh, boy. C programmers are still spending 90% of the time writing memory management code. This is sad.

– Vlad Lazarenko Feb 24 '11 at 13:59 1 char song = "song 1\0"; This adds two null termination characters. As soon as you use "", the compiler will add an invisible null termination for you, you don't have to do it manually. "x" is the same as {'x', '\0'}.

– Lundin Feb 24 '11 at 14:17 @Vlad It's a tough choise. Either write in C/C++ and spend 90% of the programmer's time for memory management, or pick another language and spend 90% of the program execution time for the same purpose. =) – Lundin Feb 24 '11 at 14:21 @Lundin: Not exactly true.

I'd say that if you need to allocate/free memory on a critical path - it is a bad design, and is a problem no matter what you use C or C++, otherwise using std::string from C++ won't harm your performance, especially if you have an object pool (or even lock-free object pool). – Vlad Lazarenko Feb 24 '11 at 14:51.

If (index == 0) { a->songs = malloc( strlen(song) ); } else a->songsindex = malloc( strlen(song)+1 ); This is not a good idea. You must song x via a->songsx, 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.

– Smokie Feb 24 '11 at 14:20 still got a segmentation fault... – Smokie Feb 24 '11 at 14:31.

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(albums0); free(albums1); will only free the structures, but not strings pointed to from those structures and you've got a memory leak.

– Smokie Feb 24 '11 at 14:07 @Smokie: It will, but this doesn't make any sense - you'll have an extra scan along the string and overengineered code. Strcpy() is what you mean here, so just use it. – sharptooth Feb 24 '11 at 14:09.

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 album1 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.

I cant really gove you an answer,but what I can give you is a way to a solution, that is you have to find the anglde that you relate to or peaks your interest. A good paper is one that people get drawn into because it reaches them ln some way.As for me WW11 to me, I think of the holocaust and the effect it had on the survivors, their families and those who stood by and did nothing until it was too late.