Освобождение памяти, все? - PullRequest
       3

Освобождение памяти, все?

1 голос
/ 21 сентября 2011

Может быть, плохая тема, но учитывая следующий код, мне тоже нужно free(player->name)?

#include <stdio.h>

struct Player
{
       char *name;
       int level;       
};

int main()
{
    struct Player *player;
    player->name = malloc(sizeof(player->name)*256);
    player->name = "John";

    printf(player->name);

    free(player);

    printf("\n\n\n");
    system("PAUSE");
    return 0;
}

Ответы [ 5 ]

15 голосов
/ 21 сентября 2011

О, мальчик, с чего начать? Вам действительно нужна хорошая книга. Вздох. Давайте начнем с вершины main():


Это

struct Player *player;

определяет указатель на struct Player, но не инициализирует его. Таким образом, оно имеет более или менее случайное значение, указывающее куда-то в память. Это

player->name = malloc(sizeof(player->name)*256);

теперь записывает в части этого случайного местоположения адрес куска памяти, полученный с помощью malloc(). Запись в память через неинициализированный указатель вызывает Неопределенное поведение . После этого все ставки выключены. Не нужно смотреть дальше вашей программы. Вам не повезло, что вы случайно записали в часть памяти, принадлежащую вашему процессу, поэтому он не сразу падает, сообщая вам о проблеме.

Есть два способа улучшить это. Либо придерживайтесь указателя, и он указывает на часть памяти, выделенную для объекта Player. Вы можете получить его, позвонив по номеру malloc(sizeof(Player).
Или просто используйте локальный автоматический (он же стековый) объект:

struct Player player;

Компилятор сгенерирует код для выделения памяти в стеке и автоматически освободит его. Это самый простой способ, и он должен быть выбран по умолчанию.


Однако ваш код имеет больше проблем, чем это.

Это

player->name = malloc(sizeof(player->name)*256);

выделяет последовательную память в куче для хранения 256 указателей на символы и назначает адрес первого указателя (адрес char*, то есть char**) для player->name ( char*). Честно говоря, я удивлен, что даже компилируется, но тогда я более привык к строгому применению типов в C ++. В любом случае, вместо этого вы, вероятно, захотите выделить память для 256 символов:

player->name = malloc(sizeof(char)*256);

(Поскольку sizeof(char) по определению 1, вы часто будете видеть это как malloc(256).)
Однако есть еще кое-что: почему 256? Что если я передам строку длиной 1000 символов? Нет, просто выделение места для более длинной строки не способ справиться с этим, потому что я мог бы передать ей строку еще дольше. Так что либо 1) исправьте максимальную длину строки (просто объявите Player::name массивом char этой длины вместо char*) и установите этот предел в своем коде, либо 2) определяет необходимую длину динамически во время выполнения и выделяет точно необходимую память (длина строки плюс 1, для завершающего '\0' char).

Но становится хуже. Это

player->name = "John";

затем присваивает адрес строкового литерала player->name, переопределяя адрес памяти, выделенной malloc(), в единственной переменной, в которой вы его храните, что приводит к потере и утечке памяти.
Но строки не являются первоклассными гражданами в C, поэтому вы не можете их назначить. Это массивы символов, по соглашению оканчивающиеся на '\0'. Чтобы скопировать их, вы должны скопировать их символ за символом, либо в цикле, либо, предпочтительно, вызвав strcpy().

Чтобы добавить оскорбление к травме, вы позже попытаетесь освободить память, в которой живет строковый литерал

free(player);

тем самым, скорее всего, серьезно зашифровывает структуры данных менеджера кучи. Опять же, вам, кажется, не повезло, что это не вызвало немедленного сбоя, но код, который, казалось бы, работает как задумано, является одной из худших возможностей Undefined Behavior , чтобы проявить себя. Если бы не все ставки были отменены раньше, теперь они были бы полностью.


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

  1. Вам нужна хорошая книга по C ++. Прямо сейчас. Здесь - список хороших книг, собранных программистами C на Stack Overflow. (Я наизусть программист на C ++, поэтому я не буду комментировать их мнение, но K & R, безусловно, хороший выбор.)

  2. Вы должны немедленно инициализировать все указатели, либо с помощью адреса существующего допустимого объекта, либо с помощью адреса фрагмента памяти, выделенного для хранения объекта правильного типа, или с помощью NULL (который вы можно легко проверить на потом). В частности, вы не должны пытаться читать или записывать часть памяти, которая не была выделена вам (динамически в куче или автоматически в стеке).

  3. Вам необходимо free() вся память, которая была получена путем вызова malloc() ровно один раз .

  4. Вы не должны пытаться free() любой другой памяти.


Я уверен, что в этом коде есть что-то еще, но я на этом остановлюсь. И я уже говорил, что вам нужна хорошая книга на С? Потому что вы делаете.

0 голосов
/ 21 сентября 2011

Это ужасный код.Зачем?Во-первых, вы выделяете память для игрока-> имя.malloc возвращает указатель на выделенную память.На следующем шаге вы потеряете это значение указателя, потому что переназначьте player-> name, чтобы указать на статическую строку "John".Может быть, вы хотите использовать функции strdup или sprintf?

Также большой ошибкой является использование неинициализированного указателя на структуру плеера.Попробуйте представить, что он может указывать на случайную ячейку памяти.Так что это хорошая идея выделить память для вашей структуры с помощью malloc.Или не используйте указатель на структуру и используйте переменную реальной структуры.

0 голосов
/ 21 сентября 2011

Хорошо, значит, ваш переменный проигрыватель является указателем, который вы не инициализировали, и поэтому указывает на случайное место в памяти.

Сначала вам нужно выделить память для player, как вы это сделалидля player->name, а затем выделить для player-> name.

Любая память, выделенная с помощью malloc(), должна быть освобождена с помощью free().

Взгляните на это и это .

0 голосов
/ 21 сентября 2011

Вы должны free() все, что вы malloc(), и вы должны malloc() все, что не выделено во время компиляции.

Итак:

Вы должны malloc player, и вы должны освободить player->name

0 голосов
/ 21 сентября 2011

player не нужно освобождать, потому что это никогда не было malloc 'd, это просто локальная переменная стека.player->name нужно освобождать, так как он был выделен динамически.

int main()
{
    // Declares local variable which is a pointer to a Player struct
    // but doesn't actually point to a Player because it wasn't initialised
    struct Player *player;

    // Allocates space for name (in an odd way...)
    player->name = malloc(sizeof(player->name)*256);

    // At this point, player->name is a pointer to a dynamically allocated array of size 256*4

    // Replaces the name field of player with a string literal
    player->name = "John";


    // At this point, the pointer returned by malloc is essentially lost...
    printf(player->name);

    // ?!?!
    free(player);

    printf("\n\n\n");
    system("PAUSE");
    return 0;
}

Я думаю, вы хотели сделать что-то вроде этого:

int main() {
  struct Player player;
  player.name = malloc( 256 );
  // Populate the allocated memory somehow...

  printf("%s", player.name);
  free(player.name);
}
Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...