Valgrind - это хорошо, но недостаточно

10 Сен 2014

Автор: Андрей Карпов

Не так давно мы пытались продемонстрировать пользу от использования статического анализатора PVS-Studio одной из компаний. Ничего дельного из этого не вышло. Но в процессе переписки я подготовил развёрнутый ответ, касающийся методологий статического и динамического анализа. Сейчас я решил оформить этот ответ в виде небольшой статьи. Думаю, текст может показаться интересным читателям, да и просто можно будет использовать эту статью при общении с новыми потенциальными клиентами.

Итак, в процессе переписке был задан вопрос приблизительно следующего содержания:

Мы уже экспериментировали со статическими анализаторами и пришли к выводу, что их точность намного хуже обычного valgrind. Поэтому непонятно зачем статический анализ нужен. Ложных срабатываний очень много, а ошибок, которые не находит запущенный valgrind, обычно никаких не находится.

Мной был подготовлен следующий ответ, который я привожу, сделав только небольшие поправки:

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

Пытаться найти что-то в давно и стабильно работающем коде достаточно неблагодарное занятие. Смысл статического анализа - это предотвратить множество ошибок на самых ранних этапах. Да, большинство этих ошибок можно найти другими методами. Их заметит или сам программист, или выявят большие тесты или тестировщики. В худшем случае, об ошибках сообщат пользователи. Но в любом случае, это зря потраченное время. Многие из опечаток, ошибок Copy-Paste и прочих ляпов можно устранить ещё на самых ранних этапах с помощью статического анализа. Найти многие ошибки сразу после написания кода - вот главная его ценность. Нахождение ошибки на любом следующем этапе обходится во много раз дороже.

Почему-то после этого, сразу все говорят, что уж наши программисты не делают опечаток и Copy-Paste. Это не правда. Делают. Все делают: http://www.viva64.com/ru/b/0260/

Хорошо, допустим мы убедили вас, что статический анализ может найти какие-то ошибки. Справедлив вопрос, но нужен ли он, раз есть такие инструменты как valgrind. Ведь они действительно дают меньше ложных срабатываний.

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

Мы уже писали о том, где статический анализ помогает другим технологиям. Например, мы описывали в чем разница статического и динамического анализа кода в этой заметке: http://www.viva64.com/ru/b/0248/

Ещё одна заметка о том, как статический анализ дополняет тестирование с помощью юнит-тестов: http://www.viva64.com/ru/a/0080/

Однако, чтобы не быть совсем абстрактными, попробуем объяснить разницу между статическим и динамическим анализом на примерах. Например, выделим вот такой интересный фрагмент в конструкторе класса SlowScanner:

class SlowScanner {
  ....
  explicit SlowScanner(Fsm& fsm)
  {
    ....
    Fill(m_letters,
         m_letters + sizeof(m_letters)/sizeof(*m_letters), 0);
    ....
  }
  ....
  size_t* m_letters;
  ....
}

Анализатор PVS-Studio выдаёт предупреждение: V514 Dividing sizeof a pointer 'sizeof (m_letters)' by another value. There is a probability of logical error presence. slow.h 238

Скорее всего, когда-то член класса 'm_letters' являлся массивом с жестко заданным размером. Конечно, это просто предположение, но вполне вероятное. Например, вначале было написано как-то так: size_t m_letters[MAX_COUNT];. В те времена определение размера массива было корректным:

sizeof(m_letters)/sizeof(*m_letters)

Затем массив стал динамическим, и переменная 'm_letters' превратилась в простой указатель. Теперь выражение "sizeof(m_letters)/sizeof(*m_letters)" всегда равно единице. В 32-битной системе размер указателя и типа size_t равны 4. В 64-битной системе размер этих типов будет 8. Однако, независимо от того, делим мы 4 на 4 или 8 на 8, мы всегда получаем 1.

Таким образом, функция Fill() обнуляет только один байт массива. Ошибка может вовсе не проявить себя, если память случайно окажется уже обнулённой или, если неинициализированные элементы не будут использоваться. В этом её коварство. Но может произойти чтение неинициализированного элемента массива.

Может ли эту ошибку найти динамический анализатор? Не знаю. Возможно, он может обнаружить чтение из неинициализированной памяти. Тогда почему он молчит? Здесь мы подходим к одному из важных различий статического и динамического анализа.

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

Статический анализ проверяет все ветви, которые теоретически могут получить управления. Поэтому он может находить ошибки, независимо от того, как часто исполняется тот или иной код.

Кстати, немного отойдём от темы. Мы можем не только предложить вам наш анализатор, но и услуги по аудиту кода. Результатом такого аудита может стать документ, включающий набор рекомендаций по улучшениям. Его вполне можно включить в стандарт кодирования. Мы уже имеем опыт выполнения таких работ. Например, чтобы не допускать ошибок, связанных с вычислением размера массива, можно рекомендовать использовать специальную технологию (подсмотренную в Chromium):

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define arraysize(array) (sizeof(ArraySizeHelper(array)))

Макрос 'arraysize' нельзя применить к обыкновенному указателю. Возникнет ошибка компиляции. Тем самым мы защищаемся от случайных ошибок. Если вдруг массив превратится в указатель, нельзя будет пропустить место, где вычисляется его размер.

Вернёмся к статическому и динамическому анализу. Например, посмотрим на эту функцию:

inline RECODE_RESULT _rune2hex(wchar32 in,
  char* out, size_t out_size, size_t &out_writed)
{
    static const char hex_digs[]="0123456789ABCDEF";
    bool leading = true;
    out_writed = 0;
    RECODE_RESULT res = RECODE_OK;
    for (int i = 7; i >=0; i--){
        unsigned char h = (unsigned char)(in>>(i*4) & 0x0F);
        if (h || !leading || i==0){
            if (out_writed + 1 >= out_size){
                res = RECODE_EOOUTPUT;
                break;
            }
            out[out_writed++] = hex_digs[h];
        }
    }
    return res;
}

С точки зрения динамического анализа здесь нет ничего подозрительного. В свою очередь, статический анализатор PVS-Studio предлагает обратить внимание на переменную 'leading': V560 A part of conditional expression is always false: !leading. recyr_int.hh 220

Думаю, здесь нет никакой ошибки. Переменная 'leading' оказалась после рефакторинга лишней. А вдруг нет? Вдруг код не дописан? Это то место, на которое стоит обратить внимание. И, если переменная лишняя, то удалить её, чтобы она не сбивала с толку не только анализатор, но и людей, которые будут поддерживать этот код.

Предупреждения, что часть выражения всегда константа, могут казаться неинтересными. Посмотрите тогда примеры ошибок, найденные с помощью диагностики V560 и будете удивлены, чего только не встретишь в коде: http://www.viva64.com/ru/examples/V560/

Такие ошибки не могут быть найдены динамическим анализом. Ему тут нечего искать. Это просто некорректные логические выражения.

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

Рассмотрим функцию sslDeriveKeys, работающую с приватными данными:

int32 sslDeriveKeys(ssl_t *ssl)
{
  ....
  unsigned char buf[SSL_MD5_HASH_SIZE + SSL_SHA1_HASH_SIZE];
  ....
  memset(buf, 0x0, SSL_MD5_HASH_SIZE + SSL_SHA1_HASH_SIZE);

  psFree(ssl->sec.premaster);
  ssl->sec.premaster = NULL;
  ssl->sec.premasterSize = 0;
skipPremaster:
  if (createKeyBlock(ssl, ssl->sec.clientRandom,
        ssl->sec.serverRandom,
        ssl->sec.masterSecret, SSL_HS_MASTER_SIZE) < 0)
  {
    matrixStrDebugMsg("Unable to create key block\n", NULL);
    return -1;
  }
  return SSL_HS_MASTER_SIZE;
}

Динамический анализатор здесь ничего не найдет. Код с точки зрения языка абсолютно корректен. Чтобы найти ошибку нужно мыслить более высокоуровневыми паттернами, что умеют делать статические анализаторы.

Нас интересует локальный массив 'buf'. Так как он хранит приватные данные, то в конце функции сделана попытка обнулить этот массив с помощью функции memset(). В этом и заключается ошибка.

Смотрите, после вызова memset() локальный массив 'buf' более не используется. Это значит, что компилятор вправе удалить вызов фунции memset(), так как её вызов не оказывает никакого эффекта с точки зрения языка Си/Си++. Более того, он не только в праве, но и действительно удалит эту функцию в release версии.

В результате, приватные данные останутся в памяти и теоретически могут попасть туда, куда им попадать не надо. Таким образом, проект становится чуть менее безопасным из-за ошибки в сторонней библиотеке.

PVS-Studio выдаст следующее предупреждение: 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. sslv3.c 123

Данная ошибка является потенциальной уязвимостью. Может показаться, что она крайне незначительна. Однако, она может привести к весьма неприятным последствиям, вплоть до отправки фрагментов приватных данных по сети. Как такие чудеса могут произойти, описано в статье специалиста компании ABBYY Дмитрия Мещерякова: http://habrahabr.ru/company/abbyy/blog/127259/

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

Если мы заинтересовали вас, то предлагаем наметить дальнейшие шаги возможного сотрудничества и демонстрации возможностей анализатора на живых больших реальных проектах.

#Cpp #StaticAnalysis