Сравниваем реализацию языков Python и Ruby по плотности ошибок
Введение
Для анализа были взяты самые свежие исходники из основных репозиториев (Ruby, Python). Проверка проводилась с помощью статического анализатора кода PVS-Studio версии 6.06. Python отлично собирается в Visual Studio, а для Ruby можно использовать Standalone версию в режиме мониторинга вызова компилятора.
В проектах нашлось не так уж много откровенных ошибок: большая часть срабатываний связана с использованием макросов, которые раскрываются в чрезвычайно подозрительный код с точки зрения анализатора, но безобидный с точки зрения разработчика. Можно долго дискутировать на тему того, являются ли макросы злом или нет, но можно сказать точно, что анализатору они не нравятся. Для того чтобы избавиться от какого-нибудь надоедливого макроса, существует опция подавления ложных срабатываний. Достаточно написать:
//-V:RB_TYPE_P:501И все срабатывания диагностики V501, в которых фигурирует макрос RB_TYPE_P пропадут.
Python
Фрагмент N1
#ifdef MS_WINDOWS
typedef SOCKET SOCKET_T;
#else
typedef int SOCKET_T;
#endif
typedef struct {
PyObject_HEAD
SOCKET_T sock_fd; /* Socket file descriptor */
....
} PySocketSockObject;
static int
internal_select(PySocketSockObject *s,
int writing,
_PyTime_t interval,
int connect)
{
....
if (s->sock_fd < 0) // <=
return 0;
....
}V547 Expression 's→sock_fd < 0' is always false. Unsigned type value is never < 0. socketmodule.c 655
Тип SOCKET в Windows является беззнаковым, поэтому сравнение его с нулём бессмысленно. Для того, чтобы проверить, вернула ли функция socket () корректный дескриптор, необходимо сравнить его значение с INVALID_SOCKET. Стоит отметить, что в Linux данное сравнение работало бы правильно, так как там в качестве типа сокета используется знаковый int и об ошибке сигнализирует значение -1. Тем не менее, для проверки лучше использовать специальные макросы или константы.
Аналогичные проверки, на которые было выдано предупреждение:
- V547 Expression 's→sock_fd < 0' is always false. Unsigned type value is never < 0. _ssl.c 1702
- V547 Expression 'sock→sock_fd < 0' is always false. Unsigned type value is never < 0. _ssl.c 2018
Фрагмент N2
int ASN1_PRINTABLE_type(const unsigned char *s, int len)
{
int c;
int ia5 = 0;
....
if (!(((c >= 'a') && (c <= 'z')) ||
((c >= 'A') && (c <= 'Z')) ||
(c == ' ') || // <=
((c >= '0') && (c <= '9')) ||
(c == ' ') || (c == '\'') || // <=
(c == '(') || (c == ')') ||
(c == '+') || (c == ',') ||
(c == '-') || (c == '.') ||
(c == '/') || (c == ':') ||
(c == '=') || (c == '?')))
ia5 = 1;
....
}V501 There are identical sub-expressions '(c == ' ')' to the left and to the right of the '||' operator. a_print.c 77
Типичный пример ошибки, возникшей в результате Copy-Paste. Нередко, при большом количестве скопированных блоков, программист теряет внимание и забывает поменять одну переменную или константу в одном их них. Так и в этом случае в большом условном выражении перепутали значения, с которыми сравнивается переменная c. Точно сказать нельзя, но похоже на то, что забыли символ двойных кавычек '»'.
Фрагмент N3
static PyObject *
semlock_acquire(SemLockObject *self, PyObject *args, PyObject *kwds)
{
....
HANDLE handles[2], sigint_event;
....
/* prepare list of handles */
nhandles = 0;
handles[nhandles++] = self->handle;
if (_PyOS_IsMainThread()) {
sigint_event = _PyOS_SigintEvent();
assert(sigint_event != NULL);
handles[nhandles++] = sigint_event;
}
/* do the wait */
Py_BEGIN_ALLOW_THREADS
if (sigint_event != NULL) //<=
ResetEvent(sigint_event);
....
}V614 Potentially uninitialized pointer 'sigint_event' used. semaphore.c 120
В случае, когда функция _PyOS_IsMainThread () вернёт false, указатель sigint_event останется неинициализированным. Это приведёт к неопределённому поведению. Такую ошибку можно легко проглядеть в отладочной версии, где указатель, скорее всего, будет инициализирован нулём.
Фрагмент N4
#define BN_MASK2 (0xffffffffffffffffLL)
int BN_mask_bits(BIGNUM *a, int n)
{
....
a->d[w] &= ~(BN_MASK2 << b); //<=
....
}V610 Undefined behavior. Check the shift operator '<<'. The left operand '(0xffffffffffffffffLL)' is negative. bn_lib.c 796
Несмотря на то, что код в большинстве случаев работает, данное выражение по стандарту является неопределённым поведением. Подробнее о сдвигах отрицательных чисел вы можете прочитать в статье Андрея Карпова «Не зная брода, не лезь в воду. Часть третья». Стоит ли избегать конструкций, результат которых не гарантирован стандартом, решать вам, но лучше от такого отказаться, о чём нас анализатор и предупреждает.
static PyObject *
binascii_b2a_qp_impl(PyModuleDef *module,
Py_buffer *data,
int quotetabs,
int istext,
int header)
{
Py_ssize_t in, out;
const unsigned char *databuf;
....
if ((databuf[in] > 126) ||
(databuf[in] == '=') ||
(header && databuf[in] == '_') ||
((databuf[in] == '.') && (linelen == 0) &&
(databuf[in+1] == '\n' || databuf[in+1] == '\r' ||
databuf[in+1] == 0)) ||
(!istext && ((databuf[in] == '\r') ||
(databuf[in] == '\n'))) ||
((databuf[in] == '\t' || databuf[in] == ' ') &&
(in + 1 == datalen)) ||
((databuf[in] < 33) &&
(databuf[in] != '\r') && (databuf[in] != '\n') &&
(quotetabs ||
(!quotetabs && ((databuf[in] != '\t') && // <=
(databuf[in] != ' '))))))
{
....
}
....
}V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'quotetabs' and '! quotetabs'. binascii.c 1453
Данный фрагмент не является ошибочным, тем не менее, стоит рассмотреть его отдельно. Предупреждение носит во многом рекомендательный характер: выражение вида 'A || (! A && B)' можно и нужно упростить до 'A || B': это немного облегчит чтение и без того переусложнённого кода.
Аналогичные предупреждения:
- V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '! type' and 'type'. digest.c 167
- V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '! cipher' and 'cipher'. evp_enc.c 120
Фрагмент N5
static int dh_cms_set_peerkey(....)
{
....
int atype;
....
/* Only absent parameters allowed in RFC XXXX */
if (atype != V_ASN1_UNDEF && atype == V_ASN1_NULL)
goto err;
....
}V590 Consider inspecting the 'atype!= — 1 && atype == 5' expression. The expression is excessive or contains a misprint. dh_ameth.c 670
Как ни странно, ошибки в логических выражениях очень часто встречаются даже в больших проектах. Логическое выражение из этого фрагмента является избыточным: его можно упростить до 'atype == V_ASN1_NULL'. Скорее всего, исходя из контекста, ошибки здесь нет, но выглядит такой код подозрительно.
Фрагмент N6
static void cms_env_set_version(CMS_EnvelopedData *env)
{
....
if (env->originatorInfo || env->unprotectedAttrs)
env->version = 2;
env->version = 0;
}V519 The 'env→version' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 907, 908. cms_env.c 908
Сложно сказать, что изначально подразумевалось автором этого кода. Возможно, здесь пропущен else. Сейчас смысла в if нет, так как значение переменой 'env→version' будет в любом случае перезаписано.
int
_PyState_AddModule(PyObject* module, struct PyModuleDef* def)
{
PyInterpreterState *state;
if (def->m_slots) {
PyErr_SetString(PyExc_SystemError,
"PyState_AddModule called on module with slots");
return -1;
}
state = GET_INTERP_STATE();
if (!def)
return -1;
....
}V595 The 'self→extra' pointer was utilized before it was verified against nullptr. Check lines: 917, 923. _elementtree.c 917
Традиционная ошибка, связанная с разыменовыванием нулевого указателя, которая встречается практически в каждом проекте. Сначала в выражении 'def→m_slots' обратились по какому-то адресу, а потом оказалось, что этот адрес мог быть нулевым. В итоге проверка на nullptr не работает, так как произойдет разыменование нулевого указателя, что приведёт к неопределённому поведению программы, например к её падению.
Ruby
Фрагмент N1
static void
vm_set_main_stack(rb_thread_t *th, const rb_iseq_t *iseq)
{
VALUE toplevel_binding = rb_const_get(rb_cObject,
rb_intern("TOPLEVEL_BINDING"));
rb_binding_t *bind;
rb_env_t *env;
GetBindingPtr(toplevel_binding, bind);
GetEnvPtr(bind->env, env);
vm_set_eval_stack(th, iseq, 0, &env->block);
/* save binding */
if (bind && iseq->body->local_size > 0) {
bind->env = vm_make_env_object(th, th->cfp);
}
}V595 The 'bind' pointer was utilized before it was verified against nullptr. Check lines: 377, 382. vm.c 377
Не избежали похожей ошибки и в проекте Ruby. Проверка 'if (bind)' не спасёт от беды, так как bind был разыменован выше по коду. Всего таких предупреждений в Ruby нашлось больше 30, приводить их все нет смысла.
Фрагмент N2
static int
code_page_i(....)
{
table = realloc(table, count * sizeof(*table));
if (!table) return ST_CONTINUE;
....
}V701 realloc () possible leak: when realloc () fails in allocating memory, original pointer 'table' is lost. Consider assigning realloc () to a temporary pointer. file.c 169
В этом участке кода значение realloc сохраняется в ту же переменную, которая используется в качестве аргумента. В случае, когда realloc вернёт nullptr, исходное значение указателя будет утеряно, что приведёт к утечке памяти.
Фрагмент N3
static int
w32_symlink(UINT cp, const char *src, const char *link)
{
....
BOOLEAN ret;
typedef DWORD (WINAPI *create_symbolic_link_func)
(WCHAR*, WCHAR*, DWORD);
static create_symbolic_link_func create_symbolic_link =
(create_symbolic_link_func)-1;
....
ret = create_symbolic_link(wlink, wsrc, flag);
ALLOCV_END(buf);
if (!ret) {
int e = GetLastError();
errno = map_errno(e);
return -1;
}
return 0;
}V724 Converting type 'DWORD' to type 'BOOLEAN' can lead to a loss of high-order bits. Non-zero value can become 'FALSE'. win32.c 4974
Тип BOOLEAN используется в WinAPI в качестве логического типа. Он объявлен следующим образом:
typedef unsigned char BYTE;
typedef BYTE BOOLEAN;DWORD является 32-битным беззнаковым числом. Поэтому, если привести, например, значение DWORD 0xffffff00 к BOOLEAN (или любое другое, у которого младший бит равен нулю), то оно станет равно 0, то есть FALSE.
Фрагмент N4
static VALUE
rb_str_split_m(int argc, VALUE *argv, VALUE str)
{
....
char *ptr = RSTRING_PTR(str);
long len = RSTRING_LEN(str);
long start = beg;
....
if (ptr+start == ptr+len)
start++;
....
}V584 The 'ptr' value is present on both sides of the '==' operator. The expression is incorrect or it can be simplified. string.c 7211
В обеих частях сравнения присутствует прибавление ptr, следовательно, его можно опустить:
if (start == len)Скорее всего в этом фрагменте нет ошибки. Тем не менее, часто в таких выражениях на самом деле хотят сравнить две разные переменные. Поэтому лучше обращать внимания на подобные сравнения.
Итоги
Проанализировав все выданные предупреждения диагностик общего назначения и убрав все ложные срабатывания, я пришёл к следующему распределению ошибок:
Большая часть срабатываний в Ruby пришлась на предупреждение V610 (369 срабатываний!), но даже если их исключить, то ситуация не изменится: по количеству подозрительных мест Python опережает своего конкурента.
Наиболее частой оказалась диагностика V595: в коде Python она встретилась 17 раз, в коде Ruby — 37.
Но интереснее всего соотношение плотности ошибок. По этой характеристике Python также однозначно побеждает. С результатами расчётов можно ознакомиться в таблице:
Может показаться, что ошибок многовато. Это не так. Во-первых, не все из найденных ошибок критичны. Например, упомянутая ранее диагностика V610, выявляет ошибки с точки зрения языка С++. Однако на практике для используемого набора компиляторов результат может быть всегда правильным. Хотя от этого ошибки не перестают быть ошибками, они никак в данный момент не сказываются на работе программы. Во-вторых, нужно учитывать размер проверенного кода. Поэтому можно говорить о высоком качестве этих проектов. Пока эта оценка субъективна, так как ранее мы не вычисляли плотность ошибок у проверенных проектов. Но постараемся это делать в дальнейшем, чтобы впоследствии можно было делать сравнения.
Заключение
Языки Python и Ruby очень популярны: на них пишут миллионы разработчиков со всего мира. При такой активности проектов и комьюнити, хорошем качестве кода, отличном тестировании и использовании статического анализа (оба проекта проверяются с помощью Coverity) сложно найти большое количество ошибок. Тем не менее, PVS-Studio удалось найти несколько подозрительных мест. Но стоит понимать, что именно регулярная проверка кода может серьёзно облегчить жизнь разработчикам. Лучше всего править ошибку сразу до того, как изменения попадут в репозиторий и в релиз — статический анализатор в этом может помочь, как никто другой.
Предлагаю всем желающим попробовать PVS-Studio на своих проектах.
