[Перевод] Исследование нескольких проблем, обнаруженных при статическом анализе
В последнее время мы занимались статическим анализом нашей кодовой базы. В результате было выявлено несколько проблем в коде C++, которые мне пришлось исправлять. Это в очередной раз помогло мне осознать, каково совершать такие ошибки, которые обычно трудно найти, просто взглянув на код (человеческим глазом). Я считаю, что стоит поделиться опытом решения некоторых из этих проблем. Не могу опубликовать мой реальный код, он все равно будет слишком сложным, но я использую несколько простых фрагментов, в которых продемонстрированы те же проблемы, что встретились мне в проанализированном коде. Это (надеюсь) поможет вам легко понять как проблему, так и её решение.
Удаление элементов из вектора с применением цикла
Первая задача, которую мы обсудим — это удаление элементов из контейнера, например, std: vector, при помощи цикла. Наш код выглядел примерно так, как показано в следующем упрощённом фрагменте:
int main()
{
std::vector data{ 1,1,2,3,5,8,13,21,34,55 };
for (auto it = data.begin(); it != data.end();)
{
/* делаем что-то с *it */
if (*it % 2 == 0)
{
data.erase(it);
}
else
{
++it;
}
}
for (auto const& e : data)
std::cout << e << '\n';
}
Здесь у нас есть вектор, содержащий данные, и цикл, в котором каждый элемент вектора проверяется на соответствие некоторому условию. Те элементы, которые соответствуют этому условию, удаляются из контейнера. Вы уже заметили, в чём проблема?
Мы используем std: vector: erase для удаления элемента. Таким образом удаляется элемент, занимающий указанную позицию, и инвалидируются все итераторы и ссылки в точке удаления или после неё, включая итератор end (). При удалении элемента итератор не приращивается, поэтому в следующем цикле будет обрабатываться только что удаленный элемент. А это добром не кончится.
Решение довольно простое: присвойте переменной it значение, возвращаемое функцией erase () (это итератор, следующий за удалённым элементом).
it = data.erase(it);
Это и есть решение. Но есть ли решение получше? Да, мы можем использовать общий алгоритм для удаления всех нужных нам элементов. Для этого мы воспользуемся std: remove_if. Можно заменить цикл for, описанный выше, на следующий:
data.erase(std::remove_if(data.begin(), data.end(), [](int const n) {return n % 2 == 0; }),
data.end());
Нужно по возможности использовать стандартные алгоритмы, потому что тогда, как правило, придётся написать меньше кода, и вероятность возникновения ошибок в этом коде будет невелика.
Возврат без освобождения динамически выделенной памяти
Второй пример, который я хочу привести, — это утечка памяти, которая происходит в функции, выделяющей память, а затем возвращающейся, не освободив эту память. Проблему можно проиллюстрировать следующим фрагментом:
void Process(int* ptr)
{
std::cout << *ptr << '\n';
delete ptr;
}
void Demo(int const limit)
{
int* ptr = new int(rand() % 100);
if (*ptr > limit)
{
return;
}
Process(ptr);
}
Это может показаться глупым, но представьте, что вместо int есть пользовательский класс, и предусмотрено несколько проверок объекта (или, возможно, связанных с ним данных). Причём, в некоторых случаях выполнение функции должно прекратиться. В противном случае выделенный объект передаётся функции (которая, возможно, получает его во владение). Хватит малейшего недосмотра (особенно если это более длинная и сложная функция), чтобы забыть удалить выделенную память перед возвратом из функции.
Для исправления ситуации можно использовать std: unique_ptr, который хорош освобождением собственного динамически выделенного объекта при выходе из области видимости.
void Demo(int const limit)
{
std::unique_ptr ptr = std::make_unique(rand() % 100);
if (*ptr > limit)
{
return;
}
Process(ptr.release());
}
Если вам нужно передать основной объект в функцию, которая принимает необработанный указатель и получает объект во владение, просто используйте std: unique_ptr: release (). Это открепит управляемый объект от умного указателя, передавая владение объектом вызывающей функции, именно той, которая отвечает за его высвобождение.
И это приводит к следующей проблеме…
Путаница между освобождением и сбросом
Класс std: unique_ptr имеет два метода:
• release (): освобождает управляемый объект, передавая владение вызывающей стороне.
• reset (): заменяет управляемый объект на другой (принимает аргумент null в качестве значения по умолчанию); если умный указатель содержит не нулевой указатель на объект, этот объект удаляется.
Следующий фрагмент содержит ошибку:
std::unique_ptr MakeObject(int const limit)
{
std::unique_ptr ptr = std::make_unique(rand() % 100);
if (*ptr > limit)
{
ptr.release();
}
return ptr;
}
Если условие не выполняется, управляемый объект высвобождается. Но на самом деле предполагается его удаление. Это приводит к утечке памяти. В данном случае правильно вызвать reset ():
std::unique_ptr MakeObject(int const limit)
{
std::unique_ptr ptr = std::make_unique(rand() % 100);
if (*ptr > limit)
{
ptr.reset();
}
return ptr;
} Подробнее об утечках памяти
Выше было показано, как можно использовать std: unique_ptr, чтобы избежать утечки объектов, выделенных в куче. Но что если нам нужно выделить и освободить массив объектов? Класс std: unique_ptr поддерживает массивы, но, вместе с тем, имеет такие недостатки: размер массива фиксирован, копирование невозможно, std: unique_ptr не является контейнером и поэтому не совместим с алгоритмами и концепциями. Но есть контейнеры, такие как std: vector, которые могут быть использованы именно в таких целях.
В качестве примера рассмотрим следующий фрагмент:
void Demo()
{
DWORD size = 0;
TCHAR* buffer = nullptr;
BOOL ret = ::GetUserName(nullptr, &size);
if(!ret && ::GetLastError() == ERROR_INSUFFICIENT_BUFFER)
buffer = new TCHAR[size];
if (buffer)
{
::GetUserName(buffer, &size);
std::cout << buffer << '\n';
}
}
Многие API Windows требуют, чтобы в распоряжении был буфер определённого размера. Но их можно вызвать и с нулевым буфером, и они вернут размер, необходимый для буфера, чтобы затем вы могли выделить необходимую память. Именно это и происходит в данном фрагменте, который извлекает и выводит имя текущего пользователя. Но выделенный буфер никогда не освобождается (из-за ошибки). Этого можно надёжно избежать, если использовать std: vector, который позаботится о выделении и освобождении внутрисистемно.
void Demo()
{
DWORD size = 0;
std::vector buffer;
BOOL ret = ::GetUserName(nullptr, &size);
if (!ret && ::GetLastError() == ERROR_INSUFFICIENT_BUFFER)
buffer.resize(size);
if (!buffer.empty())
{
::GetUserName(buffer.data(), &size);
std::cout << buffer.data() << '\n';
}
}
Независимо от того, как возвращается эта функция (нормально или даже из-за исключения), память, выделенная объектом std: vector, в этот момент освобождается.
Количество элементов в массиве
Я много раз встречал этот паттерн в унаследованном коде. Где-то объявляется массив (обычно в глобальной области видимости), а затем используется макрос для определения количества элементов в массиве. Вот небольшой фрагмент:
int AnArray[] = {1, 1, 2, 3, 5, 8};
#define COUNT_OF_ARRAY sizeof(AnArray) / sizeof(int)
int main()
{
for (size_t i = 0; i < COUNT_OF_ARRAY; ++i)
{
std::cout << AnArray[i] << '\n';
}
}
Это неудачный стиль, который чреват ошибками. Одну из них (которую было бы трудно заметить) я приведу ниже. Но статический анализ сразу же поймал её:
int AnArray[] = {1, 1, 2, 3, 5, 8};
int main()
{
for (size_t i = 0; i < sizeof(AnArray); ++i)
{
std::cout << AnArray[i] << '\n';
}
}
Предполагается перебрать все элементы массива, но sizeof (AnArray) возвращает размер, занимаемый массивом в памяти, а не количество его элементов. Это приведет к выходу за границы.
Существует несколько решений этой проблемы. Одно из них — использовать некомпонентную функцию std: size (), позволяющую получить количество элементов в массиве:
int AnArray[] = {1, 1, 2, 3, 5, 8};
int main()
{
for (size_t i = 0; i < std::size(AnArray); ++i)
{
std::cout << AnArray[i] << '\n';
}
}
Целесообразнее было бы использовать std: array вместо C-подобных массивов. Это даст множество преимуществ, включая лёгкое получение количества элементов с помощью компонентной функции size () (или некомпонентной std: size ()).
std::array AnArray {1, 1, 2, 3, 5, 8};
int main()
{
for (size_t i = 0; i < AnArray.size(); ++i)
{
std::cout << AnArray[i] << '\n';
}
}
Итак, статический анализ отлично помогает выявлять в коде такие проблемы, которые иначе трудно заметить. Несколько приведенных здесь примеров должны наглядно продемонстрировать это.
