Malloc + free + chdir = segmentation fault?

#linux #c #malloc

Приветствую всех!

Есть такой "код":

#include 
#include 
#include 
#include 
#include 
#include 

/*
 *
 */

// Структура хранящая список содержимого каталога в виде
// массива строк (аля argv). Память для записей выделяется
// из кучи.

typedef struct {
    unsigned int count;
    char **list;
} dirslist_t;


/// Очищает кучу по адресам из принятой структуры.
/// \param dl

void dirs_list_free(dirslist_t *dl) {

    while (dl->count) {
        free(*(dl->list + dl->count));
        dl->count--;

        if (!dl->count) {
            free(*(dl->list)); 
            free(dl->list);
        }

    }
}


/// Возвращает указатель на структуру dirslist_t заполненную новыми данными. 
/// \param dl
/// \param dst_name
/// \return 

dirslist_t *dirs_list(char *dst_name) {

    dirslist_t *dl = malloc(sizeof (dirslist_t));

    // Создаем новые записи. Сразу закинем в начало списка
    // обозначения дочернего и родительского каталогов.
    dl->list = (char**) malloc(sizeof (char*) * 3);

    *(dl->list) = malloc(sizeof (char) * sizeof ("."));
    strcpy(*(dl->list), ".");

    *(dl->list + 1) = malloc(sizeof (char) * sizeof (".."));
    strcpy(*(dl->list + 1), "..");
    // Указваем что в списке уже есть два элемента.
    dl->count = 2;


    // Структура описатель "потока" каталога.
    DIR *DIRp;
    // Структура описатель текущего каталога.
    struct dirent *current_dir;
    // Открываем каталог.
    DIRp = opendir(dst_name);
    // Если вернулся не NULL начинаем обходить записи данного каталога.
    if (DIRp) {
        // Повторяем пока не NULL т.е. пока не закончились записи.
        while ((current_dir = readdir(DIRp)) != NULL) {

            if (!strcmp(current_dir->d_name, "."))
                continue;
            if (!strcmp(current_dir->d_name, ".."))
                continue;

            // Выделяем место под название каталога.
            *(dl->list + dl->count) = malloc(strlen(current_dir->d_name) + 1);
            // Копируем имя в список.
            strcpy(*(dl->list + dl->count), current_dir->d_name);

            // Увеличиваем список.
            dl->count++;
            dl->list = realloc(dl->list, sizeof (char*) * (dl->count + 1));

        }
        //!!!_Если раскомментировать этот printf то упадет раньше_!!!
        //printf("%d\n", (int) dl->count);
        closedir(DIRp);
        return dl;
    } else {
        printf("Return: NULL!\n");
        return NULL;
    }

}
////////////////////////////////////////////////////////////////////////////////

int main(int argc, char **argv) {


    dirslist_t *mdirs;

    for (int k = 0; k < 10; k++) {
        mdirs = dirs_list(".");
        for (int i = 0; i < mdirs->count; i++) {
            printf("%s\n", *(mdirs->list + i));
        }
        dirs_list_free(mdirs);
        free(mdirs);

        chdir("..");
    }

}


Если в функции main закомментировать только chdir("..") то всё работает (выводит
содержимео текущего каталога 10 раз), если же в функции main закомментировать только
dirs_list_free(mdirs) то всё работает как мне надо - показывает список файлов в текущем
каталоге потом, переходит к родительскому и все повторяется, но память естественно
течет, НО, если раскомментированные обе строки то на 2-3 итерации выпадает Segmentation
fault!
В первом случае Valgrind ругается на ошибки, во втором на утечку памяти.
Это лиш кусочек небольшого велосипеда обрезанный до сути. 2й день бьюсь с этой ошибкой.
Перепробовал уже уйму вариантов.
Makefile:

CC=gcc
CFLAGS=-std=c11 -Wall

all:
    $(CC) $(CFLAGS) main.c -o el1
clean:
    rm el1


Заранее Большое Спасибо!
    

Ответы

Ответ 1

Во-первых, хочу сказать, что если у вас есть такой божественный инструмент как Valgrind,
то вам очень повезло. Любите его, уважайте его выхлоп и чините то, что он вам говорит.
Это ваш лучший друг, сразу после компилятора.

В случае вашего кода, Valgrind сразу нашел 2 места, откуда происходит ошибка: место
аллокации и место освобождения памяти. А если собрать с -ggdb, то даже номера строк
дадут. Вам сразу покажут пальцем, откуда произрастает проблема.

Использовать gdb для новичка может быть, наверное, не очень весело, потому новички
часто используют отладку принтами. Добавте перед освобождением указателя следующую строчку:

      fprintf(stderr,"Current element: %d, current pointer %p\n",dl->count, *(dl->list
+ dl->count));


После чего запустите - вы сразу увидите, что что-то пошло не так... После чего вы
можете что-то сделать, чтобы не освобождать последний элемент, память под который была
выделена, но сам элемент не аллоцирован. К примеру, в начале функции сделать:

      dl->count--


И мир заиграет яркими красками счастья и удовольствия.

Про другие странности можете почитать другие ответы, но я постарался описать то,
как надо отлаживать, чтобы подобных вопросов больше не возникало.


Ответ 2

Первая ошибка, которая бросается в глаза: у вас массив dl->list всегда содержит dl->count
+ 1 элемент, т.е. на один элемент больше, чем фактически используется. При этом последний
(неиспользуемый) элемент dl->list[dl->count] не инициализирован, т.е. содержит мусор.

Тем не менее в функции dirs_list_free вы сразу смело делаете 

free(*(dl->list + dl->count))


т.е. применяете free к "мусорному" указателю. Поведение не определено.

Если вам хочется, чтобы в массиве dl->list у вас в конце стоял "запасной" guard element,
то пусть он там будет - это, в принципе, неплохая идея, по аналогии со стандартными
argc/argv[]. Но хотя бы проинициализируйте его нулевым указателем.
Странность: что вот это

 if (!dl->count) {
    free(*(dl->list)); 
    free(dl->list);
 }


делает внутри цикла в dirs_list_free? Зачем было засовывать это внутрь цикла и затем
проверять if (!dl->count) на каждой итерации, если можно было просто сделать это один
раз после цикла?
Странность: функция dirs_list выделяет память для самой структуры dirslist_t, а вот
функция dirs_list_free эту память не освобождает. Обязанность освобождать саму структуру
взвалена на "пользователя". Почему? В честь чего такая ассиметрия?


P.S. Зачем вам сдался этот дубовый синтаксис *(dl->list + i)? Почему бы не писать
по человечески dl->list[i]?