Thread: Problem with free()

Hello!

This is the problem: when I try to delete all the elements of a linked list the program returns an error while it doesn't when I do the same with only one element of the list.

Here's the code:

Code:

typedef struct my_articolo
{
    signed int code;
    char *name;
    char *des;
    float price;
    signed int q_disp;
    signed int q_min;
    struct my_articolo *n;
} articolo;

articolo *start = 0;



void add()
{
    articolo *p = 0, *c = 0, *nn = calloc(1, sizeof(articolo));
    char *n_name = (char*) calloc(NAME_LENGTH, sizeof(char)), *n_des = (char*) calloc(DES_LENGTH, sizeof(char));

    if(!nn)
    {
        fputs("\nErrore di memoria. Impossibile aggiungere l'articolo.\n", stderr);
        free(nn);
        return;
    }

i_code:
    fputs("\nInserisci il codice del componente (0 - 99999999): ", stderr);
    fflush(stdin);
    scanf("%8i", &nn->code);

    if(nn->code < 0 || nn->code > 99999999)
    {
        fputs("\nIl codice inserito non e' valido.\n", stderr);
        goto i_code;
    }

    for(c = start, p = 0; c && nn->code > c->code; p = c, c = c->n);

    if(c && (nn->code == c->code))
    {
        fputs("\nL'articolo esiste gia'. Impossibile aggiungerlo.\n", stderr);
        free(nn);
        return;
    }

i_name:
    fputs("Inserisci il nome dell'articolo: ", stderr);
    r_line(n_name, NAME_LENGTH, stdin);

    if(strlen(n_name) <= 0)
    {
        fputs("\nIl nome inserito non e' valido.\n", stderr);
        goto i_name;
    }

    nn->name = n_name;


i_des:
    fputs("Inserisci la descrizione dell'articolo: ", stderr);
    r_line(n_des, DES_LENGTH, stdin);

    if(strlen(n_des) <= 0)
    {
        fputs("\nLa descrizione inserita non e' valida.\n", stderr);
        goto i_des;
    }

    nn->des = n_des;


i_price:
    fputs("Inserisci il prezzo dell'articolo(0 - 9999999.99): ", stderr);
    fflush(stdin);
    scanf("%7f", &nn->price);

    if(nn->price < 0.0 || nn->price > 9999999.99)
    {
        fputs("\nIl prezzo inserito non e' valido.\n", stderr);
        goto i_price;
    }


i_disp:
    fputs("Inserisci la disponibilita' dell'articolo(0 - 9999999): ", stderr);
    fflush(stdin);
    scanf("%7i", &nn->q_disp);

    if(nn->q_disp < 0 || nn->q_disp > 9999999)
    {
        fputs("\nIl valore inserito non e' valido.\n", stderr);
        goto i_disp;
    }


i_min:
    fputs("Inserisci la quantita' minima di riordino(0 - 9999998): ", stderr);
    fflush(stdin);
    scanf("%7i", &nn->q_min);

    if(nn->q_min < 0 || nn->q_min > 9999998)
    {
        fputs("\nIl valore inserito non e' valido.\n", stderr);
        goto i_min;
    }

    nn->n = c;

    if(!p)
    {
        start = nn;
    }
    else
    {
        p->n = nn;
    }
}



void delete()
{
    articolo *p = 0, *c = 0;
    signed int code = 0, c_code = 0;

    if(!start)
    {
        fputs("\nDatabase vuoto.\n", stderr);
        return;
    }

menu_e:
    fputs("\n1 - Elimina uno o piu' componenti\n", stderr);
    fputs("\n2 - Elimina tutto il database\n", stderr);
    fputs("\n3 - Torna al menu'\n", stderr);
    fputs("\nInserisci il codice corrispondente alla tua scelta: ", stderr);

    fflush(stdin);
    scanf("%u", &c_code);

    switch(c_code)
    {
    case 1:
del:
        fputs("\nInserisci il codice del componente: ", stderr);

        fflush(stdin);
        scanf("%u", &code);

        for(c = start, p = 0; c && (c->code != code); p = c, c = c->n);

        if(!c)
        {
            fputs("\nComponente non trovato. Impossibile completare la cancellazione.\n", stderr);
            fputs("\nRiprovare?[s/n]: ", stderr);
            fflush(stdin);
            if(fgetc(stdin) == 's')
            {
                PUTS_LINE_STDERR;
                goto del;
            }
            goto ask_e;
        }
        if(!p)
        {
            start = c->n;
        }
        else
        {
            p->n = c->n;
        }

        free(c->name);
        free(c->des);
        free(c);

        fputs("\nComponente eliminato.\n", stderr);
        break;

    case 2:
        for(c = start, p = 0; c; p = c, c = c->n);
        {
            free(c->name);
            free(c->des);
            free(c);
        }

        start = 0;

        fputs("\nDatabase svuotato.\n", stderr);
        return;

    case 3:
        return;

    default:
        fputs("\nIl codice inserito non e' valido.\n", stderr);
        PUTS_LINE_STDERR
        goto menu_e;
        break;
    }

ask_e:
    fputs("\nContinuare con le eliminazioni?[s/n]: ", stderr);
    fflush(stdin);
    if(fgetc(stdin) == 's')
    {
        PUTS_LINE_STDERR;
        goto menu_e;
    }
}

In delete() the case 1 works but the case 2 returns an error. Any suggestion?

Does r_line acquire memory via malloc? Otherwise, that memory is not yours to free.

Also, once you have done free(c), there's no way you can do c = c->n (since c->n doesn't exist any longer). You will have to store that somewhere before you free c.

Code:

char *n_name = (char*) calloc(NAME_LENGTH, sizeof(char)), *n_des = (char*) calloc(DES_LENGTH, sizeof(char));

Your second correction is true but the for fails at the first cicle.

Originally Posted by drew99

Code:

char *n_name = (char*) calloc(NAME_LENGTH, sizeof(char)), *n_des = (char*) calloc(DES_LENGTH, sizeof(char));

Your second correction is true but the for fails at the first cicle.

If you trust r_line to not overwrite that, then great. (EDIT: And looking at the code again, I suspect it can't, which is good.) I would suggest then running your program in a debugger and/or using valgrind to pinpoint what exactly is going wrong and why.

Thanks to all, but what are the alternatives for goto and how can I resolve the problem with free()?

Ok:

Code:

for(p = start, c = 0; p; p = c);
        {
            c = p->n;
            free(p->name);
            free(p->des);
            free(p);
        }

Originally Posted by oogabooga

Ouch! Actual, real-life spaghetti code! Rare to see these days.
What possessed you to use gotos all over the place?

At this point, the best remedy is to print the code out, delete the file, then burn the printout while repeating "I will not use gotos, I will not use gotos, ...".

Then rewrite the code from scratch.

Can you post a solution without gotos, thanks?

Can you tell me what are the advantages of using cycles instead of goto?
Looking at the assembly produced by isolating a small portion with a menu, there seems to be no substantial differences, even if I'm not very expert in ASM.