Przydzielanie pamięci na ciąg wewnątrz struktury

Question

Wiem, że to pytanie było już w pobliżu, ale znalazłem odpowiedzi trochę mgliste, długie lub/i mylące; więc zamierzam konkretnie odnieść się do mojego kodu, aby w pełni zrozumieć punkt.

więc mam tej struktury:

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

następujące funkcje:

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; 
} 

i główną funkcję:

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]); 
} 

winy segmentacji przy wydawaniu strncpy aby dodać piosenkę w "add_song()".

Co robię krytycznie źle? jak słyszałem kilka razy, nie ma "poprawnej" metody implementacji wc, o ile działa i nie ma błędów, jest OK, ale jako początkujący świetnie byłoby uzyskać ostrzeżenie zwrotne lub porady dotyczące korzystania z alokacji pamięci wraz ze złożonymi strukturami danych.

Wielkie dzięki! /s

Answer 1

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

To nie jest dobry pomysł. Musisz utwór x przez a->songs[x], więc musisz przydzielić a->songs jako (char**)malloc(sizeof(char*)*numsongs). Kiedy jest tylko jedna piosenka, powinieneś umieścić ją w podstrumie.

Jednym z powodów jesteś segfaulting dlatego powyższe nie mają +1 dla NUL jak masz wszędzie indziej ... Innym jest to, że nie dodać +1 długości strncpy, więc nic rzeczywiście dostaje zakończony.

Answer 2

Problemem jest strncpy() nie null zakończyć ciąg dla Ciebie:

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

od poinformować go bufor ma tylko miejsca dla samego łańcucha. Rozwiązaniem tutaj jest po prostu użycie strcpy() - wiesz na pewno, że bufor jest wystarczająco duży.

Również w tym:

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

uwolni tylko struktury, ale nie wskazał na smyczki z tych struktur i masz przeciek pamięci.

Answer 3

Moim zdaniem to, co robisz źle nie jest krytycznie użyciu odpowiednich narzędzi :-)

Jeden problem jest następujący wiersz:

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

przeznaczyć pewną ilość bajtów równa długości pierwszą piosenkę, ale chcesz mieć tablicę wskaźników. Może to działać głupie szczęście, pierwszy tytuł utworu ma więcej znaków niż liczba bajtów wymaganych dla liczby użytych znaków char.

Ale byłoby lepiej zrobić

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

lub nawet rozszerzyć wachlarz piosenek „” w razie potrzeby dynamicznie i realloc.

Nawiasem mówiąc, nigdy nie należy wprowadzać do żadnej wiadomości numeru songs_c, co oznacza, że ​​w ogóle nie można przydzielić adresu songs.

Ponadto, można przydzielić albums z

albums = malloc(sizeof(struct album)); 

Ponownie, to może pracować przez głupie szczęście od wielkości dwóch wskaźników może być mniejsza niż wielkość struct album, ale myślę, że naprawdę rozumie

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

Wszystkie te problemy powinny zostać przechwycone przez analizę statycznego kodu lub narzędzia do analizy środowiska wykonawczego.

Answer 4

W funkcji initalbum funkcja song_c zmienna dla albumu [1] nie została zainicjowana. Będzie to miało wartość śmieci.

W funkcji add_song, ponieważ indeks nie jest zainicjalizowany, powoduje to sEGV.

Answer 5

poważnie rozważyć zastąpienie tego:

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

z tym:

a->artist = my_strdup(artist); 

Gdzie:

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; 
} 

ja sądzę, że to oczywiste, że ten ostatni jest wyraźniejszy. Jest to również lepsze pod względem funkcjonalności, ponieważ strncpy() ma okropną semantykę i naprawdę należy go unikać, moim zdaniem. Ponadto moje rozwiązanie jest prawdopodobnie szybsze. Jeśli Twój system ma numer strdup(), możesz go użyć bezpośrednio, ale nie jest on w 100% przenośny, ponieważ nie jest dobrze zestandaryzowany. Oczywiście powinieneś zastąpić wszystkie miejsca, w których musisz skopiować ciąg znaków do dynamicznie przydzielonej pamięci.