Для пополнения списка языков программирования с открытым исходным кодом, которые были проверены с помощью статического анализатора кода PVS-Studio, был выбран Perl 5. Эта статья о найденных ошибках и трудностях просмотра результатов анализа. Количество макросов в коде столь велико, что создаётся ощущение, что код написан не на языке Си, а на каком-то его странном диалекте. Несмотря на затруднения при просмотре кода, удалось насобирать интересные проблемы, о которых и будет рассказано в этой статье.
Perl - высокоуровневый интерпретируемый динамический язык программирования общего назначения (Perl is a family of two high-level, general-purpose, interpreted, dynamic programming languages). Разработка Perl 5 была начата в 1994 году. Спустя пару десятилетий, код на языке Си с многочисленными макросами вызывает нервозность у современных программистов.
Исходный код Perl 5 был взят из официального репозитория (ветка blead). Для проверки проекта использовался статический анализатор кода PVS-Studio. Анализ проводился на операционной системе Linux, но анализатор также доступен для Windows и macOS.
Просмотр результатов анализа был не простой задачей. Дело в том, что анализатор проверяет препроцессированные .i файлы, в которых уже раскрыты все директивы препроцессора, а выдаёт предупреждения на файлы с исходным кодом. Это правильное поведение анализатора, ничего менять не нужно, но много предупреждений выдаётся на макросы! А за макросами скрывается нечитабельный код.
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. toke.c 9494
STATIC char *
S_scan_ident(pTHX_ char *s, char *dest, STRLEN destlen, I32 ck_uni)
{
....
if ((s <= PL_bufend - (is_utf8)
? UTF8SKIP(s)
: 1)
&& VALID_LEN_ONE_IDENT(s, PL_bufend, is_utf8))
{
....
}
....
}Начнём обзор с красивой ошибки. Каждые несколько обзоров кода приходится повторять, что тернарный оператор имеет чуть ли не самый низкий приоритет в вычислениях.
Рассмотрим фрагмент с ошибкой:
s <= PL_bufend - (is_utf8) ? UTF8SKIP(s) : 1Порядок операций, который ожидает программист:
Что происходит на самом деле:
Держите табличку с приоритетами операций: "Приоритет операций в языке Си/Си++".
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. re_exec.c 9193
STATIC I32
S_regrepeat(pTHX_ regexp *prog, char **startposp, const regnode *p,
regmatch_info *const reginfo, I32 max _pDEPTH)
{
....
assert(STR_LEN(p) == reginfo->is_utf8_pat ? UTF8SKIP(STRING(p)) : 1);
....
}Простой код с похожей ошибкой. Но если не знать приоритеты операций, то допустить ошибку можно в выражении любого размера.
Ещё одно место с assert'ом:
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '&&' operator. pp_hot.c 3036
PP(pp_match)
{
....
MgBYTEPOS_set(mg, TARG, truebase, RXp_OFFS(prog)[0].end);
....
}А вот и предупреждение на макрос... Чтобы понять, что там происходит, не поможет даже реализация макроса, потому что в нём используется ещё несколько макросов!
Поэтому прикладываю фрагмент препроцессированного файла для этой строки кода:
(((targ)->sv_flags & 0x00000400) && (!((targ)->sv_flags & 0x00200000) ||
S_sv_only_taint_gmagic(targ)) ? (mg)->mg_len = ((prog->offs)[0].end),
(mg)->mg_flags |= 0x40 : ((mg)->mg_len = (((targ)->sv_flags & 0x20000000)
&& !__builtin_expect(((((PL_curcop)->cop_hints + 0) & 0x00000008) ?
(_Bool)1 :(_Bool)0),(0))) ? (ssize_t)Perl_utf8_length( (U8 *)(truebase),
(U8 *)(truebase)+((prog->offs)[0].end)) : (ssize_t)((prog->offs)[0].end),
(mg)->mg_flags &= ~0x40));Где-то здесь анализатор усомнился в правильном использовании тернарного оператора (их 3), но я не нашёл в себе сил понять, что делается в этом коде. Мы уже убедились, что разработчики допускают такие ошибки, поэтому тут она тоже может быть с большой вероятностью.
Ещё три использования этого макроса:
Примечание коллеги Андрея Карпова. Я 10 минут медитировал над этим кодом и склоняюсь к мнению, что ошибки здесь нет. Но в любом случае, крайне мучительно читать такой код, и лучше так не писать.
V523 The 'then' statement is equivalent to the 'else' statement. toke.c 12056
static U8 *
S_add_utf16_textfilter(pTHX_ U8 *const s, bool reversed)
{
....
SvCUR_set(PL_linestr, 0);
if (FILTER_READ(0, PL_linestr, 0)) {
SvUTF8_on(PL_linestr);
} else {
SvUTF8_on(PL_linestr);
}
PL_bufend = SvEND(PL_linestr);
return (U8*)SvPVX(PL_linestr);
}Я думаю, можно обойтись без изучения содержимого макросов, чтобы убедиться в наличии подозрительно продублированных фрагментов кода.
V564 The '|' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '||' operator. op.c 11494
OP *
Perl_ck_rvconst(pTHX_ OP *o)
{
....
gv = gv_fetchsv(kidsv,
o->op_type == OP_RV2CV
&& o->op_private & OPpMAY_RETURN_CONSTANT
? GV_NOEXPAND
: iscv | !(kid->op_private & OPpCONST_ENTERED), iscv // <=
? SVt_PVCV
: o->op_type == OP_RV2SV
? SVt_PV
: o->op_type == OP_RV2AV
? SVt_PVAV
: o->op_type == OP_RV2HV
? SVt_PVHV
: SVt_PVGV);
....
}Очень странный код. Выражение "iscv | !(kid->op_private & OPpCONST_ENTERED)" никак не используется. Здесь явно есть какая-то опечатка. Например, возможно, здесь следовало написать:
: iscv = !(kid->op_private & OPpCONST_ENTERED), iscv // <=V547 Expression 'RETVAL == 0' is always true. Typemap.c 710
XS_EUPXS(XS_XS__Typemap_T_SYSRET_pass);
XS_EUPXS(XS_XS__Typemap_T_SYSRET_pass)
{
dVAR; dXSARGS;
if (items != 0)
croak_xs_usage(cv, "");
{
SysRet RETVAL;
#line 370 "Typemap.xs"
RETVAL = 0;
#line 706 "Typemap.c"
{
SV * RETVALSV;
RETVALSV = sv_newmortal();
if (RETVAL != -1) { // <=
if (RETVAL == 0) // <=
sv_setpvn(RETVALSV, "0 but true", 10);
else
sv_setiv(RETVALSV, (IV)RETVAL);
}
ST(0) = RETVALSV;
}
}
XSRETURN(1);
}Переменная RETVAL проверяется два раза подряд. При этом, из кода видно, что эта переменная всегда равна нулю. Возможно, в одном или обоих условиях хотели проверить указатель RETVALSV, но допустили опечатку.
В анализаторе есть несколько типов диагностических правил, которые ищут ошибки с использованием оператора sizeof. На проекте Perl 5 две таких диагностики суммарно выдали около тысячи предупреждений. В этом случае виноват не анализатор, а макросы.
V568 It's odd that the argument of sizeof() operator is the 'len + 1' expression. util.c 1084
char *
Perl_savepvn(pTHX_ const char *pv, I32 len)
{
....
Newx(newaddr,len+1,char);
....
}В коде много подобных макросов. Я выбрал один для примера, нам интересен аргумент "len + 1".
Препроцессором макрос раскрывается в следующий код:
(newaddr = ((void)(__builtin_expect(((((( sizeof(size_t) < sizeof(len+1) ||
sizeof(char) > ((size_t)1 << 8*(sizeof(size_t) - sizeof(len+1)))) ?
(size_t)(len+1) : ((size_t)-1)/sizeof(char)) > ((size_t)-1)/sizeof(char))) ?
(_Bool)1 : (_Bool)0),(0)) && (S_croak_memory_wrap(),0)),
(char*)(Perl_safesysmalloc((size_t)((len+1)*sizeof(char))))));Предупреждение анализатора выдаётся на конструкцию sizeof(len +1). Дело в том, что никакие вычисления в аргументах оператора sizeof не выполняются. В подобный код раскрываются многие макросы. Наверное, это старый legacy-код, в котором уже никто не хочет что-то трогать, но текущие разработчики продолжают использовать старые макросы, предполагая об ином их поведении.
V522 Dereferencing of the null pointer 'sv' might take place. pp_ctl.c 577
OP * Perl_pp_formline(void)
{
....
SV *sv = ((void *)0);
....
switch (*fpc++) {
....
case 4:
arg = *fpc++;
f += arg;
fieldsize = arg;
if (mark < sp)
sv = *++mark;
else {
sv = &(PL_sv_immortals[2]);
Perl_ck_warner( (28 ), "....");
}
....
break;
case 5:
{
const char *s = item = ((((sv)->sv_flags & (....)) == 0x00000400) ? ....
....
}
....
}Этот фрагмент кода полностью взят из препроцессированного файла, потому что по исходному файлу убедиться в наличии проблемы невозможно, опять же из-за макросов.
Указатель sv при объявлении инициализируется нулём. Анализатор обнаружил, что при переходе по значению 5 в операторе switch происходит разыменование этого указателя, который так и не был перед этим инициализирован. Изменение указателя sv присутствует в ветви со значением 4, но в конце этого блока кода находится оператор break. Скорее всего, в этом месте требуется написание дополнительного кода.
V595 The 'k' pointer was utilized before it was verified against nullptr. Check lines: 15919, 15920. op.c 15919
void
Perl_rpeep(pTHX_ OP *o)
{
....
OP *k = o->op_next;
U8 want = (k->op_flags & OPf_WANT); // <=
if ( k // <=
&& k->op_type == OP_KEYS
&& ( want == OPf_WANT_VOID
|| want == OPf_WANT_SCALAR)
&& !(k->op_private & OPpMAYBE_LVSUB)
&& !(k->op_flags & OPf_MOD)
) {
....
}В этом фрагменте кода анализатор обнаружил указатель k, который разыменовывается на одну строку раньше, чем выполняется его проверка на валидность. Это может быть как ошибкой, так и лишним кодом.
Диагностика V595 находит очень много предупреждений в любом проекте, Perl 5 не исключение. В статью всё это уместить невозможно, поэтому ограничимся одним примером, а разработчики при желании проверят проект самостоятельно.
V779 Unreachable code detected. It is possible that an error is present. universal.c 457
XS(XS_utf8_valid);
XS(XS_utf8_valid)
{
dXSARGS;
if (items != 1)
croak_xs_usage(cv, "sv");
else {
SV * const sv = ST(0);
STRLEN len;
const char * const s = SvPV_const(sv,len);
if (!SvUTF8(sv) || is_utf8_string((const U8*)s,len))
XSRETURN_YES;
else
XSRETURN_NO;
}
XSRETURN_EMPTY;
}В строке с XSRETURN_EMPTY анализатор обнаружил недостижимый код. В этой функции есть два оператора return и croak_xs_usage – макрос, который раскрывается в noreturn-функцию:
void Perl_croak_xs_usage(const CV *const cv, const char *const params)
__attribute__((noreturn));В подобных местах кода Perl 5 для указания недостижимой ветви используется макрос NOT_REACHED.
V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. inffast.c 296
void ZLIB_INTERNAL inflate_fast(z_streamp strm, unsigned start)
{
....
unsigned long hold; /* local strm->hold */
unsigned bits; /* local strm->bits */
....
hold &= (1U << bits) - 1;
....
}Анализатор обнаружил подозрительную операцию при работе с битовыми масками. В качестве битовой маски используется переменная меньшей разрядности, чем переменная hold. Это приводит к потере старших бит. Разработчикам следует обратить внимание на этот код.
Находить ошибки сквозь макросы было очень сложно. Просмотр отчёта отнял много сил и времени. Тем не менее, в статью вошли очень интересные случаи, похожие на настоящие ошибки. Отчёт анализатора достаточно большой, там точно есть ещё много интересного. Но я не в силах его далее просматривать :). Советую разработчиком проверить проект самостоятельно и устранить дефекты, которые им удастся обнаружить.
P.S. Нам однозначно хочется поддержать этот интересный проект, и мы готовы предоставить разработчикам лицензию на несколько месяцев.