How to become an author
Search
Write a publication
Pull to refresh

Comments 88

Ну вот, пофиксят все баги по подсказке из статьи и бета тестерам в Гугл урежут ЗП…

Total votes 2: ↑1 and ↓1 0
Тут всё сложнее.
1) Сделать бранч
2) Заасайнить туда парочку лидов, которые знают этот модуль
3) Зафиксить
4) Прогнать тесты, а может и написать свои тесты (с багами, разумеется)
Возможно, пофиксить багу. И надеяться, что расхождения с тестами нет. Темболее этот код может быть в таких дебрях, что юзается только на 1.5 моделях Lenovo, которые уже не выпускаются.
Total votes 1: ↑1 and ↓0 +1
Было бы круто если бы ваш сервис работал над всем кодом в публичном Github и репродукции всем владельцам репозиториев, но увы )
Total votes 2: ↑2 and ↓0 +2

Было бы круто встроить подобный анализатор во все среды разработки

Total votes 1: ↑1 and ↓0 +1
если бы ваш сервис работал над всем кодом в публичном Github
Тогда pvs не сможет зарабатывать, а значит придётся сокращать штат и закрываться, значит продукт перестанет развиваться, значит некому будет делать такие проверки…
Total votes 2: ↑2 and ↓0 +2

Ну если встроить в IDE — работу оплатят авторы IDE, если в Github — владельцы гитхаба и т.п.

Total votes 5: ↑2 and ↓3 -1
Причем здесь travis ci? Что они сделали хороший продукт, смогли привлечь инвестиции и выжили? А если у PVS так не получится? И зачем тогда PVS, если есть travis?
This comment wasn’t rated yet 0
Я не пойму, вы слышали про github marketplace?
This comment wasn’t rated yet 0
UFO just landed and posted this here
Было бы круто если бы Google выкупил бы PVS-Studio и сделал бы его опенсорс. ))
Total votes 4: ↑2 and ↓2 0
do {
  ....
  if (x) continue;
  ....
} while (0)


Не исключено что это так и задумывалось, программист хотел избежать использования goto.
Total votes 3: ↑3 and ↓0 +3
Возможно, но тогда намного лучше написать break. И вообще, зачем тогда цикл? Можно было вообще цикл не писать.
Total votes 4: ↑3 and ↓1 +2
Он цикл не использует как цикл. Он проходит по телу якобы «цикла» один раз, используя его как блок, заранее зная что пост-условие всегда ложно. Но ему как-то нужно не имея метки и goto выйти из тела блока. Вот он и делает continue, тщательно избегая использования goto, который стал «табу». А то что можно было использовать break — это да.
Можно даже понять психологию, почему программист поставил continue. Оно представляется как «стрелка» перехода. А вот break — он мыслится как своего рода exception, return, stop. Так как программист задумывал именно goto-переход — он и написал continue.

А вообще интересно, как часто в разных исходниках используют goto?
Total votes 5: ↑5 and ↓0 +5
Он цикл не использует как цикл. Он проходит по телу якобы «цикла» один раз, используя его как блок, заранее зная что пост-условие всегда ложно.
Все остальные case заканчиваются на return. Если цикл убрать, ничего не изменится.

А вообще интересно, как часто в разных исходниках используют goto?
Не смогу ответить. Не ведём такую статистику.
Total votes 5: ↑4 and ↓1 +3
Нет, не так про continue. Там еще switch внутри цикла. break — переход в точку после switch, а continue — в конец цикла. И do используется именно как цикл — в этом if инкрементируется счетчик, который инициализируется перед do. Так что про ошибку все правильно.
Total votes 1: ↑1 and ↓0 +1
Давайте уж тогда весь контекст рассмотрим:

status_t Check(const std::string& source) {
  ....
  int pass = 1;
  ....
  do {
    ....
    switch(rc) {
    ....
    case 4:
      if (pass++ <= 3) {
          SLOGW("Filesystem modified - rechecking (pass %d)",
                  pass);
          continue;                                         // <=
      }
      SLOGE("Failing check after too many rechecks");
      errno = EIO;
      return -1;
    ....
    }
  } while (0);                                              // <=

  return 0;
}


Абсолютно очевидно, что идея continue была выполнить еще одну итерацию цикла, который, все правильно, в нормальных условиях должен иметь только один проход
Total votes 3: ↑3 and ↓0 +3

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


пока(естьОшибки И числоПовторов < МаксимальноеЧисло)
{
   ...
}```
Total votes 3: ↑3 and ↓0 +3
Или использовать «оператор» "-->" ;-)
int limit = 3;
while (limit --> 0)
{
   ...
}
This comment wasn’t rated yet 0
Если выносить Switch case в отдельную функцию и использовать в ней return вместо break, то мир становится проще, приятнее и более читабельным.
This comment wasn’t rated yet 0
Ничем. Зато со стороны других компаний после этой публикации был всплеск интереса и новые продажи.
Total votes 4: ↑3 and ↓1 +2

Не "ничем" а "улучшением анализатора" :))

Total votes 2: ↑2 and ↓0 +2
После такого исхода понятно, почему tizen такая кривая со всех сторон. Пилят «лишь бы было», видимо.
This comment wasn’t rated yet 0
по поводу проверки после выделения памяти. всё обьясняется просто:
для олд-скульных С пересевших на С++, использовали malloc-free и перенесли проверку на выделение уже в С++ new.
А С++, привыкшие к new, скорее всего придерживались уже устоявшегося malloc-free в старых модулях, ну и не заморачиваются с проверками.
Это всё потому что люди пишут на несуществующем языке С/С++ вместо С или С++.
Total votes 3: ↑3 and ↓0 +3
Правильная проверка должна быть такой:
if (f->mode == O_RDONLY && expect_zeros) {

Думаю так правильнее, т.к. mode скорее всего может содержать дополнительные флаги, вроде O_NOFOLLOW
if (f->mode & O_RDONLY == O_RDONLY && expect_zeros) {
Total votes 2: ↑1 and ↓1 0
f->mode & O_RDONLY == O_RDONLY
И получается, что 0 == 0. Т.е. условие всегда истинно. «Отличная» правка :).
Total votes 4: ↑3 and ↓1 +2
Конец рабочего дня даёт о себе знать :) Тем не менее, просто сравнивать с O_RDONLY тоже некорректно из-за возможных дополнительных флагов
This comment wasn’t rated yet 0
А мне почему-то кажется, что имелось в виду что-то такое
#define O_NORIGHTS 0
#define O_READ 1
#define O_WRITE 2
#define O_READWRITE 3

и проверка
f->mode & O_READ

должна была проверять разрешённость именно чтения. Хотя комментарий нам и говорит явно про «read-only», он мог писаться постфактум и быть просто навеян названием константы O_RDONLY. Тут без остального контекста сложно понять. Хотя, конечно, Ваше предположение выглядит проще.
This comment wasn’t rated yet 0
Т.е., оперируя старыми названиями констант, тянет заменить
if (f->mode & O_RDONLY && expect_zeros)

скорее на
if ( (f->mode == O_RDONLY || f->mode == O_RDWR) && expect_zeros)
Total votes 1: ↑1 and ↓0 +1
Успокойтесь, всё уже придумано за нас. Имелось в виду
if (f->mode & O_ACCMODE == O_RDONLY && expect_zeros)
Хороший пример для Andrey2008, когда он отбивается на тему: «ошибки мы нашли, а вот как правильно их пофиксить — это уже разработчики думать нужны».

Человеку легко догадаться, что где-то там должна быть маска и найти её посмотрев в .h файл (и то, как видим не всем). А вот роботу…
Total votes 2: ↑2 and ↓0 +2
Вот только у "==" приоритет выше, чем у "&", и выше сравнение распарсится как «f->mode & (O_ACCMODE == O_RDONLY)», т.е. всегда false.
Total votes 3: ↑3 and ↓0 +3
А это уже повод запустить ещё раз PVS-Studio и пофиксить.

Впрочем у нас clang и -Werror, так что до PVS-Studio эта ошибка не доживёт…
Total votes 1: ↑0 and ↓1 -1
компилятор вправе удалить вызов функции memset, если после этого буфер больше не используется

Скоро компиляторы научатся определять, какие программы не будут использоваться. Для начала перестанут компилировать «Hello, world».
Total votes 6: ↑5 and ↓1 +4
Без оптимизатора жить больно. Циклы разворачивать больно. Векторизовать больно. Всё больно.
This comment wasn’t rated yet 0
Интересно, а код pvs-студии прогоняли через неё же? )
Total votes 5: ↑2 and ↓3 -1
А не пора ли обновить список ошибок в Qt? 4 года прошло!
Total votes 1: ↑1 and ↓0 +1
Согласен! Интересно посмотреть на повторный анализ Qt!)
This comment wasn’t rated yet 0
Ok, пометим себе, но не обещаю.
Total votes 1: ↑1 and ↓0 +1
Android работает на linux, а на linux по-умолчанию (когда включён overcommit) malloc никогда не возвращает NULL. Проверять на NULL возвращаемое значение в таком случае бессмысленно. Если память на самом деле «не выделилась» к моменту её использования то система убъёт процесс. Никакого всё равно NULL не будет.
Total votes 3: ↑1 and ↓2 -1
А мужики-то и не знают:
$ cat test.c && gcc -O3 test.c -o test && ./test
#include <stdio.h>
#include <stdlib.h>

int main() {
  void* p = malloc(1000000000000);
  if (p == NULL) {
    printf("Ooops\n");
  }
}

Ooops
Да, вы правы, не всегда можно поймать неиспользуемую память на malloc… но говорить, что он вот совсем-совсем никогда не возвращает NULL — всё-таки некорректно.
Total votes 8: ↑8 and ↓0 +8
#if GENERIC_TARGET
Дело в том, что макрос GENERIC_TARGET не определён

А на #if с неопределенным макросом анализатор указывает?
This comment wasn’t rated yet 0
Нет. Зачем? Неопределённый макрос — это стандартный паттерн программирования. Мы стараемся подобных беспощадных диагностик не делать.
This comment wasn’t rated yet 0
Э-э-э… «Стандартный паттерн» — это #ifdef/#ifndef. Использование в #if макроса, который может быть не определен — явный моветон, ибо легко пропустить/исказить символ в имени, и большинство компиляторов не обратит на это внимания. Слава богу, MS VC++ давно имеет на этот случай отдельное предупреждение.
This comment wasn’t rated yet 0
Объясните, пожалуйста, как #ifdef/#ifndef спасёт от того, что вы пропустили/исказили символ в имени.
Total votes 1: ↑1 and ↓0 +1
#ifdef/#ifndef от этого, разумеется, не спасает. Именно поэтому для повышения надежности и применяют #if в сочетании с соответствующим предупреждением.
This comment wasn’t rated yet 0
С такими ситуациями, к сожалению, ничего нельзя сделать
Вот это неверно. Человек же может распознать, что ошибки нет, значит, можно и анализатор научить.

Но гораздо проще написать «тут ничего нельзя сделать», чем признаться, что для отлова таких ошибок надо полностью переделывать концепцию, обрабатывая исходники до препроцессора.
Total votes 3: ↑0 and ↓3 -3
UFO just landed and posted this here
Жаль, не указали версию Android, которую проверяли.
This comment wasn’t rated yet 0
Версия не имеет значения. Естественно проверялось самое свежее. Статья демонстрирует возможности методологии статического анализа, а не является справочником по конкретным ошибкам в конкретной версии :). Разработчикам рационально самостоятельно проводить анализ и изучать отчёт.
This comment wasn’t rated yet 0
UFO just landed and posted this here
Анализатор PVS-Studio выполняет анализ потока данных (пример) и символьные вычисления. Символьные вычисления, например, позволяют найти следующее бессмысленное условие:
void F(int a, int b, int c)
{
  if (a > b)
    if (b > c)
      if (a < c)  // V547 Expression 'a < c' is always false.
      {}
}

Также всем желающим предлагаю познакомиться с докладом моего коллеги Павла Беликова "Как работает анализ Data Flow в статическом анализаторе кода".

Что касается, приведённого примера, да здесь пока в PVS-Studio ложное срабатывание. Его избежать можно, но сложно. Со временем сделаем.
This comment wasn’t rated yet 0
UFO just landed and posted this here
Спасибо за интересный вопрос. Сейчас объясню.
зачем включать это для пользовательских функций?
Это позволяет находить ошибки даже при использовании тех функций, о которых анализатор ничего не знает. Как именно работает эта эмпирическая диагностика, описано в документации: V666.

Здесь представлены примеры ошибок, связанные с неправильным использованием пользовательских функций. Обратите внимание на проекты: Geant4, OpenSSL, Spring Engine, Bind.

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

В данном случае, для подавления ложных срабатываний я рекомендую использовать рядом с объявлением функции комментарий "//-V:cml_find:666" или полностью отключить диагностику V666. Подробнее о работе с ложными срабатываниями.
This comment wasn’t rated yet 0
когда PVS «разучится» видеть несуществующие проблемы в таком коде

Подобные анализаторы сами по себе никогда не должны «разучаться видеть» любые потенциальные проблемы. Должна быть только возможность динамически отключать предупреждения. Анализатор (как и любой хороший компилятор) должен уметь увидеть все, что хоть как-то способно создать помеху работоспособности.
This comment wasn’t rated yet 0
UFO just landed and posted this here
Я думаю, у Вас весьма завышенные ожидания, особенно от Cppcheck. Там, где хочется увидеть преимущество, вполне может быть наоборот недоработка и неумение что-то делать. Хороший пример на эту тему: "Почему я не люблю синтетические тесты". Там как раз про PVS-Studio и Cppcheck.
This comment wasn’t rated yet 0
Да, тут действительно нет — это я неверно интерпретировал.

Основной смысл моего высказывания был в том, чтобы анализатор кода мог найти любую потенциальную ошибку, даже весьма нетривиальную. Погасить ложное срабатывание гораздо проще, чем пропустить реальное.
This comment wasn’t rated yet 0
a = b = c = d = e = 0;

Кажется, что b, c, d, e все-таки обнулятся, ведь их значение сразу же используется в следующей операции присваивания как аргумент. В статье указано обратное. К тому же PVS-Studio: V1001 приведена всего один раз и по поводу только 'a' или в логе есть еще сообщения по этому месту.
This comment wasn’t rated yet 0
Анализатор ругается только на 'a'. Если удалить 'a =', он начнёт ругаться на 'b' и так далее.
Да, теоретически, не используется только 'a'. Но на практике, компилятор удаляет всю цепочку присваиваний. Я даже не поленился составить тестовый пример, чтобы показать здесь ассемблерный код. Но пока я добился, что компилятор в силу оптимизаций не превращал вообще всё в тыкву, пример сильно вырос. Поэтому не буду утомлять читателей, просто поверьте, что компилятор (в моём случае Visual C++) удаляет всю цепочку a = b = c = d = e = 0;.
This comment wasn’t rated yet 0

Можно пример в. Ideone или gist виде? Даже большой нетыквенный интересен!

This comment wasn’t rated yet 0
Как насчет такого?
godbolt.org/g/NzaWG4
Если закомментировать 11 строку результат не меняется
Total votes 3: ↑3 and ↓0 +3
Я экспериментировал с подобным кодом в Visual C++
__declspec(noinline) void foo(int n)
{
  int a, b, c, d, e;
  a = 1; b = 2; c = 3; d = 4; e = 5;
  for (int i = 0; i != n; ++i)
  {
    a *= 2; b *= 2; c *= 2; d *= 2; e *= 2;
  }
  printf("%d,%d,%d,%d,%d,%d,%d,%d\n", 11, 22, 33, d, a, c, b, e);
  a = b = c = d = e = 0;
}

int main() {
  for (int i = 0; i != 22; ++i)
    foo(i);
  return 0;
}

image
Total votes 2: ↑2 and ↓0 +2
Интересно, есть ли какая-то польза от подобного рода программ (анализаторов кода) в языках программирования вроде Rust'а, у которых потребность «писать правильно и безопасно» вынесена как идея для самого языка.
This comment wasn’t rated yet 0
Часть ошибок универсальны для любого языка, например ошибки копипаста, ошибки в логических условиях (всегда правда или всегда ложь). Польза всегда будет.
Total votes 6: ↑6 and ↓0 +6

Кстати, удивительно, но rust не ловит тривиальные условия (if a < 1 && a > 1 {}). Пойду зарепорчу фичереквест.

This comment wasn’t rated yet 0
На собеседовании в качестве одного из первых вопросов соискателю я задаю следующий: «Что напечатает функция printf и почему?»

Изредка я даже демонстрирую, что этот код, собранный с помощью Visual C++, выводит на экран «6,5», чем ставлю в полный тупик новичков, слабых знанием и духом :).

какая-то наркомания. Хотя каждому свое.


На Java же логичные:


  • printf("%d,%d", i++, i++) = 5,6
  • printf("%d,%d", ++i, ++i) = 6,7
This comment wasn’t rated yet 0
Java — это язык из совсем другой эпохи и с совсем другим подходом к переносимости.

На самом деле всё логично если вспомнить о том, что cdecl кладёт аргументы в стек именно начиная с последнего. stdcall, кстати, пушит в обратном порядке, но, похоже, MSVC старается оптимизировать именно cdecl…
This comment wasn’t rated yet 0

Я же и говорю — каждому свое. Кто-то кактус жрет, кто-то перешел на кофе..

Total votes 1: ↑0 and ↓1 -1
Кстати, а как давно проводился этот анализ?
Например, ошибка из a2dp_vendor_ldac.cc была исправлена месяц назад (26 июня)…
This comment wasn’t rated yet 0
Где-то месяца 2 назад. К сожалению, меня всё время что-то отвлекало и процесс работы над статьёй в этот раз сильно затянулся.

А то что ошибка была поправлена, лишний раз свидетельствует в пользу регулярного использования анализатора кода. Если бы использовался анализатор, то ошибка была бы исправлена не 1 месяца назад, а 2 месяца назад (или когда он там появилась...).
This comment wasn’t rated yet 0

Интересно, а ваш анализатор учитывает что код может компилироваться с флагом -fno-exeptions? Не будет ли new T выдавать nullptr в таком случае?

This comment wasn’t rated yet 0
Нет, к счастью не будет. Программа упадёт с сообщением об ошибке и всё, если памяти не хватит.

Хотите, чтобы работало с -fno-exeptions? Используйте std::nothrow явно.
Total votes 1: ↑1 and ↓0 +1
Аномалия выявляется в этом коде сразу двумя диагностиками:

V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator: ns != 1 || ns != 1 fingerprint.c 126
V560 CWE-570 A part of conditional expression is always false: ns != 1. fingerprint.c 126
This comment wasn’t rated yet 0
Потому, что правая часть условия всегда будет ложной. Иначе, мы просто не дойдём до этой правой части. См. short-circuit evaluation.
This comment wasn’t rated yet 0

Правильно ли я понимаю, что для "if (x>1 || x>3)" будет создана V560, а для "if (x>3 || x>1)" — не будет?

Total votes 1: ↑1 and ↓0 +1
Конкретно на эти выражения алгоритм выдаёт вообще другие сообщения:
  • if (x>1 || x>3) — V590 Consider inspecting the 'x > 1 || x > 3' expression. The expression is excessive or contains a misprint. consoleapplication2017.cpp 40
  • if (x>3 || x>1) — V590 Consider inspecting the 'x > 3 || x > 1' expression. The expression is excessive or contains a misprint. consoleapplication2017.cpp 47

This comment wasn’t rated yet 0
Хотел спросить в контексте статьи Зло живёт в функциях сравнения что вы думаете про автогенерацию функций сравнения средствами IDE и препроцессоров (например Lombok в случае Java)?
В таком случае опечатки полностью исключаются, минусы которые я вижу в этом решении это непотимальный порядок сравнения полей (генератор не знает что поле devTeam большой массив, а qaTeam маленький массив) и риск со стороны программиста включить в сравнение лишние поля — например id объекта.
This comment wasn’t rated yet 0
Если честно, то ничего не думаю :). Я в силу профессиональной деформации на всё смотрю исключительно с точки зрения паттернов ошибок. :)
This comment wasn’t rated yet 0
Я всегда использую delete для массивов примитивов (ну и unique_ptr a = new int[10]). По моему единственный смысл в delete[] только в вызове vector_deleting_destructor, что не применимо для базовых типов.

В отсутствие конструкторов и деструкторов new и delete аналогичны вызовам
operator delete(operator new(17));

Исключением может быть гипотетическая система в которой new int; и new int[2] реализованы разными алгоритмами, что, по моему, лишено смысла.
This comment wasn’t rated yet 0
Очень прошу прекращать заниматься программированием.
Total votes 3: ↑3 and ↓0 +3
Sign up to leave a comment.

Your account

Sections

Information

Services

Support
© 2006–2024, Habr