array of structs created with malloc is not deleting nor resizing last element using free and realloc

Question

My code consist in a aplication which receive by console one client (clientes) and add it into an array created with malloc. After we can remove this cliente. Also we can add a new travel (viajes) and remove it (process is analog to showed in code).

I am trying to remove last item of an array of struct created with malloc, as I said earlier. I will show you the code (how I add a new clientes and how i try to delete it). Also say that errors I get are commented in the line which produce the error, so I think it is more visual. Also say the code is MCVE, so only copying code will run. The code is of my aplication is:

#include<stdlib.h>
#include<stdio.h>
#include<string.h>

// DEFINE STRUCTS
struct viaje {
    char *identificador;
    char *ciudadDestino;
    char *hotel;
    int numeroNoches;
    char *tipoTransporte;
    float precioAlojamiento;
    float precioDesplazamiento;
};

struct cliente {
    char *dni;
    char *nombre;
    char *apellidos;
    char *direccion;
    int totalViajes;
    struct viaje *viajes;
};

int main(){
    // Variables
    int i = 0;
    int totalClientes = 0;
    char dni[255];
    char nombre[255];
    char apellidos[255];
    char direccion[255];

    // Init array of struct cliente (in var clientes)
    struct cliente *clientes = (struct cliente *)malloc(0);

    printf("Input number of elements of struct clientes: ");
    scanf("%i", &totalClientes);
    fflush(stdin);

    // ADDING NEW ELEMENTS INTO CLIENTES
    for (i = 0; i < totalClientes; i++) {

        // Receive parameters
        printf("Input NIF: ");
        gets(dni);
        fflush(stdin);
        printf("Input NAME: ");
        gets(nombre);
        fflush(stdin);
        printf("Input SURNAME: ");
        gets(apellidos);
        fflush(stdin);
        printf("Input ADDRESS: ");
        gets(direccion);
        fflush(stdin);

        // Create memory for his child (MAX_TAM_* are #define int)
        clientes = (struct cliente *)realloc(clientes, (i+1)*sizeof(struct cliente));
        clientes[i].dni = (char *)malloc(200*sizeof(char));
        clientes[i].nombre = (char *)malloc(200*sizeof(char));
        clientes[i].apellidos = (char *)malloc(200*sizeof(char));
        clientes[i].direccion = (char *)malloc(200*sizeof(char));
        clientes[i].viajes = (struct viaje *)malloc(0); // Init clientes[i].viajes to 0 as previously done with clientes

        // Adding received element
        strcpy(clientes[i].dni, dni);
        strcpy(clientes[i].nombre, nombre);
        strcpy(clientes[i].apellidos, apellidos);
        strcpy(clientes[i].direccion, direccion);
    /* DANGER - ERROR HERE */
        clientes[i].totalViajes = 0; // HERE I GET A NOT EXPECTED BEHAVIOR - int is not added to clientes[i].totalViajes - Receive NULL
        // New element added sucessfully
    }

    // SHOW ELEMENTS CREATED
    for (i = 0; i < totalClientes; i++) {
        printf("\n");
        printf("%i.\n", i);
        printf("NIF: %s\n", clientes[i].dni);
        printf("NAME: %s\n", clientes[i].nombre);
        printf("SURNAME: %s\n", clientes[i].apellidos);
        printf("ADDRESS: %s\n", clientes[i].direccion);
        printf("TRAVELS_COUNT: %s\n", clientes[i].totalViajes);
       printf("\n");
    }

    // DELETING AN ELEMENT OF CLIENTES
    int posCliente = 0;

    printf("Input position of struct clientes that you wanna delete: ");
    // posCliente is the position (inside the array) of cliente we wanna remove
    scanf("%i", &posCliente);
    fflush(stdin);

    // Rewind one position array clientes since cliente we want to remove
    for (i = posCliente; i < totalClientes-1; i++) {
        // Rewind one element array clientes
        clientes[i] = clientes[i+1];
    }

    //freeing memory of this element (now, the element is the last of the array, we have moved it into last position with previously for)
    free(clientes[totalClientes].dni);
    free(clientes[totalClientes].nombre);
    free(clientes[totalClientes].apellidos);
    free(clientes[totalClientes].direccion);
    clientes[totalClientes].totalViajes = 0;
    // Remove / free memory of the element deleted
/* DANGER - ERROR HERE */
    //free(clientes[totalClientes]); // HERE I GET AN ERROR: `error: incompatible type for argument 1 of 'free'`

    // Now totalClientes is one less (we have deleted one element)
    totalClientes--;

    // Resize array clientes. It must have one less element (now totalClientes is totalClientes-1)
/* DANGER - ERROR HERE */
    //clientes = (struct cliente *)realloc(clientes, (totalClientes)*sizeof(struct cliente)); // HERE I DO NOT GET AN ERROR BUT PROGRAM CRASH

    // SHOW ELEMENTS AFTER DELETING
/* DANGER - ERROR HERE */
    // if the max legnth is totalClientes+1 we can see that the last element removed with free cointinues existing, so free is not freeing memory well
    for (i = 0; i < totalClientes; i++) {
        printf("\n");
        printf("%i.\n", i);
        printf("NIF: %s\n", clientes[i].dni);
        printf("NAME: %s\n", clientes[i].nombre);
        printf("SURNAME: %s\n", clientes[i].apellidos);
        printf("ADDRESS: %s\n", clientes[i].direccion);
        printf("TRAVELS_COUNT: %s\n", clientes[i].totalViajes);
        printf("\n");
    }
}

Don't try to free the last element, just realloc the array down one. — Martin James, Mar 23 '18 at 14:15
@MartinJames but is needed to free the memory earlier delete last element, isn't it? However, when I try to resize array, program crash without error, so I do not Know what is happening — JuMoGar, Mar 23 '18 at 14:16
A global variable `i` is a bad idea, especially in a single-function program. Doubly so when it is used as a loop control variable inside `main()`. — Jonathan Leffler, Mar 23 '18 at 14:17
yeah, I know, it is only one example functional of my code. In my original code `i` is local, no global — JuMoGar, Mar 23 '18 at 14:19
Note the constraints on [Using `fflush(stdin)`](https://stackoverflow.com/questions/2979209/). — Jonathan Leffler, Mar 23 '18 at 14:21
@JonathanLeffler Then, how can I delete buffer in? . What does fflush have to do with the proposed problems? Do you have any relationship? I say it because I do not understand what can influence one thing in the other. Thank you. — JuMoGar, Mar 23 '18 at 14:25
There is no clientes[totalClientes]. The array size only goes from 0 thru totalClientes-1. — Jiminion, Mar 23 '18 at 14:26
@JuMoGar `fflush(stdin);` leads to undefined behavior, thus making it impossible to reason about what your program is doing. Once UB is in, all bets are off. — unwind, Mar 23 '18 at 14:26
You are moving data one position down, this means that positions `totalClientes` and `totalClientes-1` have same malloced pointers that you're freeing... but that you shouldn't... — Frankie_C, Mar 23 '18 at 14:28
Using `fflush` on `stdin` is a common (bad) practice for MS-OS's. See https://msdn.microsoft.com/en-us/library/9yky46tz.aspx. — Frankie_C, Mar 23 '18 at 14:32
@unwind: if they're working on Windows, then `fflush(stdin)` has a meaning defined by Microsoft, not by the C standard. — Jonathan Leffler, Mar 23 '18 at 14:38
Note, too, that [`gets()` is too dangerous to be used — ever](https://stackoverflow.com/questions/1694036/why-is-the-gets-function-dangerous-why-should-it-not-be-used). Again (see next comment), this isn't directly the cause of your trouble; it is an issue of which you should be aware. — Jonathan Leffler, Mar 23 '18 at 14:41
I didn't say "`fflush(stdin)` is the cause of your trouble"; I simply pointed you to where you can find a discussion on the limits on where it can be used. If you're using `gets()`, there won't be any stray data in the input buffer; it reads up to and including the newline. There will be a newline in the input buffer after the first `scanf()` for the number of clients; that needs to be consumed before you start reading lines. There are other ways than using `fflush(stdin)` to do that — see the x-ref. — Jonathan Leffler, Mar 23 '18 at 14:44

score 3 · Accepted Answer · answered Mar 23 '18 at 14:32

Several issues here. First:

printf("TRAVELS_COUNT: %s\n", clientes[i].totalViajes);

The field totalViajes is an integer but you're using %s to print, which expects a char * pointing to a null terminated string. The value 0 is being interpreted as NULL pointer which is why NULL is being printed. Use %d to print integers:

printf("TRAVELS_COUNT: %d\n", clientes[i].totalViajes);

Second, when you go to free the strings for the deleted element, you're deleting the wrong one. You're using index totalClientes, which is off the end of the array. Reading past the end of the array, as well as attempting to free memory that wasn't received from malloc, invokes undefined behavior.

You presumably wanted to do this for the last element, which has index totalClientes-1, but that's not correct either, since that contains the strings for the last element in the list. The pointers to the memory you wanted to remove are in index posCliente, which you overwrite when moving the element, causing a memory leak. Move the calls to free to before the loop that shifts the elements, and use posCliente as the index to clean up:

// Remove / free memory of the element deleted
free(clientes[posCliente].dni);
free(clientes[posCliente].nombre);
free(clientes[posCliente].apellidos);
free(clientes[posCliente].direccion);

// Rewind one position array clientes since cliente we want to remove
for (i = posCliente; i < totalClientes-1; i++) {
    // Rewind one element array clientes
    clientes[i] = clientes[i+1];
}

Finally, you can't call free on the last element of the array, since 1) it's not a pointer, and 2) even if you took its address, that address wasn't returned by malloc. The realloc you have commented out is fine. The reason it crashed was due to undefined behavior that appeared earlier in your program.

Also, fflush(stdin) is not valid, and gets is insecure and should not be used because it could allow you to overrun your buffers. Use scanf instead with length modifiers to specify the maximum size:

    printf("Input NIF: ");
    scanf("%254s", dni);
    printf("Input NAME: ");
    scanf("%254s", nombre);
    printf("Input SURNAME: ");
    scanf("%254s", apellidos);
    printf("Input ADDRESS: ");
    scanf("%254s", direccion);

Ok, thank you so much!! When I changed free lines up, problem was solved, both things works fine! I am so grateful. Also, your information is so good to, I will take it in care for change proyect! Thank you, again :). I have one question: If I put ´free´'s lines after totalClientes++; this works fine too right? (it must delete last item of `clientes`). I have tryed it, and it does not give error. But last element if I print it, continues existing, is not deleted — JuMoGar, Mar 23 '18 at 16:13
@JuMoGar Dereferencing pointers that have already been free'ed is undefined behavior. The physical data *might* still be there, but there's no guarantee. — dbush, Mar 23 '18 at 16:43
Oh, ok, thank you!. Then, It is correct too do: totalClientes--; and after free's? — JuMoGar, Mar 23 '18 at 16:46
@JuMoGar Yes, but then you'll have to adjust the loop which moves everything one position if you put `totalClientes--` before the loop. — dbush, Mar 23 '18 at 16:47

array of structs created with malloc is not deleting nor resizing last element using free and realloc

1 Answers1