Мы используем куки, чтобы пользоваться сайтом
было удобно.
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.
Проект Apache HTTP Server продолжает развиваться. Анализатор PVS-Studio не отстаёт и становится всё более мощным с каждой новой версией. Посмотрим, какие результаты покажет проверка на этот раз.
Apache HTTP Server - это кроссплатформенный проект с открытым исходным кодом. Проект состоит из множества модулей. Ядро HTTP Server полностью разработано командой Apache Software Foundation на языке С. Прочие компоненты сервера созданы различными сторонними разработчиками, поддерживающими инициативу открытого программного обеспечения.
Ранние версии проекта Apache HTTP Server проверялись с помощью Сoverity. В проверенной версии не было обнаружено следов проверки другими анализаторами. Качество кода проекта на высоком уровне. Несмотря на это, анализатор PVS-Studio смог обнаружить несколько интересных ошибок.
Проект уже проверялся в 2011 году. О найденных в то время ошибках можно прочитать в статье "Лев Толстой и статический анализ кода".
Для проверки использовался анализатор PVS-Studio версии 6.08.
typedef struct {
....
ap_regmatch_t *re_pmatch;
apr_size_t re_nmatch;
const char **re_source;
....
} ap_expr_eval_ctx_t;
static const char *ap_expr_eval_re_backref(
ap_expr_eval_ctx_t *ctx, ....)
{
int len;
if (!ctx->re_pmatch ||
!ctx->re_source ||
*ctx->re_source == '\0' || // <=
ctx->re_nmatch < n + 1)
return "";
....
}Предупреждение:
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: ** ctx->re_source == '\0'. util_expr_eval.c 199
При работе с указателями иногда возникает путаница между указателем и значениями, на которые он указывает. В примере видно, что при проверке третьего подвыражения в условии забыли разыменовать указатель. Разработчик предполагает, что он проверяет, является строка пустой или нет. Для этого нужно сравнить первый символ строки с терминальным нулём. Но на самом деле происходит сравнение указателя с нулевым символом. После исправления выражения становится очевидно, что код так же следует дополнить проверкой на существование указателя на строку.
Анализатор уже обнаруживал данную ошибку в проекте, о чём свидетельствует запись на странице с примерами ошибок, находимых диагностикой V528. Раз ошибка по-прежнему присутствует в проекте, о ней следует упомянуть вновь. Для исправления требуется изменить код следующим образом:
if (!ctx->re_pmatch ||
!ctx->re_source ||
!*ctx->re_source ||
**ctx->re_source == '\0' ||
ctx->re_nmatch < n + 1)
return "";apr_status_t iconv_uc_conv(...., apr_size_t *res)
{
....
*res = (apr_size_t)(0);
if (data == NULL) {
*res = (apr_size_t) -1;
return APR_EBADF;
}
....
if (size < 0) {
....
if (size)
*res ++; // <=
}
....
}Предупреждение:
V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. iconv_uc.c 114
В коде используется операция разыменования указателя. Полученное значение при этом не используется. Судя по коду функции, работать требовалось именно со значением указателя. А значит, для исправления ошибки надо повысить приоритет операции разыменования с помощью скобок: (*res) ++;
int get_password(struct passwd_ctx *ctx)
{
....
if (strcmp(ctx->passwd, buf) != 0) {
ctx->errstr = "password verification error";
memset(ctx->passwd, '\0', strlen(ctx->passwd));
memset(buf, '\0', sizeof(buf));
return ERR_PWMISMATCH;
}
....
memset(buf, '\0', sizeof(buf)); // <=
return 0;
....
}Предупреждение:
V597 The compiler could delete the 'memset' function call, which is used to flush 'buf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. passwd_common.c 165
Обязанность любой программы, работающей с конфиденциальными данными, затирать в памяти пароли и прочую важную информацию после завершения работы с ней. В этом фрагменте разработчики пытаются обнулить буфер, использовавшийся для хранения пароля. Выбранный ими способ должен был выполнить эту задачу. Однако для того, чтобы функция memset выполнила задачу правильно, буфер должен использоваться в последующем коде. Если же память заполняется нулями, а потом не используется, компилятор имеет право удалить функцию memset при построении кода. В результате информация, которая должна быть уничтожена, остаётся в памяти. Неизвестно, что будет с этим регионом памяти после, и куда попадёт эта неуничтоженная информация. Для очистки памяти надо использовать специализированные функции, такие как RtlSecureZeroMemory() или memset_s().
Хочется отметить, что это, пожалуй, самые серьезные из найденных дефектов для такого приложения как Apache HTTP Server!
Ещё несколько сообщений найденных этой диагностикой:
static int warrsztoastr(...., const wchar_t * arrsz, int args)
{
const apr_wchar_t *wch;
apr_size_t totlen;
apr_size_t newlen;
apr_size_t wsize;
сhar **env;
char *pstrs;
char *strs;
int arg;
if (args < 0) {
for (args = 1, wch = arrsz; wch[0] || wch[1]; ++wch)
if (!*wch)
++args;
}
wsize = 1 + wch - arrsz;
newlen = totlen = wsize * 3 + 1;
....
(void)apr_conv_ucs2_to_utf8(arrsz, &wsize, strs, &newlen);
....
return args;
}Предупреждение:
V614 Potentially uninitialized pointer 'wch' used. start.c 58
Функция подготавливает информацию для конвертирования строки из Wide Unicode в UTF-8. Если переменная args содержит отрицательное значение, значит количество символов в строке неизвестно, и их требуется посчитать.
Позднее, на основе адреса последнего символа в строке, содержащегося в переменной wch, и адреса начала строки arrsz, вычисляется значение wsize. С помощью wsize создаётся буфер для новой строки. Переменная wch инициализируется внутри цикла, который начинает работу только если args содержит отрицательное значение. Если же в функцию поступит положительное значение, то инициализации переменой не произойдёт. Это приведёт к неопределённому поведению, так как размер буфера будет вычислен некорректно.
На данный момент функция используется в проекте всего один раз с значением args равным -1. Поэтому эта ошибка долгое время оставалась бы незаметна, до тех пор, пока не понадобилось использовать функцию с положительным значением args. Я не могу знать, что планировалось делать внутри этой функции в такой ситуации. Как минимум странно видеть в виде параметра значение, которое в итоге возвращается как результат её работы. В то время как присутствие условного оператора в начале функции, в случае положительного значения args, делает её выполнение совершено бессмысленным.
static int is_quoted_pair(const char *s)
{
int res = -1;
int c;
if (((s + 1) != NULL) && (*s == '\\')) { // <=
c = (int) *(s + 1);
if (apr_isascii(c)) {
res = 1;
}
}
return (res);
}Предупреждение:
V694 The condition ((s + 1) != ((void *) 0)) is only false if there is pointer overflow which is undefined behaviour anyway. mod_mime.c 531
Очень подозрительное условие. Первое выражение в условии может быть ложным только в том случае, если произойдёт переполнение при сложении указателя с единицей. Если происходит переполнение указателя, то возникает неопределённое поведение, так что этот код в любом случае некорректен.
#define SHSTDAPI EXTERN_C DECLSPEC_IMPORT HRESULT STDAPICALLTYPE
SHSTDAPI SHGetMalloc(_Outptr_ IMalloc **ppMalloc);
LRESULT CALLBACK ConnectDlgProc(....)
{
....
if (SHGetMalloc(&pMalloc)) { // <=
pMalloc->lpVtbl->Free(pMalloc, il);
pMalloc->lpVtbl->Release(pMalloc);
}
....
}Предупреждение:
V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'SHGetMalloc(& pMalloc)'. The SUCCEEDED or FAILED macro should be used instead. apachemonitor.c 915
SHGetMalloc представляет из себя системную функцию и возвращает результат типа HRESULT. HRESULT 32-разрядное значение, логически состоящее из трёх полей. Его нельзя использовать как значение типа bool. Для обработки таких значений необходимо использовать макрос SUCCEEDED.
static const char *process_resource_config_fnmatch(....)
{
apr_status_t rv;
....
rv = apr_dir_open(&dirp, path, ptemp);
if (rv != APR_SUCCESS) {
return apr_psprintf(p,
"Could not open config directory %s: %pm",
path, &rv);
}
candidates = apr_array_make(ptemp, 1, sizeof(fnames));
while (apr_dir_read(....) == APR_SUCCESS) {
....
if (rest && (rv == APR_SUCCESS) && // <=
(dirent.filetype != APR_DIR)) {
continue;
}
fnew = (fnames *) apr_array_push(candidates);
fnew->fname = full_path;
}
....
}Предупреждение:
V560 A part of conditional expression is always true: (rv == 0). config.c 2029
Анализатор усмотрел лишнюю проверку внутри условия. На первый взгляд это просто избыточный код. При подробном рассмотрении видно, что такое значение переменной rv не позволило бы дойти до входа в цикл. К тому же странно, зачем использовать значение переменной, оставшееся после выполнения предыдущих операций, если в теле цикла она больше негде не используется.
Логично предположить, что перед условием также надо было использовать функцию rv = apr_dir_open(...); и тогда проверка результата rv обретает смысл. Конечно, могу ошибаться и это просто лишняя проверка, но на мой взгляд фрагмент кода обязательно надо проверить. Возможно, разработчики увидят, что в код цикла и обрабатываемых в нём операций действительно закралась ошибка и смогут вовремя её исправить.
Ещё несколько похожих ошибок:
static int uldap_connection_init(....)
{
....
if (ldc->ChaseReferrals==AP_LDAP_CHASEREFERRALS_ON){
if ((ldc->ReferralHopLimit != AP_LDAP_HOPLIMIT_UNSET) &&
ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON) {
....
}
}
....
}Предупреждение:
V571 Recurring check. The 'ldc->ChaseReferrals == 1' condition was already verified in line 399. util_ldap.c 400
В этом примере приведено избыточное условие. Нет необходимости проверять одно и тоже выражение во внутреннем и внешнем условном операторе, так как условием входа во внутренний оператор, является прохождение проверок внешнего. В данном случае, весь код, заложенный внутри операторов, требует проверки всех выражений в обеих операторах if. Поэтому логичнее убрать внешний условный оператор. А для сохранения последовательности проверок предлагаю немного изменить выражение внутреннего условного оператора.
if (ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON &&
(ldc->ReferralHopLimit != AP_LDAP_HOPLIMIT_UNSET)) {
....
}#ifdef _MSC_VER
#pragma warning(disable: 4032)
#include <conio.h>
#pragma warning(default: 4032)
#else
#include <conio.h>
#endifПредупреждение:
V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 38, 40. apr_getpass.c 40
В приведённом фрагменте кода разработчики вместо возвращения директиве предыдущего значения используют значение по умолчанию для директивы. Это ошибочный подход. В таких случаях необходимо действовать иначе: запомнить предыдущее значение с помощью директивы #pragma warning(push), и вернуть его #pragma warning(pop). Исправленный код:
#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable: 4032)
#include <conio.h>
#pragma warning(pop)
#else
#include <conio.h>
#endifНайденные недочёты показывают, что ошибки могут остаться даже в самых качественных и хорошо протестированных проектах. Статический анализ требует регулярного подхода. Одноразовой проверки недостаточно. Как бы ни был опытен программист, он все равно может допускать опечатки и иные ошибки. Анализатор PVS-Studio позволит выявить ошибки и подозрительные места раньше, чем они отразятся на итоговом релизе. Предлагаю скачать и попробовать анализатор PVS-Studio самостоятельно.
0
0
0
0