Zwolnienie dynamicznie alokowanej tablicy duwymiarowej

Z pliku binarnego ładuję do dwuwymiarowej tablicy wartości int. Końcem wiersza jest -1. Tablicę realokuję po drodze:

while (fread(&num, sizeof(int), 1, file) == 1) {
    if (columns == 0) { //pierwszy element wiersza
        tmp2 = (int**)realloc(arr, (row + 1) * sizeof(int*)); //realokacja wierszy

        if (!tmp2) { //czyszczenie w przypadku niepowodzenia
            fclose(file);
            for (int k = 0; k < row; k++) {
                free(*(arr + k));
            }
            free(arr);
            return 4;
        }
        arr = tmp2;

        *(arr + row) = NULL;
        columns++;
    }

    tmp1 = (int*)realloc(*(arr + row), columns * sizeof(int)); //reaolokacja kolumn
    if (!tmp1) { // czyszczenie w przypadku niepowodzenia
        fclose(file);
        for (int k = 0; k < row; k++) {
            free(*arr + k);
        }
        free(arr);
        return 4;
    }
    *(arr + row) = tmp1;
    *(*(arr + row) + columns - 1) = num;
    if (num == -1) {
        row++;
        columns = 0;
    }
    else
        columns++;
}

Tablica napełnia się prawidłowo, funkcja wyświetlająca dane działa prawidłowo, niestety podczas zwalniania pamięci:

void destroy(int ***ptr) {
    if (*ptr != NULL) {
        int** p = *ptr;
        int i=0;
        while (*(p+i) != NULL) {
            free (*(p+i));
            *(p+i)=NULL;
            i++;
        }
        free(p);

        *ptr = NULL;
    }
    return;
}

już przy pierwszym wywołaniu free(*(p+i)) otrzymuję komunikat: Wykryto naruszenie granic bloku pamięci.

W czym tkwi problem?

pozdrawiam
Rafał

Daj cały kod.

Nie podoba mi się to ale bez całości nie ma co się zabierać za pisanie uwag

Cały kod funkcji load:

int load(const char *filename, int ***ptr, enum save_format_t format) {
    FILE *file;
    int row = 0, columns = 0;
    int num = 0;
    int **arr = NULL;
    int **tmp2 = NULL;
    int *tmp1 = NULL;
    file = fopen(filename, (format == fmt_binary) ? "rb" : "r");

    if (!file) return 2;
    
    // BINARY
   if (format == fmt_binary) {
        while (fread(&num, sizeof(int), 1, file) == 1) {
                if (columns == 0) {    //pierwszy element wiersza
                        tmp2 = (int**) realloc(arr, (row + 1) * sizeof(int *));

                        if (!tmp2) {  //czyszczenie w przypadku niepowodzenia
                            fclose(file);
                            for (int k = 0; k < row; k++) {
                                free(*(arr + k));
                            }
                            free(arr);
                            return 4;
                        }
                        arr = tmp2;

                    *(arr+row) = NULL;
                    columns++;
                }

                tmp1 = (int*)realloc(*(arr+row), columns * sizeof(int));
                if (!tmp1) {  // czyszczenie w przypadku niepowodzenia
                    fclose(file);
                    for (int k = 0; k < row; k++) {
                        free(*arr+k);
                    }
                    free(arr);
                    return 4;
                }
                *(arr+row)=tmp1;
                *(*(arr+row)+columns - 1) = num;
            if (num == -1) {
                row++;
                columns = 0;
            }
            else    columns++;

        }

    }

    fclose(file);

    if (!arr) {
        return 3;
    }
    else *(arr+row) = NULL;
    *ptr = arr;

    return 0;
}

Ile bajtów ma int w pliku. Tej informacji nie podajesz. Pewnie 32 ale polegać, że jakoś to będzie to słaby pomysł. Przy tworzeniu i czytaniu plików binarnych jak masz int jak tutaj to używasz int32_t.
To raz.

Dwa: moim zdaniem powinieneś zwracać int ( * funkcja ) [2] i to by cię urządzało, na błędzie zwracasz NULL. Allokuj od razu po dwa i czytaj po dwa.

Trzy jak jest binarny to otwieraj z rb po prosu. Po co to przekazywanie enum.

Sprawdzaj błędy na wszystkich funkcjach. (fread -> masz ferror do sprawdzenia czy EOF czy nie )
'man funkcja ' i patrzysz na return.

return 2 3 4 na błędach jest bez sensu. Zwracaj -1 albo NULL.
Od ustalania co się stało masz errno. Albo jakieś swoje 'własne errno' ewentualnie.

ksh napisał(a):

Ile bajtów ma int w pliku.

4 bajty

Dwa: moim zdaniem powinieneś zwracać int ( * funkcja ) [2] i to by cię urządzało, na błędzie zwracasz NULL. Allokuj od razu po dwa i czytaj po dwa.

nie rozumiem. Innym wymogiem zadania jest alokować tylko tyle, ile jest niezbędne.

Trzy jak jest binarny to otwieraj z rb po prosu. Po co to przekazywanie enum.

to jest jedna z opcji funkcji. W drugiej opcji czyta z plików tekstowych

Sprawdzaj błędy na wszystkich funkcjach. (fread -> masz ferror do sprawdzenia czy EOF czy nie )
'man funkcja ' i patrzysz na return.

return 2 3 4 na błędach jest bez sensu. Zwracaj -1 albo NULL.
Od ustalania co się stało masz errno. Albo jakieś swoje 'własne errno' ewentualnie.

te returny to wymóg zadania

już przy pierwszym wywołaniu free(*(p+i))

Jesteś pewien, że pierwszym? Ile wynosi i gdy dostajesz komunikat?

Na pierwszy rzuk oka load jest prawie mechanicznie poprawny, nie sądzę by każdorazowe wołanie realloc co cztery bajty było dobrym pomysłem, ale powinno to zadziałać jak się spodziewasz. Błąd na pewno masz tutaj else *(arr+row) = NULL; na końcu, zerujesz tutaj pointer poza zaalokowanym zakresem, i potem polegasz na tym fakcie destroy. Jest to UB, które nie uryzło Cię podczas wyświetlania bo to mały, szkolny przykład. Nie jestem natomiast pewny, czy właśnie ten bład nie wychodzi podczas dealokacji. Jak chcesz być pewny, że na końcu tablicy wierszy jest null pointer to również dla niego musisz zaalokować pamięć.

Na Twoim miejscu problem rozwiązałbym poprzez użycie ciągłej pamięci, w ten sposób free będziesz musiał zawołać tylko raz i nie będziesz musiał iterować po wierszach w destroy https://c-faq.com/aryptr/dynmuldimary.html. Nie będziesz musiał się bawić w null pointery a do wyświetlania lepiej zwrócić rozmiary z funkcji load.

Posiedziałem nad tym bo mam przerwę i jest tak jak wyżej Ci napisał ^ Mam na myśli błąd.

Od siebie bym dodał jeszcze to: Dałbym jeden void * tmp
i zwracał z obu realloc wartość do niego. I nie castuj void* bo to nie C++

Alokowanie "tablicy tablic", nie ma sensu z punktu widzenia ew. trafiania w cache oraz zwalniania takiego Behemota. Lepiej alokować pamięć ciągłą i zwolnić ją raz.

Zakładając że to będzie 2-wymiarowa tablica, po przeczytaniu 1 wiersza już będziesz wiedział jaką pamięć ostatecznie chcesz zarezerwować. Stąd takie "ścibolenie" z użyciem realloc(...), nie ma sensu. Po wczytaniu 1 wiersza alokuj całość pamięci. Rozumiem że nie wiesz ile jej będzie. Tu lepiej alokować mnożąc poprzednią wielkość przez 2. Czyli np. przyjmujesz (a priori) że danych będzie 8, po wczytaniu 8 wartości, jeśli będzie następna, realloc(...) zrób na wielkość 16. Analogicznie dalej. Jak osiągniesz koniec wiersza, odkryjesz ile docelowo będzie potrzebne pamięci i zrobisz już realloc(...) ostateczny. A zwolnienie trywialne, bo jedno free(...).

several napisał(a):

już przy pierwszym wywołaniu free(*(p+i))

Jesteś pewien, że pierwszym? Ile wynosi i gdy dostajesz komunikat?

Na pierwszy rzuk oka load jest prawie mechanicznie poprawny, nie sądzę by każdorazowe wołanie realloc co cztery bajty było dobrym pomysłem, ale powinno to zadziałać jak się spodziewasz. Błąd na pewno masz tutaj else *(arr+row) = NULL;

Dzięki, dokładnie to było przyczyną problemu

na końcu, zerujesz tutaj pointer poza zaalokowanym zakresem, i potem polegasz na tym fakcie destroy. Jest to UB, które nie uryzło Cię podczas wyświetlania bo to mały, szkolny przykład. Nie jestem natomiast pewny, czy właśnie ten bład nie wychodzi podczas dealokacji. Jak chcesz być pewny, że na końcu tablicy wierszy jest null pointer to również dla niego musisz zaalokować pamięć.