Проверка VirtualDub

[Gorthauer87]

А нет желания сделать серверную реализацию всего этого дела и подцепить запуск анализатора через git hooks, а все результаты просто в виде html таблицы выдавать? И добавить варианты чтобы коммиты с некоторыми типами предупреждений оно не пропускало вовсе.

[Indexator]

Я вот тоже размышлял, почему бы им не сотрудничать с гитхабом и/или битбакетом, можно для каждого аккаунта учитывать насколько он качественный код пушит, растет или деградирует, строить всякие графики качества кода проектов целиком и каждого члена команды в отдельности и тд и тп?

[koreec]

Чем закончилась история с Хромиумом?

[EvgeniyRyzhkov]

Она еще не закончилась.

[belonesox]

Андрей, привествую!

Реквествую следующим на ревью AVISynth.

[Indexator]

А вы исходники PVS-Studio так же анализируете самой PVS-Stuio? :)
ps: почему вы не избавитесь от этого дурацкого дефиса в названии?

[EvgeniyRyzhkov]

ps: почему вы не избавитесь от этого дурацкого дефиса в названии?

У нас половина заходов с поиска по PVS-Studio, половина по PVS Studio. Как ни назови, половина будет промахиваться.

[Andrey2008]

Ответы на вопросы, которые часто задают после прочтения наших статей.

[Andrey2008]

В статье упоминается ссылка вот на это. А что Вы думаете? Стали ли вы править такой свой код? А чужой? Стоит ли оставить всё как есть? Стоит ли бороться со странным, но работающим кодом? А что в плане поддержки чужого такого кода? В общем, интересуют любые мысли и мнения по этому поводу.

Вопрос не просто так. Не раз сталкивался с позицией: «Мы не будем искать и править ошибки/кривые места в старом коде, но готовы делать это в свежем коде». В результате, зреет идея сделать новый режим в PVS-Studio. Все ошибки после первой проверки считаются не ошибками. И выдаем сообщения на подозрительные места только в измененных или новых фрагментах кода. Глупость или мудрость?

[Indexator]

«Мы не будем искать и править ошибки/кривые места в старом коде, но готовы делать это в свежем коде»… Глупость или мудрость?

Глупость!

Стали ли вы править такой свой код? А чужой? Стоит ли оставить всё как есть? Стоит ли бороться со странным, но работающим кодом? А что в плане поддержки чужого такого кода?

Я в одном из своих топиков переводил Линуса, и я с ним согласен, где все можно свести к достаточно простым правилам: Код должен быть читаем. Должен соблюдаться баланс между фичами (не должно одно ломаться за счет другого). Код должен быть поддерживаемым, переносимым. Не нужно допускать того, чтобы приходилось править чей-то кривой код, просто его не должно оказаться в репозитории изначально, пока автор фичи не оформит все как полагается его код даже смотреть никто не будет. Почитайте, там много умных мыслей.
Бороться надо, но лучше, когда это делает мейнтейнер/автор проекта… А тут, похоже, ситуация печальная… Вместо благодарности — детские обиды…

[Andrey2008]

Но ведь старый код работает. Работает — не тронь! Что-то с ним делать — только тратить время и вносить новые ошибки.

[KindDragon]

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

[quarantino]

Смотря как «работает». Если работает, но нашли переполнение буфера или подобную дыру — надо исправлять.

[Amomum]

У меня вопрос по поводу сдвигов отрицательных чисел.
Насколько я помню, в своей статье habrahabr.ru/company/pvs-studio/blog/141880/ вы рекомендуете вместо (-1 << x) использовать (~0 << x).

Однако, вот тут stackoverflow.com/questions/809227/is-it-safe-to-use-1-to-set-all-bits-to-true пишут, что ~0 не обязательно эквивалентен всем единичным битам в двоичном представлении (хотя, честно говоря, я не до конца понял, почему).

Возможно, наиболее безопасным будет сделать что-то вроде:

unsigned long a = -1; b = (a << x);

[Andrey2008]

Вот ведь язык… Никогда нельзя чувствовать себя в безопасности. :)

[Amomum]

(развел руками)

[markhor]

пишут, что ~0 не обязательно эквивалентен всем единичным битам в двоичном представлении

1) ~0 инвертирует все биты, если отрицательные числа хранятся на целевой машине в дополнительном коде (см. вики). Однако, может быть и по-другому: как пишет автор ответа, в обратном коде получится ноль (-0=1111...1, ~-0=0).

2) Ситуация с ~0u не зависит от представления отрицательных чисел. Однако, если long длиннее, чем int, при присваивании unsigned long = ~0u левая часть числа останется с носом нулями.

[Amomum]

Однако, может быть и по-другому: как пишет автор ответа, в обратном коде получится ноль (-0=1111...1, ~-0=0).

Вот этот момент был не ясен. Я не заметил, что бы там писали "~ — 0".

В любом случае, я сомневаюсь, что сейчас вообще можно найти машину с хранением отрицательных чисел в обратном коде.

[vsespb]

Намного более скользкой темой являются операции сдвига

То, что код работает, это везение.

Если тест написан, то всё ок.

[dimoclus]

Диагностическое сообщение выданное PVS-Studio: V595 The 'lpbiOutput' pointer was utilized before it was verified against nullptr. Check lines: 82, 85. VirtualDub yuvcodec.cpp 82

LRESULT YUVCodec::DecompressGetFormat(BITMAPINFO *lpbiInput,
                                      BITMAPINFO *lpbiOutput)
{
  BITMAPINFOHEADER *bmihInput    = &lpbiInput->bmiHeader;
  BITMAPINFOHEADER *bmihOutput  = &lpbiOutput->bmiHeader;
  LRESULT res;

  if (!lpbiOutput)
    return sizeof(BITMAPINFOHEADER);
  ....
}

Это ложное срабатывание. Здесь нет ни разыменования нулевого указателя, ни UB. Сомневаетесь?

#define offsetof(Struct, Field) (uintptr_t)((char*)&(((Struct*)NULL)->Field) - (char*)((Struct*)NULL))

Примерно так выглядит макрос offsetof в stddef.h

[sin_avatar]

Вы случайно не перепутали sizeof с offsetof?

[Andrey2008]

Да, что-то не очень удачный пример выбрал. Надо будет поменять на другой фрагмент. Спасибо.

[Andrey2008]

Будем считать, что в статье приводится вот такой пример. :)

void VDTContextD3D9::Shutdown() {
  ....
  mpData->mFenceManager.Shutdown();
  ....
  if (mpData) {
    if (mpData->mhmodD3D9)
      FreeLibrary(mpData->mhmodD3D9);
  ....
}

[tagir_valeev]

Всегда читаю посты про PVS-студио, примеряя их на Java с разработкой в Eclipse. По данному посту вывод такой:
Неактуальные для Java проблемы:
Виртуальные деструкторы, неопределённое поведение со сдвигами (в Java эти операции не зависят от версии и ключей компилятора), ссылка на уничтоженный объект, приведённые неправильные сравнения (в Java нет типов, не включающих отрицательные значения).

Проблемы, о которых предупредит бесплатная IDE:
Проверка на null после создания объекта:

Проверка на null после использования объекта:

Присваивание самому себе:

Точка с запятой после while:

(если у вас этих предупреждений нет, поройтесь в настройках Eclipse)

Частично актуальные проблемы:
Магическая длина строк. С одной стороны, длина строки или массива хранится в Java в самом объекте строки и массива, поэтому подобной проблемы быть не должно. Но, например, такие конструкции я встречал в реальном коде:

if(str.startsWith("http://"))
	return str.substring(7);

Работа с типом HRESULT. Ну если б был такой тип, конечно, можно бы было неправильно проверять его значение. Но Java автоматом не приводит к логическому типу, поэтому программисту пришлось бы явно писать if( hResult != 0 ), и он бы, возможно, задумался, правильно ли это :-)

Неправильный аргумент printf. В Java есть аналогичные методы (например, String.format), и такая ошибка не будет поймана на этапе компиляции, но при любом несоответствии типов случится исключение на этапе выполнения, поэтому выстрелит сразу, а не неизвестно когда.

Актуальные проблемы:
Взаимоисключающие проверки. Eclipse не сечёт даже обращения к локальным переменным или параметрам типа таких:

public void test(boolean val) {
  if(val) {
    if(!val) {
      System.out.println("Unreachable");
    }
  }
}

То же самое с повторной проверкой одного и того же условия. А уж проверки с разыменованием, как в примере в статье, наверно, и не будет никогда просекать, ведь другой поток мог изменить объект в это время, и повторная проверка выдаст другой результат.

Потенциальное отсутствие else, повторное присваивание Eclipse не подсвечивает. За абсолютные пути, конечно, тоже никто не поругает.

[Wolong]

IDEA это обнаруживает:

[tagir_valeev]

Я подозревал :-) Интересно, а более сложные зависимости она сечёт? Например, вот тут у эклипса ложное срабатывание, хотя он мог бы догадаться:

[Wolong]

Отлично обраруживает:

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

[tagir_valeev]

Круто, IDEA рулит :-)

[quarck]

Прогнал анализ с помощью PVS Studio кода рабочего проекта, таки да, нашло много интересного (особенно с учетом того, что часть кода мы аутсорсили в китай). Было интересно и полезно, спасибо

Но вот некоторые вещи — PVS Studio через чур самоуверенно утрверждает не правильные утверждения ;) например, код:

for (int x = start_x; x * sign <= end_x * sign; x += 16 * sign)

PVS Studio выдает:

V584 The 'sign' value is present on both sides of the '<=' operator. The expression is incorrect or it can be simplified. XXXXX.h 2385

что принципиально не верно, т.к. в данном случае значение «sign» не известно в момент компиляции, а от него зависит знак оператора — если вдруг sign окажется отрицательным, то " x * sign <= end_x * sign" упрощается до " x >= end_x"

[Andrey2008]

Рад, что нашлось интересное. Теперь предлагаю попробовать поработать с включенным инкрементальным анализом.

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

[sluge]

Предлагаю вам прочекать реально большие продукты, например firefox, linux kernel

[EvgeniyRyzhkov]

Про Chromium читали наш отчет?