Самая опасная функция в мире с/с++

Красивый Chromium и корявый memset +50

  • 26.01.18 08:23


Andrey2008

#347594

Хабрахабр

11200

C++, Информационная безопасность, Google Chrome, Open source, Блог компании PVS-Studio

Предлагаем вашему вниманию цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте Chromium. Это первая часть, которая будет посвящена функции memset.
Господа программисты, с функцией memset надо что-то делать в C++ программах! Вернее, даже сразу понятно что делать — её надо прекратить использовать. В своё время я написал статью «Самая опасная функция в мире С/С++». Я думаю, несложно догадаться, что речь в статье идёт как раз о memset.
Однако, не буду голословным и ещё раз на примерах продемонстрирую опасность этой функции. Код проекта Chromium и используемых в нём библиотек очень качественный. Разработчики Google уделяют много внимания тестам и использованию различного инструментария для выявления дефектов. Например, Google разработал такие инструменты, как AddressSanitizer, ThreadSanitizer и MemorySanitizer.
В результате, ошибок, связанных с функций memset, очень мало, но печально, что они всё-таки есть. Очень качественный проект, но всё равно они есть!
Давайте посмотрим, что я заметил в процессе разбора отчета, выданного PVS-Studio. Как я писал в вводной статье, отчёт я смотрел достаточно бегло, поэтому могут быть и другие, незамеченные мной ошибки. Тем не менее, найденных дефектов нам будет достаточно для обсуждения функции malloc.

Новое наблюдение

Теперь у меня есть ещё одно интересное наблюдение. Используя те или иные функции, программисты могут допускать ошибки. При этом вероятность допущения ошибки зависит от используемой функции. Другими словами, какие-то функции провоцируют ошибки, а какие-то — нет.

Так вот, я готов назвать функцию, при использовании которой есть наибольшая вероятность сесть в лужу.

Итак, победитель на глючность — функция memset!

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

Почетное второе место занимает функция printf() и её разновидности. Думаю, это никого не удивит. Про опасность функции printf() не писал только ленивый. Возможно из-за общеизвестности связанных с printf() проблем, она и попала на второе место.

Всего у меня в базе 9055 ошибок. Это те ошибки, которые умеет находить анализатор PVS-Studio. Понятно, что он умеет далеко не всё. Однако большое количество найденных ошибок позволяет мне быть уверенным в своих выводах. Так вот, я посчитал, что с использованием функции memset() связано 329 ошибок.

Итого, около 3,6% ошибок в базе связано с функцией memset(). Это много!

Возвращаемое значениеReturn Value

Возвращает нуль в случае успеха или код ошибки в случае неудачи.Zero if successful; an error code on failure.

Ситуации, которые могут привести к ошибкеError Conditions

destdest дестсизеdestSize srcsrc countcount Возвращаемое значениеReturn value Содержимое конечного объектаContents of dest
anyany anyany anyany Без измененийNot modified
ЗАКАНЧИВАЮЩNULL anyany anyany ненулевое значениеnon-zero еинвалEINVAL Без измененийNot modified
anyany anyany ЗАКАНЧИВАЮЩNULL ненулевое значениеnon-zero еинвалEINVAL конечный адрес обнуленdest is zeroed out
anyany < расчета< count anyany ненулевое значениеnon-zero ERANGEERANGE конечный адрес обнуленdest is zeroed out

Ошибка затирания приватных данных

memset memset CWE-14memset Безопасная очистка приватных данных

V597memset бывает

  • V597 CWE-14 The compiler could delete the ‘memset’ function call, which is used to flush ‘sensitive’ object. The RtlSecureZeroMemory() function should be used to erase the private data. httpcommon.cc 721
  • V597 CWE-14 The compiler could delete the ‘memset’ function call, which is used to flush ‘sensitive’ object. The RtlSecureZeroMemory() function should be used to erase the private data. httpcommon.cc 766
  • V597 CWE-14 The compiler could delete the ‘memset’ function call, which is used to flush ‘sensitive’ object. The RtlSecureZeroMemory() function should be used to erase the private data. httpcommon.cc 917

Рекомендацияmemset RtlSecureZeroMemorymemset_sN1

N2

OPENSSL_cleanse

ПримечанияRemarks

memcpy копирует число байтов из src в dest; wmemcpy копирует символы расширенного числа (2 байта).memcpy copies count bytes from src to dest; wmemcpy copies count wide characters (two bytes). Если источник и назначение перекрываются, поведение memcpy не определено.If the source and destination overlap, the behavior of memcpy is undefined. Используйте memmove для управления перекрывающимися областями.Use memmove to handle overlapping regions.

Важно!

Убедитесь в том, что буфер назначения равен или превосходит по размеру исходный буфер.Make sure that the destination buffer is the same size or larger than the source buffer. Дополнительные сведения см. в разделе Как избежать переполнения буфера.For more information, see Avoiding Buffer Overruns.

Важно!

Так как большое количество переполнений буфера и, таким образом, потенциальные эксплойты безопасности, отслеживаются в неправильном использовании memcpy, эта функция указана между «запрещенными» функциями жизненного цикла разработки безопасности (SDL).Because so many buffer overruns, and thus potential security exploits, have been traced to improper usage of memcpy, this function is listed among the «banned» functions by the Security Development Lifecycle (SDL). Можно заметить, что некоторые классы библиотек VC + + продолжают использовать memcpy.You may observe that some VC++ library classes continue to use memcpy. Кроме того, можно заметить, что оптимизатор компилятора VC + + иногда создает вызовы memcpy.Furthermore, you may observe that the VC++ compiler optimizer sometimes emits calls to memcpy. Язык Visual C++ разрабатывался в соответствии с требованиями SDL, поэтому использование этой запрещенной функции тщательно анализировалось.The Visual C++ product is developed in accordance with the SDL process, and thus usage of this banned function has been closely evaluated. В случае использования в библиотеке вызовы были тщательно изучены на предмет того, не могут ли они вызвать переполнение буфера.In the case of library use of it, the calls have been carefully scrutinized to ensure that buffer overruns will not be allowed through these calls. В случае компилятора иногда определенные шаблоны кода распознаются как идентичные шаблону memcpyи, таким образом, заменяются вызовом функции.In the case of the compiler, sometimes certain code patterns are recognized as identical to the pattern of memcpy, and are thus replaced with a call to the function. В таких случаях использование memcpy является более ненадежным по сравнению с исходными инструкциями. они просто оптимизированы для вызова функции memcpy , настроенной для производительности.In such cases, the use of memcpy is no more unsafe than the original instructions would have been; they have simply been optimized to a call to the performance-tuned memcpy function. Точно так же, как использование «безнадежных» функций CRT не гарантирует безопасность (они просто затрудняют их ненадежность), использование «запрещенных» функций не гарантирует опасности (они просто потребовали более тщательного обеспечения безопасности).Just as the use of «safe» CRT functions doesn’t guarantee safety (they just make it harder to be unsafe), the use of «banned» functions doesn’t guarantee danger (they just require greater scrutiny to ensure safety).

Поскольку использование memcpy компилятором и библиотеками VC + + было тщательно изучены, эти вызовы разрешены в коде, который в противном случае соответствует SDL.Because memcpy usage by the VC++ compiler and libraries has been so carefully scrutinized, these calls are permitted within code that is otherwise compliant with SDL. вызовы memcpy , появившиеся в исходном коде приложения, соответствуют требованиям SDL, только если они были проверены экспертами по безопасности.memcpy calls introduced in application source code are only compliant with the SDL when that use has been reviewed by security experts.

Функции memcpy и wmemcpy будут считаться устаревшими только в том случае, если константа _CRT_SECURE_DEPRECATE_MEMORY определена до инструкции включения, чтобы функции были устаревшими, например, в примере ниже:The memcpy and wmemcpy functions will only be deprecated if the constant _CRT_SECURE_DEPRECATE_MEMORY is defined prior to the inclusion statement in order for the functions to be deprecated, such as in the example below:

илиor

Не полностью очищенные массивы

Как уже говорилось, в проекте MAME можно встретить большое количество мест, где некорректно используется функция memset. Типичной ошибкой является заполнение только части массива. Рассмотрим простой пример:

PVS-Studio: V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_pstars_regs’. pgm.c 4458

Число 16 означает количество элементов в массиве «m_pstars_regs». Однако в функцию memset следует передавать количество заполняемых в буфере байт. В результате, только часть массива будет заполнена нулями.

Корректный вариант кода:

Ошибка тривиальна. Программисты часто думают, что тривиальных ошибок в программах мало (см. второй миф ). Это не так. Как раз большинство ошибок в программах являются простыми и глупыми.

Как вы считаете, приведенная выше ошибка единична? Нет. Вот как минимум ещё 8 мест, где можно встретить ошибки идентичного типа:

  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_kb_regs’. pgm.c 4975
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_kb_regs’. pgm.c 4996
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_kb_regs’. pgm.c 5056
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_oldsplus_ram’. pgm.c 5780
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_oldsplus_regs’. pgm.c 5781
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_sysreg’. rungun.c 399
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_ttl_vram’. rungun.c 400
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_playfield_code’. malzak.c 392

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

PVS-Studio: V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_control_0’. tumbleb.c 2065

Количество элементов в массиве вычисляется с помощью макроса ARRAY_LENGTH. Это опять неправильно. Здесь нужно было вычислить размер массива, а не количество элементов в нем.

Возможно два способа исправления.

Первый:

Второй:

Вот ещё несколько мест, где подобным образом неудачно заполняются массивы:

  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_pmac_read’. megadriv.c 7156
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_pmac_write’. megadriv.c 7157
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_cart_is_genesis’. megatech.c 426
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_vol_ctrl’. nycaptor.c 841
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_rotate_ctrl’. wgp.c 949
  • V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘state->m_vreg’. othldrby.c 237

На этом злоключения с функцией memset() заканчиваются. Хотя возможно, я приспустил некоторые ошибки. Но ещё есть не менее страшная функция memcpy().

Undefined behavior

Достаточно много предупреждений, выданных PVS-Studio, относится операциям сдвигов. Эти операции приводят к undefined behavior. Конечно, при использовании конкретных компиляторов, программа может вести себя правильно многие годы. Поэтому ошибки можно назвать потенциальными. Они могут проявить себя при смене платформы, компиляторов, ключей оптимизации. Подробнее рассуждения на эту тему приводятся в статье: «Не зная брода, не лезь в воду. Часть третья.» .

Рассмотрим пару примеров, приводящих к неопределенному поведению. Пример первый:

PVS-Studio: V610 Undefined behavior. Check the shift operator ‘<<. The left operand ‘~0’ is negative. atarig42.c 220

Любой фрагмент кода, использующий макрос ATARIRLE_PRIORITY_MASK приводит к неопределенному поведению. Нельзя сдвигать отрицательные числа. Этот макрос лучше переписать так:

А вот другой, более длинный пример:

PVS-Studio: V610 Undefined behavior. Check the shift operator ‘<<. The right operand (‘i’ = ) is greater than or equal to the length in bits of the promoted left operand. firetrk.c 111

В массиве ‘colortable_source’ находится 44 элемента. Соответственно, счетчик цикла ‘i’ принимает значения от 0 до 43. Число ‘1’ имеет тип int. Его нельзя сдвигать более чем на 31 бит. Сдвиг на большее количество, согласно стандарту, приводит к Undefined behavior.

Так как предупреждения, связанных со сдвигами крайне много, нет смысла приводить их в статье. Список этих сообщений, можно посмотреть в текстовом файле: mame-shift-ub.txt.

Другие ошибки

Помимо memset() и memcpy(), я чуть не забыл про memcmp(). Эта функция из той же банды. К счастью в проекте MAME я обнаружил только одну ошибку, связанную с её использованием.

PVS-Studio: V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. apridisk.c 128

Оператор sizeof() вычисляет не количество байт в строке, а размер указателя. В результате, будет сравниваться только несколько первых байт. Ситуацию можно исправить, объявив переменную ‘apr_magic’ как массив:

Пример выражения, которое всегда истинно:

PVS-Studio: V547 Expression is always true. Probably the ‘&&’ operator should be used here. mpu4.c 934

Условие «X != 1 || X != 0» всегда истинно. Скорее всего, вместо оператора ‘||’ здесь следует написать ‘&&’.

Использование указателя до его проверки. Приведу только один пример. Я видел и другие сообщения V595, но не стал их выписывать. Часто код вполне работоспособен, так как указатель в этом коде никогда не равен нулю. Пример подозрительного кода:

PVS-Studio: V595 The ‘gfx’ pointer was utilized before it was verified against nullptr. Check lines: 2457, 2483. stvvdp2.c 2457

Попадается странный код, по который я не могу с уверенностью сказать, есть в нём ошибка или нет. Возможно, в нём имеется ошибка Copy-Paste. А возможно, всё верно и две ветви кода действительно должны совпадать. Пример:

PVS-Studio: V523 The ‘then’ statement is equivalent to the ‘else’ statement. deco16ic.c 943

В независимости от условия выполняется одно и то же действие. А вот ещё один аналогичный пример:

PVS-Studio: V523 The ‘then’ statement is equivalent to the ‘else’ statement. resnet.c 628

Опечатки и Copy-Paste

В любом проекте можно встретить опечатки и ошибки, возникшие из-за использования технологии Copy-Paste. Где-то больше таких ошибок, где-то меньше. В MAME их нашлось не много, но всё-таки они есть. Рассмотрим некоторые примеры.

PVS-Studio: V517 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 577, 584. tms7000.c 577

Если присмотреться, то можно заметить, что первое и второе условия идентичны. В них изменён порядок сравнений, но это никак не сказывается на результате.

Рассмотрим другой пример.

PVS-Studio: V524 It is odd that the body of ‘max_opcode_bytes’ function is fully equivalent to the body of ‘min_opcode_bytes’ function (debugcpu.h, line 150). debugcpu.h 151

Функция max_opcode_bytes() идентична функции min_opcode_bytes(). Скорее всего это неправильно. Я думаю, функция min_opcode_bytes()должна была выглядеть так:

Некоторые другие фрагменты кода, скорее всего являющиеся опечатками:

  • V583 The ‘?:’ operator, regardless of its conditional expression, always returns one and the same value: «,(%d,». 9900dasm.c 670
  • V517 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 549, 579. cdrom.c 549
  • V501 There are identical sub-expressions ‘offset != (0x370 >> 1)’ to the left and to the right of the ‘&&’ operator. decoprot.c 118
  • V501 There are identical sub-expressions ‘offset != (0x3c0 >> 1)’ to the left and to the right of the ‘&&’ operator. decoprot.c 118
  • V501 There are identical sub-expressions ‘offset != 0x2c / 2’ to the left and to the right of the ‘&&’ operator. decoprot.c 240
  • V501 There are identical sub-expressions ‘offset != 0xe’ to the left and to the right of the ‘&&’ operator. decoprot.c 447

Заключение

Как всегда отмечу, что в статье, скорее всего, описаны далеко не все ошибки, которые можно найти в MAME с помощью PVS-Studio. Задача статьи – показать, что PVS-Studio учится проверять кроссплатформенные проекты. Как именно интегрироваться в make-файл, можно узнать из документации. Также вы можете обратиться к нам, если у вас возникнут затруднения с проверкой проектов, собираемых с помощью MinGW.

P.S. Просмотр результатов анализа сейчас подразумевает наличие среды Visual Studio, где можно открыть и изучить отчёт. Анализировать отчёт вручную затруднительно. Возможно, в дальнейшем появится специальный инструмент, позволяющий удобно просматривать отчёт и осуществлять навигацию по коду, не имея установленного Visual Studio.

Оцените статью
Рейтинг автора
5
Материал подготовил
Андрей Измаилов
Наш эксперт
Написано статей
116
Добавить комментарий