Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален
Разработка больших сложных проектов невозможна без использования методологий программирования и инструментальных средств, помогающих контролировать качество кода. В первую очередь, это грамотный стандарт кодирования, обзоры кода, юнит-тесты, статические и динамические анализаторы кода. Всё это помогает выявлять дефекты в коде на самых ранних этапах разработки. В этой статье демонстрируются возможности статического анализатора PVS-Studio по выявлению ошибок и потенциальных уязвимостей в коде операционной системы Android. Надеемся, что статья привлечёт внимание читателей к методологии статического анализа кода и они захотят внедрить её в процесс разработки собственных проектов.
Введение
Прошёл год с момента написания большой статьи про ошибки в операционной системе Tizen, и захотелось вновь провести столь же интересное исследование какой-то операционной системы. Выбор пал на Android.
Код операционной системы Android является качественным, хорошо протестированным. При разработке используется как минимум статический анализатор кода Coverity, свидетельством чему являются комментарии вида:
/* Coverity: [FALSE-POSITIVE error] intended fall through */
/* Missing break statement between cases in switch statement */
/* fall through */
В общем, это интересный, качественный проект, и найти в нём ошибки — вызов для нашего статического анализатора PVS-Studio.
Думаю, уже только по объему статьи читатель понимает, что анализатор PVS-Studio отлично справился с поставленной задачей и нашел массу дефектов в коде этой операционной системы.
Common Weakness Enumeration
В статье вы встретите отсылки к Common Weakness Enumeration (CWE). Поясним причину отсылки к этому списку и почему это важно с точки зрения безопасности.
Часто причиной уязвимостей в программах является не какое-то хитрое стечение обстоятельств, а банальная программная ошибка. Здесь будет уместно привести вот эту цитату с сайта prqa.com:
The National Institute of Standards and Technology (NIST) reports that 64% of software vulnerabilities stem from programming errors and not a lack of security features.
Вы можете ознакомиться в статье «Как PVS-Studio может помочь в поиске уязвимостей?» с некоторыми примерами простых ошибок, которые приводили к уязвимостям в таких проектах, как MySQL, iOS, NAS, illumos-gate.
Соответственно, многие уязвимости можно устранить, вовремя обнаружив обыкновенные ошибки и исправив их. И вот здесь на сцену выходит Common Weakness Enumeration.
Ошибки бывают разные, и не все ошибки опасны с точки зрения безопасности. Те ошибки, которые потенциально могут стать причиной уязвимости, собраны в Common Weakness Enumeration. Этот список пополняется, и наверняка существуют ошибки, которые могут приводить к уязвимостям, но они ещё не попали в этот список.
Однако, если ошибка классифицирована согласно CWE, то, значит, теоретически возможно, что она может эксплуатироваться как уязвимость (CVE). Да, вероятность этого мала. Очень редко CWE превращается в CVE. Однако, если вы хотите защитить свой код от уязвимостей, вы должны, по возможности, найти как можно больше ошибок, описанных в CWE, и устранить их.
Схематически взаимосвязь между PVS-Studio, ошибками, CWE и CVE показана на рисунке:
Часть ошибок классифицируется как CWE. Многие из этих ошибок может обнаружить PVS-Studio, тем самым не дав некоторым из этих дефектов превратиться в уязвимости (CVE).
Можно сказать, что PVS-Studio выявляет многие потенциальные уязвимости до того, как они причинили вред. Таким образом, PVS-Studio является средством статического тестирования защищённости приложений (SAST).
Теперь, я думаю, понятно, почему, описывая ошибки, я посчитал важным отметить, как они классифицируются согласно CWE. Так легче показать всю важность применения статического анализатора кода в ответственных проектах, к которым однозначно относятся операционные системы.
Проверка Android
Для анализа проекта я использовал анализатор PVS-Studio версии 6.24. Анализатор на данный момент поддерживает следующие языки и компиляторы:
- Windows. Visual Studio 2010–2017 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. IAR Embedded Workbench, C/C++ Compiler for ARM C, C++
- Windows/Linux. Keil µVision, DS-MDK, ARM Compiler 5/6 C, C++
- Windows/Linux. Texas Instruments Code Composer Studio, ARM Code Generation Tools C, C++
- Windows/Linux/macOS. Clang C, C++
- Linux/macOS. GCC C, C++
- Windows. MinGW C, C++
Примечание. Возможно, некоторые наши читатели пропустили новость о том, что мы поддержали работу в среде macOS, и им будет интересна вот эта публикация: «Релиз PVS-Studio для macOS: 64 weaknesses в Apple XNU Kernel».
Процесс проверки исходных кодов Android не составил проблем, поэтому я не буду на нём останавливаться. Проблему, скорее, составила моя занятость другими задачами, из-за чего я не нашёл времени и сил просмотреть отчёт столь внимательно, как мне бы хотелось. Впрочем, даже беглого просмотра оказалось более чем достаточно, чтобы насобирать большую коллекцию интересных ошибок для солидной статьи.
Самое главное: прошу разработчиков Android не только поправить ошибки, описанные статье, но и провести более внимательный самостоятельный анализ. Я смотрел отчёт анализатора поверхностно и мог пропустить множество серьезных ошибок.
При первой проверке анализатор выдаст много ложных срабатываний, но это не является проблемой. Наша команда готова помочь с рекомендациями по настройке анализатора для сокращения количества ложных срабатываний. Также мы готовы предоставить лицензионный ключ на месяц или более, если это потребуется. В общем, напишите нам, мы поможем и подскажем.
Теперь давайте посмотрим, какие ошибки и потенциальные уязвимости мне удалось найти. Надеюсь, вам понравится то, что умеет находить статический анализатор кода PVS-Studio. Приятного чтения.
Бессмысленные сравнения
Анализатор считает выражения аномальными, если они всегда истинны или ложны. Такие предупреждения, согласно Common Weakness Enumeration, классифицируются как:
- CWE-570: Expression is Always False
- CWE-571: Expression is Always True
Анализатор выдаёт много таких предупреждений, и, к сожалению, большинство из них для кода Android являются ложными. При этом анализатор не виноват. Просто код так написан. Продемонстрирую это на простом примере.
#if GENERIC_TARGET
const char alternative_config_path[] = "/data/nfc/";
#else
const char alternative_config_path[] = "";
#endif
CNxpNfcConfig& CNxpNfcConfig::GetInstance() {
....
if (alternative_config_path[0] != '\0') {
....
}
Здесь анализатор выдаёт предупреждение: V547 CWE-570 Expression 'alternative_config_path[0] != '\0'' is always false. phNxpConfig.cpp 401
Дело в том, что макрос GENERIC_TARGET не определён, и с точки зрения анализатора код выглядит следующим образом:
const char alternative_config_path[] = "";
....
if (alternative_config_path[0] != '\0') {
Анализатор просто обязан выдать предупреждение, так как строка пустая, и по нулевому адресу всегда располагается терминальный нуль. Таким образом, анализатор формально прав, выдавая предупреждение. Однако с практической точки зрения пользы от этого предупреждения нет.
С такими ситуациями, к сожалению, ничего нельзя сделать. Придётся методично просмотреть все подобные предупреждения и разметить многие места как ложные срабатывания, чтобы анализатор в дальнейшем больше не выдавал на эти строки кода сообщения. Это действительно нужно сделать, так как, помимо бессмысленных сообщений, будет найдена масса настоящих дефектов.
Признаюсь честно, что мне было неинтересно внимательно просматривать предупреждения этого типа, и я пробежался по ним поверхностно. Однако даже этого будет достаточно, чтобы показать, что такие диагностики весьма полезны и находят интересные ошибки.
Начать хочется с классической ситуации, когда неверно реализована функция сравнения двух объектов. Почему классической? Это типовой паттерн ошибки, который постоянно встречается нам в разнообразных проектах. Скорее всего, есть три причины его возникновения:
- Функции сравнения просты и пишутся «на автомате» и с использованием технологии Copy-Paste. Человек при написании подобного кода невнимателен и часто допускает опечатки.
- Обычно для таких функций не выполняется code-review, так как лень смотреть простые и скучные функции.
- Для таких функций обычно не делают юнит-тесты. Потому что лень. Вдобавок, функции просты, и программисты не думают, что в них возможны ошибки.
Более подробно эти мысли и примеры изложены в статье «Зло живёт в функциях сравнения».
static inline bool isAudioPlaybackRateEqual(
const AudioPlaybackRate &pr1,
const AudioPlaybackRate &pr2)
{
return fabs(pr1.mSpeed - pr2.mSpeed) <
AUDIO_TIMESTRETCH_SPEED_MIN_DELTA &&
fabs(pr1.mPitch - pr2.mPitch) <
AUDIO_TIMESTRETCH_PITCH_MIN_DELTA &&
pr2.mStretchMode == pr2.mStretchMode &&
pr2.mFallbackMode == pr2.mFallbackMode;
}
Итак, перед нами классическая функция сравнения двух объектов типа AudioPlaybackRate. И, как я думаю, читатель догадывается, что она неправильная. Анализатор PVS-Studio замечает здесь сразу две опечатки:
- V501 CWE-571 There are identical sub-expressions to the left and to the right of the '==' operator: pr2.mStretchMode == pr2.mStretchMode AudioResamplerPublic.h 107
- V501 CWE-571 There are identical sub-expressions to the left and to the right of the '==' operator: pr2.mFallbackMode == pr2.mFallbackMode AudioResamplerPublic.h 108
Поле pr2.mStretchMode и поле pr2.mFallbackMode сравниваются сами с собой. Получается, что функция сравнивает объекты недостаточно точно.
Следующее бессмысленное сравнение живёт в функции, сохраняющей информацию об отпечатке пальца в файл.
static void saveFingerprint(worker_thread_t* listener, int idx) {
....
int ns = fwrite(&listener->secureid[idx],
sizeof(uint64_t), 1, fp);
....
int nf = fwrite(&listener->fingerid[idx],
sizeof(uint64_t), 1, fp);
if (ns != 1 || ns !=1) // <=
ALOGW("Corrupt emulator fingerprints storage; "
"could not save fingerprints");
fclose(fp);
return;
}
Аномалия выявляется в этом коде сразу двумя диагностиками:
- V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator: ns!= 1 || ns!= 1 fingerprint.c 126
- V560 CWE-570 A part of conditional expression is always false: ns!= 1. fingerprint.c 126
Нет обработки ситуации, когда второй вызов функции fwrite не сможет записать данные в файл. Другими словами, не проверяется значение переменной nf. Правильная проверка должна выглядеть так:
if (ns != 1 || nf != 1)
Перейдем к следующей ошибке, связанной с использованием оператора &.
#define O_RDONLY 00000000
#define O_WRONLY 00000001
#define O_RDWR 00000002
static ssize_t verity_read(fec_handle *f, ....)
{
....
/* if we are in read-only mode and expect to read a zero
block, skip reading and just return zeros */
if (f->mode & O_RDONLY && expect_zeros) {
memset(data, 0, FEC_BLOCKSIZE);
goto valid;
}
....
}
Предупреждение PVS-Studio: V560 CWE-570 A part of conditional expression is always false: f→mode & 00000000. fec_read.cpp 322
Обратите внимание, что константа O_RDONLY равна нулю. Это делает выражение f→mode & O_RDONLY бессмысленным, так как оно всегда равно 0. Получается, что условие оператора if никогда не выполняется, и statement-true представляет из себя мёртвый код.
Правильная проверка должна быть такой:
if (f->mode == O_RDONLY && expect_zeros) {
Теперь давайте рассмотрим классическую опечатку, когда забыли написать часть условия.
enum {
....
CHANGE_DISPLAY_INFO = 1 << 2,
....
};
void RotaryEncoderInputMapper::configure(nsecs_t when,
const InputReaderConfiguration* config, uint32_t changes) {
....
if (!changes ||
(InputReaderConfiguration::CHANGE_DISPLAY_INFO)) {
....
}
Предупреждение PVS-Studio: V768 CWE-571 The enumeration constant 'CHANGE_DISPLAY_INFO' is used as a variable of a Boolean-type. InputReader.cpp 3016
Условие всегда истинно, так как операнд InputReaderConfiguration: CHANGE_DISPLAY_INFO представляет собой константу, равную 4.
Если посмотреть, как написан код по соседству, то становится ясно, что на самом деле условие должно быть таким:
if (!changes ||
(changes & InputReaderConfiguration::CHANGE_DISPLAY_INFO)) {
Следующее сравнение, не имеющее смысла, я встретил в операторе цикла.
void parse_printerAttributes(....) {
....
ipp_t *collection = ippGetCollection(attrptr, i);
for (j = 0, attrptr = ippFirstAttribute(collection);
(j < 4) && (attrptr != NULL);
attrptr = ippNextAttribute(collection))
{
if (strcmp("....", ippGetName(attrptr)) == 0) {
....TopMargin = ippGetInteger(attrptr, 0);
} else if (strcmp("....", ippGetName(attrptr)) == 0) {
....BottomMargin = ippGetInteger(attrptr, 0);
} else if (strcmp("....", ippGetName(attrptr)) == 0) {
....LeftMargin = ippGetInteger(attrptr, 0);
} else if (strcmp("....", ippGetName(attrptr)) == 0) {
....RightMargin = ippGetInteger(attrptr, 0);
}
}
....
}
Предупреждение PVS-Studio: V560 CWE-571 A part of conditional expression is always true: (j
Обратите внимание, что значение переменной j нигде не инкрементируется. Это значит, что подвыражение (j < 4) всегда истинно.
Наибольшее количество полезных срабатываний анализатора PVS-Studio, касающихся всегда истинных/ложных условий, относится к коду, где проверяется результат создания объектов с помощью оператора new. Другими словами, анализатор выявляет следующий паттерн кода:
T *p = new T;
if (p == nullptr)
return ERROR;
Такие проверки бессмысленны. Если не удалось выделить память под объект, то генерируется исключение типа std: bad_alloc, и до проверки значения указателя дело просто не дойдёт.
Примечание. Оператор new может вернуть nullptr, если написать new (std: nothrow) T. Однако это не относится к обсуждаемым ошибкам. Анализатор PVS-Studio учитывает (std: nothrow) и не выдаёт предупреждение, если объект создаётся таким способом.
Может показаться, что подобные ошибки безобидны. Ну, подумаешь, лишняя проверка, которая никогда не срабатывает. Всё равно ведь сгенерируется исключение, которое будет где-то обработано. К сожалению, некоторые разработчики размещают в statement-true оператора if действия, освобождающие ресурсы, и т.д. Так как этот код не выполняется, то это может приводить к утечкам памяти и другим ошибкам.
Рассмотрим один из таких случаев, замеченных мною в коде Android.
int parse_apk(const char *path, const char *target_package_name)
{
....
FileMap *dataMap = zip->createEntryFileMap(entry);
if (dataMap == NULL) {
ALOGW("%s: failed to create FileMap\n", __FUNCTION__);
return -1;
}
char *buf = new char[uncompLen];
if (NULL == buf) {
ALOGW("%s: failed to allocate %" PRIu32 " byte\n",
__FUNCTION__, uncompLen);
delete dataMap;
return -1;
}
....
}
Предупреждение PVS-Studio: V668 CWE-570 There is no sense in testing the 'buf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. scan.cpp 213
Обратите внимание, что в случае невозможности выделить второй блок памяти, программист пытается освободить первый блок:
delete dataMap;
Этот код никогда не получит управление. Это мёртвый код. Если возникнет исключение, то произойдёт утечка памяти.
Писать подобный код принципиально неправильно. Для таких случаев придумали умные указатели.
Всего анализатор PVS-Studio обнаружил в коде Android 176 мест, где выполняется проверка указателя после создания объектов с помощью new. Я не стал разбираться, насколько каждое из этих мест опасно, и, конечно, я не стану загромождать статью всеми этими предупреждениями. Желающие могут посмотреть другие предупреждения в файле Android_V668.txt.
Разыменовывание нулевого указателя
Разыменовывание нулевого указателя приводит к неопределённому поведению программы, поэтому такие места полезно найти и исправить. В зависимости от ситуации анализатор PVS-Studio может классифицировать данные ошибки согласно Common Weakness Enumeration следующим образом:
- CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer
- CWE-476: NULL Pointer Dereference
- CWE-628: Function Call with Incorrectly Specified Arguments
- CWE-690: Unchecked Return Value to NULL Pointer Dereference
Такие ошибки я нередко нахожу в коде, отвечающем за обработку нестандартных или некорректных ситуаций. Такой код никто не тестирует, и ошибка может жить в нём очень долго. Сейчас будет рассмотрен как раз такой случай.
bool parseEffect(....) {
....
if (xmlProxyLib == nullptr) {
ALOGE("effectProxy must contain a <%s>: %s",
tag, dump(*xmlProxyLib));
return false;
}
....
}
Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'xmlProxyLib' might take place. EffectsConfig.cpp 205
Если указатель xmlProxyLib равен nullptr, то программист выводит отладочное сообщение, для чего требуется разыменовать этот самый указатель. Упс…
Теперь рассмотрим более интересный вариант ошибки.
static void soinfo_unload_impl(soinfo* root) {
....
soinfo* needed = find_library(si->get_primary_namespace(),
library_name, RTLD_NOLOAD, nullptr, nullptr);
if (needed != nullptr) { // <=
PRINT("warning: couldn't find %s needed by %s on unload.",
library_name, si->get_realpath());
return;
} else if (local_unload_list.contains(needed)) {
return;
} else if (needed->is_linked() && // <=
needed->get_local_group_root() != root) {
external_unload_list.push_back(needed);
} else {
unload_list.push_front(needed);
}
....
}
Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'needed' might take place. linker.cpp 1847
Если указатель needed!= nullptr, то производится вывод предупреждения, что является очень подозрительным поведением программы. Окончательно становится ясно, что код содержит ошибку, если посмотреть ниже и увидеть, что при needed == nullptr произойдёт разыменование нулевого указателя в выражении needed→is_linked ().
Скорее всего, здесь просто перепутаны операторы != и ==. Если провести замену, то код функции обретает смысл, и ошибка исчезает.
Основная масса предупреждений про потенциальное разыменование нулевого указателя относится к ситуации вида:
T *p = (T *) malloc (N);
*p = x;
Такие функции, как malloc, strdup и так далее могут вернуть NULL, если невозможно выделить память. Поэтому нельзя разыменовывать указатели, которые вернули эти функции без предварительной проверки указателя.
Подобных ошибок много, поэтому я приведу только два простых фрагмента кода: первый с malloc и второй с strdup.
DownmixerBufferProvider::DownmixerBufferProvider(....)
{
....
effect_param_t * const param = (effect_param_t *)
malloc(downmixParamSize);
param->psize = sizeof(downmix_params_t);
....
}
Предупреждение PVS-Studio: V522 CWE-690 There might be dereferencing of a potential null pointer 'param'. Check lines: 245, 244. BufferProviders.cpp 245
static char* descriptorClassToDot(const char* str)
{
....
newStr = strdup(lastSlash);
newStr[strlen(lastSlash)-1] = '\0';
....
}
Предупреждение PVS-Studio: V522 CWE-690 There might be dereferencing of a potential null pointer 'newStr'. Check lines: 203, 202. DexDump.cpp 203
Кто-то может сказать, что это несущественные ошибки. Если памяти недостаточно, то программа просто аварийно завершится при разыменовании нулевого указателя, и это нормально. Раз памяти нет, то и нечего пытаться как-то обработать эту ситуацию.
Такой человек неправ. Указатели нужно обязательно проверять! Я подробно разбирал эту тему в статье «Почему важно проверять, что вернула функция malloc». Очень рекомендую ознакомиться с ней всех, кто её ещё не читал.
Если совсем кратко, то опасность состоит в том, что запись в память может произойти не обязательно рядом с нулевым адресом. Можно записать данные куда-то очень далеко в страницу памяти, не защищенную от записи, и тем самым вызвать трудноуловимую ошибку или вообще эту ошибку можно будет начать использовать как уязвимость. Давайте посмотрим, что я имею в виду, на примере функции check_size.
int check_size(radio_metadata_buffer_t **metadata_ptr,
const uint32_t size_int)
{
....
metadata = realloc(metadata,
new_size_int * sizeof(uint32_t));
memmove(
(uint32_t *)metadata + new_size_int - (metadata->count + 1),
(uint32_t *)metadata + metadata->size_int -
(metadata->count + 1),
(metadata->count + 1) * sizeof(uint32_t));
....
}
Предупреждение PVS-Studio: V769 CWE-119 The '(uint32_t *) metadata' pointer in the '(uint32_t *) metadata + new_size_int' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 91, 89. radio_metadata.c 91
Я не разбирался в логике работы функции, но это и не нужно. Главное, что создаётся новый буфер и туда копируются данные. Если функция realloc вернёт NULL, то данные будут скопированы по адресу ((uint32_t *)NULL + metadata→size_int — (metadata→count + 1)).
Если значение metadata→size_int большое, то последствия будут прискорбны. Получается, что данные записываются в случайный участок памяти.
Кстати, есть ещё одна разновидность разыменования нулевого указателя, которую анализатор PVS-Studio классифицирует не как CWE-690, а как CWE-628 (неправильный аргумент).
static void
parse_tcp_ports(const char *portstring, uint16_t *ports)
{
char *buffer;
char *cp;
buffer = strdup(portstring);
if ((cp = strchr(buffer, ':')) == NULL)
....
}
Предупреждение PVS-Studio: V575 CWE-628 The potential null pointer is passed into 'strchr' function. Inspect the first argument. Check lines: 47, 46. libxt_tcp.c 47
Дело в том, что разыменование указателя произойдёт при вызове функции strchr. Поэтому анализатор интерпретирует эту ситуацию как передачу в функцию некорректного значения.
Остальные 194 предупреждения этого типа я привожу списком в файле Android_V522_V575.txt.
Кстати, особую пикантность всем этим ошибкам придают рассмотренные ранее предупреждения про проверку указателя после вызова оператора new. Получается, что есть 195 вызовов функций malloc/realloc/strdup и так далее, когда указатель не проверятся. Зато есть 176 мест, где указатель проверяется после вызова new. Согласитесь, странноватый подход!
Напоследок нам осталось рассмотреть предупреждения V595 и V1004, которые также связаны с использованием нулевых указателей.
V595 выявляет ситуации, когда указатель разыменовывается, а затем проверяется. Синтетический пример:
p->foo();
if (!p) Error();
V1004 выявляет обратные ситуации, когда сначала указатель проверяли, а потом забыли это сделать. Синтетический пример:
if (p) p->foo();
p->doo();
Давайте посмотрим на несколько фрагментов кода Android, где нашлись ошибки этого типа. Специально пояснять их особенности не потребуется.
PV_STATUS RC_UpdateBuffer(VideoEncData *video,
Int currLayer, Int num_skip)
{
rateControl *rc = video->rc[currLayer];
MultiPass *pMP = video->pMP[currLayer];
if (video == NULL || rc == NULL || pMP == NULL)
return PV_FAIL;
....
}
Предупреждение PVS-Studio: V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 385, 388. rate_control.cpp 385
static void resampler_reset(struct resampler_itfe *resampler)
{
struct resampler *rsmp = (struct resampler *)resampler;
rsmp->frames_in = 0;
rsmp->frames_rq = 0;
if (rsmp != NULL && rsmp->speex_resampler != NULL) {
speex_resampler_reset_mem(rsmp->speex_resampler);
}
}
Предупреждение PVS-Studio: V595 CWE-476 The 'rsmp' pointer was utilized before it was verified against nullptr. Check lines: 54, 57. resampler.c 54
void bta_gattc_disc_cmpl(tBTA_GATTC_CLCB* p_clcb,
UNUSED_ATTR tBTA_GATTC_DATA* p_data) {
....
if (p_clcb->status != GATT_SUCCESS) {
if (p_clcb->p_srcb) {
std::vector().swap(
p_clcb->p_srcb->srvc_cache);
}
bta_gattc_cache_reset(p_clcb->p_srcb->server_bda);
} ....
}
Предупреждение PVS-Studio: V1004 CWE-476 The 'p_clcb→p_srcb' pointer was used unsafely after it was verified against nullptr. Check lines: 695, 701. bta_gattc_act.cc 701
Рассматривать другие предупреждения этого типа неинтересно. Среди них есть как ошибки, так и ложные предупреждения, которые возникают из-за плохо или сложно написанного кода.
Я выписал десяток полезных предупреждений:
- V1004 CWE-476 The 'ain' pointer was used unsafely after it was verified against nullptr. Check lines: 101, 105. rsCpuIntrinsicBLAS.cpp 105
- V595 CWE-476 The 'outError' pointer was utilized before it was verified against nullptr. Check lines: 437, 450. Command.cpp 437
- V595 CWE-476 The 'out_last_reference' pointer was utilized before it was verified against nullptr. Check lines: 432, 436. AssetManager2.cpp 432
- V595 CWE-476 The 'set' pointer was utilized before it was verified against nullptr. Check lines: 4524, 4529. ResourceTypes.cpp 4524
- V595 CWE-476 The 'reply' pointer was utilized before it was verified against nullptr. Check lines: 126, 133. Binder.cpp 126
- V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 532, 540. rate_control.cpp 532
- V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 702, 711. rate_control.cpp 702
- V595 CWE-476 The 'pInfo' pointer was utilized before it was verified against nullptr. Check lines: 251, 254. ResolveInfo.cpp 251
- V595 CWE-476 The 'address' pointer was utilized before it was verified against nullptr. Check lines: 53, 55. DeviceHalHidl.cpp 53
- V595 CWE-476 The 'halAddress' pointer was utilized before it was verified against nullptr. Check lines: 55, 82. DeviceHalHidl.cpp 55
А потом мне стало скучно, и я отфильтровал предупреждения этого типа. Поэтому я даже не знаю, сколько настоящих ошибок выявил анализатор. Эти предупреждения ждут своего героя, который внимательно их изучит и внесёт правки в код.
Хочу только обратить внимание моих новых читателей на ошибки вот такого вида:
NJ_EXTERN NJ_INT16 njx_search_word(NJ_CLASS *iwnn, ....) {
....
NJ_PREVIOUS_SELECTION_INFO *prev_info =
&(iwnn->previous_selection);
if (iwnn == NULL) {
return NJ_SET_ERR_VAL(NJ_FUNC_NJ_SEARCH_WORD,
NJ_ERR_PARAM_ENV_NULL);
}
....
}
Предупреждение PVS-Studio: V595 CWE-476 The 'iwnn' pointer was utilized before it was verified against nullptr. Check lines: 686, 689. ndapi.c 686
Некоторые считают, что ошибки здесь нет, так как «нет настоящего разыменования указателя». Просто вычисляется адрес несуществующей переменной. Далее, если указатель iwnn нулевой, то произойдёт выход из функции. Следовательно, ничего плохого не случилось, что мы ранее некорректно вычисляли адрес члена класса.
Нет, так рассуждать нельзя. Этот код приводит к неопределённому поведению, и поэтому так писать нельзя. Неопределённое поведение может проявить себя, например, следующим образом:
- Компилятор видит, что указатель разыменовывается: iwnn→previous_selection
- Разыменовывать нулевой указатель нельзя, ведь это undefined behavior
- Компилятор делает вывод, что указатель iwnn всегда ненулевой
- Компилятор удаляет лишнюю проверку: if (iwnn == NULL)
- Всё, теперь при выполнении программы проверка на нулевой указатель не выполняется, и начинается работа с некорректным указателем на член класса
Более подробно эта тема описана в моей статье «Разыменовывание нулевого указателя приводит к неопределённому поведению».
Приватные данные не затираются в памяти
Рассмотрим серьезный вид потенциальной уязвимости, которая классифицируется согласно Common Weakness Enumeration как CWE-14: Compiler Removal of Code to Clear Buffers.
Если кратко, то суть в следующем: компилятор вправе удалить вызов функции memset, если после этого буфер больше не используется.
Когда я пишу про этот вид уязвимости, обязательно появляются комментарии, что это просто глюк в компиляторе, который надо исправить. Нет, это не глюк. И прежде чем возражать, прошу ознакомиться со следующими материалами:
В общем, всё серьезно. Есть ли такие ошибки в Android? Конечно, есть. Они вообще много где есть: proof:).
Вернёмся к коду Android и рассмотрим начало и конец функции FwdLockGlue_InitializeRoundKeys, так как середина нам неинтересна.
static void FwdLockGlue_InitializeRoundKeys() {
unsigned char keyEncryptionKey[KEY_SIZE];
....
memset(keyEncryptionKey, 0, KEY_SIZE); // Zero out key data.
}
Предупреждение PVS-Studio: V597 CWE-14 The compiler could delete the 'memset' function call, which is used to flush 'keyEncryptionKey' buffer. The memset_s () function should be used to erase the private data. FwdLockGlue.c 102
Массив keyEncryptionKey создаётся на стеке и хранит приватную информацию. В конце функции этот массив хотят заполнить нулями, чтобы он случайно не попал куда не следует. Как информация может попасть туда, куда не следует, расскажет статья «Перезаписывать память — зачем?».
Для заполнения массива с приватной информацией нулями используется функция memset. Комментарий «Zero out key data» подтверждает, что мы всё понимаем правильно.
Беда в том, что с очень большой вероятностью компилятор при сборке release-версии удалит вызов функции memset. Раз буфер после вызова memset не используется, то и сам вызов функции memset с точки зрения компилятора является лишним.
Ещё 10 предупреждений я выписал в файл Android_V597.txt.
Нашлась ещё одна ошибка, где память не затирается, хотя в этом случае функция memset ни при чём.
void SHA1Transform(uint32_t state[5], const uint8_t buffer[64])
{
uint32_t a, b, c, d, e;
....
/* Wipe variables */
a = b = c = d = e = 0;
}
Предупреждение PVS-Studio: V1001 CWE-563 The 'a' variable is assigned but is not used until the end of the function. sha1.c 213
PVS-Studio выявил аномалию, связанную с тем, что после присваивания переменным значений, они больше не используются. Анализатор классифицировал этот дефект как CWE-563: Assignment to Variable without Use. И, формально, он прав, хотя, на самом деле, здесь мы опять имеем дело с CWE-14. Компилятор выбросит эти присваивания, так как с точки зрения языка C и C++ они лишние. В результате в стеке останутся прежние значения переменных a, b, c, d и e, хранящих приватные данные.
Unspecified/implementation-defined behavior
Пока вы ещё не устали, давайте рассмотрим сложный случай, который потребует обстоятельного описания с моей стороны.
typedef int32_t GGLfixed;
GGLfixed gglFastDivx(GGLfixed n, GGLfixed d)
{
if ((d>>24) && ((d>>24)+1)) {
n >>= 8;
d >>= 8;
}
return gglMulx(n, gglRecip(d));
}
Предупреждение PVS-Studio: V793 It is odd that the result of the '(d >> 24) + 1' statement is a part of the condition. Perhaps, this statement should have been compared with something else. fixed.cpp 75
Программист хотел проверить, что 8 старших бит переменной d содержат единицы, но при этом не все биты сразу. Другими словами, программист хотел проверить, что в старшем байте находится любое значение, отличное от 0×00 и 0xFF.
Подошёл он к решению этой задачи излишне творчески. Для начала он проверил, что старшие биты ненулевые, написав выражение (d>>24). К этому выражению есть претензии, но интереснее разобрать правую часть выражения: ((d>>24)+1). Программист сдвигает старшие восемь бит в младший байт. При этом он рассчитывает, что самый старший знаковый бит дублируется во всех остальных битах. Т.е. если переменная d равна 0b11111111'00000000'00000000'00000000, то после сдвига получится значение 0b11111111'11111111'11111111'11111111. Прибавив 1 к значению 0xFFFFFFFF типа int, программист планирует получить 0. То есть: -1+1=0. Таким образом, выражением ((d>>24)+1) он проверяет, что не все старшие восемь бит равны 1. Я понимаю, что это довольно сложно, поэтому прошу не торопиться и попробовать разобраться, как и что работает :).
Теперь давайте разберём, что не так с этим кодом.
При сдвиге старший знаковый бит вовсе не обязательно «размазывается». Вот что написано про это в стандарте: «The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined».
Нам важно самое последнее предложение. Итак, мы повстречали поведение, определяемое реализацией (implementation-defined behavior). Как будет работать этот код, зависит от архитектуры микропроцессора и реализации компилятора. После сдвига в старших битах вполне могут оказаться нули, и тогда выражение ((d>>24)+1) всегда будет отлично от 0, т.е. будет всегда истинным значением.
Отсюда вывод: не надо зря мудрить. Код станет надёжней и понятней, если написать, например, так:
GGLfixed gglFastDivx(GGLfixed n, GGLfixed d)
{
uint32_t hibits = static_cast(d) >> 24;
if (hibits != 0x00 && hibits != 0xFF) {
n >>= 8;
d >>= 8;
}
return gglMulx(n, gglRecip(d));
}
Наверное, я предложил не идеальный вариант, но в этом коде нет поведения, определяемого реализацией, и читателю будет легче понять, что проверяется.
Вы заслужили чашечку кофе или чая. Отвлекитесь, и продолжим: нас ждёт интересный случай неуточненного поведения.
На собеседовании в качестве одного из первых вопросов соискателю я задаю следующий: «Что напечатает функция printf и почему?»
int i = 5;
printf("%d,%d", ++i, ++i)
Правильный ответ: это неуточненное поведение. Порядок вычисления фактических аргументов при вызове функции не определён. Изредка я даже демонстрирую, что этот код, собранный с помощью Visual C++, выводит на экран »7,6», чем ставлю в полный тупик новичков, слабых знанием и духом :).
Может показаться, что это надуманная проблема. Но нет, подобный код можно встретить и в серьезных приложениях, например, в коде Android.
bool ComposerClient::CommandReader::parseSetLayerCursorPosition(
uint16_t length)
{
if (length != CommandWriterBase::kSetLayerCursorPositionLength) {
return false;
}
auto err =
mHal.setLayerCursorPosition(mDisplay, mLayer,
readSigned(), readSigned());
if (err != Error::NONE) {
mWriter.setError(getCommandLoc(), err);
}
return true;
}
Предупреждение PVS-Studio: V681 CWE-758 The language standard does not define an order in which the 'readSigned' functions will be called during evaluation of arguments. ComposerClient.cpp 836
Нам интереса вот эта строка кода:
mHal.setLayerCursorPosition(...., readSigned(), readSigned());
С помощью вызова функции readSigned читаются два значения. Но вот в какой последовательности будут прочитаны значения, предсказать невозможно. Это классический случай Unspecified Behavior.
Польза от использования статического анализатора кода
Вся эта статья популяризирует статический анализ кода в целом и наш инструмент PVS-Studio в частности. Однако некоторые ошибки просто идеальны для демонстрации возможностей статического анализа. Их сложно выявить обзорами кода, и только не устающая программа легко замечает их. Рассмотрим парочку таких случаев.
const std::map kBootReasonMap = {
....
{"watchdog_sdi_apps_reset", 106},
{"smpl", 107},
{"oem_modem_failed_to_powerup", 108},
{"reboot_normal", 109},
{"oem_lpass_cfg", 110}, // <=
{"oem_xpu_ns_error", 111}, // <=
{"power_key_press", 112},
{"hardware_reset", 113},
{"reboot_by_powerkey", 114},
{"reboot_verity", 115},
{"oem_rpm_undef_error", 116},
{"oem_crash_on_the_lk", 117},
{"oem_rpm_reset", 118},
{"oem_lpass_cfg", 119}, // <=
{"oem_xpu_ns_error", 120}, // <=
{"factory_cable", 121},
{"oem_ar6320_failed_to_powerup", 122},
{"watchdog_rpm_bite", 123},
{"power_on_cable", 124},
{"reboot_unknown", 125},
....
};
Предупреждения PVS-Studio:
- V766 CWE-462 An item with the same key '«oem_lpass_cfg»' has already been added. bootstat.cpp 264
- V766 CWE-462 An item with the same key '«oem_xpu_ns_error»' has already been added. bootstat.cpp 265
В отсортированный ассоциативный контейнер (std: map) вставляются разные значения с одинаковыми ключами. С точки зрения Common Weakness Enumeration — это CWE-462: Duplicate Key in Associative List.
Текст программы сокращен, и ошибки помечены комментариями, поэтому ошибка кажется очевидной, но когда просто читаешь такой код глазами, найти такие ошибки очень сложно.
Рассмотрим ещё один фрагмент кода, который очень сложен для восприятия, так как однотипен и неинтересен.
MtpResponseCode MyMtpDatabase::getDevicePropertyValue(....) {
....
switch (type) {
case MTP_TYPE_INT8:
packet.putInt8(longValue);
break;
case MTP_TYPE_UINT8:
packet.putUInt8(longValue);
break;
case MTP_TYPE_INT16:
packet.putInt16(longValue);
break;
case MTP_TYPE_UINT16:
packet.putUInt16(longValue);
break;
case MTP_TYPE_INT32:
packet.putInt32(longValue);
break;
case MTP_TYPE_UINT32:
packet.putUInt32(longValue);
break;
case MTP_TYPE_INT64:
packet.putInt64(longValue);
break;
case MTP_TYPE_UINT64:
packet.putUInt64(longValue);
break;
case MTP_TYPE_INT128:
packet.putInt128(longValue);
break;
case MTP_TYPE_UINT128:
packet.putInt128(longValue); // <=
break;
....
}
Предупреждение PVS-Studio: V525 CWE-682 The code contains the collection of similar blocks. Check items 'putInt8', 'putUInt8', 'putInt16', 'putUInt16', 'putInt32', 'putUInt32', 'putInt64', 'putUInt64', 'putInt128', 'putInt128' in lines 620, 623, 626, 629, 632, 635, 638, 641, 644, 647. android_mtp_MtpDatabase.cpp 620
В случае MTP_TYPE_UINT128 должна была быть вызвана функция putUInt128, а не putInt128.
И последний в этом разделе шикарный пример неудачного Copy-Paste.
static void btif_rc_upstreams_evt(....)
{
....
case AVRC_PDU_REQUEST_CONTINUATION_RSP: {
BTIF_TRACE_EVENT(
"%s() REQUEST CONTINUATION: target_pdu: 0x%02d",
__func__, pavrc_cmd->continu.target_pdu);
tAVRC_RESPONSE avrc_rsp;
if (p_dev->rc_connected == TRUE) {
memset(&(avrc_rsp.continu), 0, sizeof(tAVRC_NEXT_RSP));
avrc_rsp.continu.opcode =
opcode_from_pdu(AVRC_PDU_REQUEST_CONTINUATION_RSP);
avrc_rsp.continu.pdu = AVRC_PDU_REQUEST_CONTINUATION_RSP;
avrc_rsp.continu.status = AVRC_STS_NO_ERROR;
avrc_rsp.continu.target_pdu = pavrc_cmd->continu.target_pdu;
send_metamsg_rsp(p_dev, -1, label, ctype, &avrc_rsp);
}
} break;
case AVRC_PDU_ABORT_CONTINUATION_RSP: {
BTIF_TRACE_EVENT(
"%s() ABORT CONTINUATION: target_pdu: 0x%02d", __func__,
pavrc_cmd->abort.target_pdu);
tAVRC_RESPONSE avrc_rsp;
if (p_dev->rc_connected == TRUE) {
memset(&(avrc_rsp.abort), 0, sizeof(tAVRC_NEXT_RSP));
avrc_rsp.abort.opcode =
opcode_from_pdu(AVRC_PDU_ABORT_CONTINUATION_RSP);
avrc_rsp.abort.pdu = AVRC_PDU_ABORT_CONTINUATION_RSP;
avrc_rsp.abort.status = AVRC_STS_NO_ERROR;
avrc_rsp.abort.target_pdu = pavrc_cmd->continu.target_pdu;
send_metamsg_rsp(p_dev, -1, label, ctype, &avrc_rsp);
}
}
break;
....
}
Прежде чем прочитать предупреждение анализатора и дальнейший текст, предлагаю поискать ошибку самостоятельно.
Чтобы вы сразу случайно не прочитали ответ, вот вам картинка для отвлечения внимания. Если Вас заинтересовало, что означает яйцо с надписью Java, то вам сюда.
Итак, надеюсь, вы получили удовольствие от поиска опечатки. Теперь время привести предупреждение анализатора: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'abort' variable should be used instead of 'continu'. btif_rc.cc 1554
Видимо, код писался методом Copy-Paste, и человек, как всегда, не смог быть внимательным в процессе правки скопированного фрагмента к
