Asignación de memoria para una cadena dentro de una estructura

Question

Sé que esta pregunta ha existido, pero las respuestas me parecieron un poco confusas, largas y/y confusas; así que voy a referirme específicamente a mi código para obtener el punto por completo.

Así que me dio esta estructura:

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

las siguientes funciones:

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

y una función principal:

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

Fallo de segmentación al emitir strncpy añadir una canción "add_song()".

¿Qué estoy haciendo muy mal? como se ha escuchado un montón de veces, no hay una forma "correcta" de implementación en c, siempre y cuando funcione y no tenga errores, está bien, pero como principiante sería genial recibir comentarios o consejos sobre el uso de la asignación de memoria junto con estructuras de datos complejas.

¡Muchas gracias! /s

Answer 1

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

Esto no es una buena idea. Debe canción x a través de a->songs[x], por lo que necesita asignar a->songs como (char**)malloc(sizeof(char*)*numsongs). Cuando solo hay una canción, debes ponerla en el sub-puntero.

Una de las razones que está en violación de segmento se debe a que lo anterior no tiene un +1 para la NUL como que tiene en todas partes ... Otra es que no agregó la +1 a la longitud strncpy, para que nada se hecho terminado.

Answer 2

El problema es strncpy() voluntad no nulo interrumpir la cadena para usted:

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

ya que decir que la memoria intermedia sólo se dispone de espacio para la propia cadena. La solución aquí es simplemente usar strcpy() - usted sabe con certeza que el buffer es lo suficientemente grande.

también esto:

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

sólo se liberará las estructuras, pero no a partir de cadenas señalado esas estructuras y usted tiene una pérdida de memoria.

Answer 3

En mi opinión, lo que se hace críticamente incorrecto no está usando herramientas adecuadas :-)

Uno de los problemas es la siguiente línea:

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

Se asigna una cantidad de bytes igual a la longitud de la primera canción, pero quieres una serie de punteros de char. Esto podría funcionar por pura suerte, el primer título de la canción tiene más caracteres que la cantidad de bytes requeridos para la cantidad de punteros utilizados.

Pero sería mejor que hacer

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

o incluso ampliar la gama 'canciones' de forma dinámica y realloc cuando sea necesario.

Por cierto, nunca intializar songs_c a nada, lo que significa que es posible que no haya asignado songs en absoluto.

Además, se puede asignar albums con

albums = malloc(sizeof(struct album)); 

Una vez más, esto podría funcionar por pura suerte ya que el tamaño de los dos punteros podría ser menor que el tamaño de struct album, pero creo que en realidad quería decir

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

Todos estos problemas deben detectarse mediante análisis de código estático o herramientas de análisis de tiempo de ejecución.

Answer 4

En la función de álbum interno la variable songs_c para el álbum [1] no se inicializa. Esto tendrá un valor de basura.

En la función add_song porque el índice no está inicializado, está causando sEGV.

Answer 5

seriamente la posibilidad de reemplazar esto:

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

con esto:

a->artist = my_strdup(artist); 

Dónde:

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

yo creo que es obvio que este último es más clara. También es mejor para la funcionalidad, ya que strncpy() tiene una semántica horrible y realmente debería evitarse, en mi opinión. Además, mi solución es bastante más rápida. Si su sistema tiene strdup(), puede usarlo directamente, pero no es 100% portátil ya que no está bien estandarizado. Por supuesto, debe hacer el reemplazo de todos los lugares donde necesita copiar una cadena en la memoria asignada dinámicamente.