Комментарии 88
Ну вот, пофиксят все баги по подсказке из статьи и бета тестерам в Гугл урежут ЗП…
0
Тут всё сложнее.
1) Сделать бранч
2) Заасайнить туда парочку лидов, которые знают этот модуль
3) Зафиксить
4) Прогнать тесты, а может и написать свои тесты (с багами, разумеется)
Возможно, пофиксить багу. И надеяться, что расхождения с тестами нет. Темболее этот код может быть в таких дебрях, что юзается только на 1.5 моделях Lenovo, которые уже не выпускаются.
1) Сделать бранч
2) Заасайнить туда парочку лидов, которые знают этот модуль
3) Зафиксить
4) Прогнать тесты, а может и написать свои тесты (с багами, разумеется)
Возможно, пофиксить багу. И надеяться, что расхождения с тестами нет. Темболее этот код может быть в таких дебрях, что юзается только на 1.5 моделях Lenovo, которые уже не выпускаются.
+1
Было бы круто если бы ваш сервис работал над всем кодом в публичном Github и репродукции всем владельцам репозиториев, но увы )
+2
Было бы круто встроить подобный анализатор во все среды разработки
+1
если бы ваш сервис работал над всем кодом в публичном GithubТогда pvs не сможет зарабатывать, а значит придётся сокращать штат и закрываться, значит продукт перестанет развиваться, значит некому будет делать такие проверки…
+2
Ну если встроить в IDE — работу оплатят авторы IDE, если в Github — владельцы гитхаба и т.п.
-1
А что, travis ci уже закрывается?
0
НЛО прилетело и опубликовало эту надпись здесь
Было бы круто если бы Google выкупил бы PVS-Studio и сделал бы его опенсорс. ))
0
do {
....
if (x) continue;
....
} while (0)
Не исключено что это так и задумывалось, программист хотел избежать использования goto.
+3
Возможно, но тогда намного лучше написать break. И вообще, зачем тогда цикл? Можно было вообще цикл не писать.
+2
Он цикл не использует как цикл. Он проходит по телу якобы «цикла» один раз, используя его как блок, заранее зная что пост-условие всегда ложно. Но ему как-то нужно не имея метки и goto выйти из тела блока. Вот он и делает continue, тщательно избегая использования goto, который стал «табу». А то что можно было использовать break — это да.
Можно даже понять психологию, почему программист поставил continue. Оно представляется как «стрелка» перехода. А вот break — он мыслится как своего рода exception, return, stop. Так как программист задумывал именно goto-переход — он и написал continue.
А вообще интересно, как часто в разных исходниках используют goto?
Можно даже понять психологию, почему программист поставил continue. Оно представляется как «стрелка» перехода. А вот break — он мыслится как своего рода exception, return, stop. Так как программист задумывал именно goto-переход — он и написал continue.
А вообще интересно, как часто в разных исходниках используют goto?
+5
Он цикл не использует как цикл. Он проходит по телу якобы «цикла» один раз, используя его как блок, заранее зная что пост-условие всегда ложно.Все остальные case заканчиваются на return. Если цикл убрать, ничего не изменится.
А вообще интересно, как часто в разных исходниках используют goto?Не смогу ответить. Не ведём такую статистику.
+3
Мелкомягкие, видимо, не гнушаются. В доках встречается:
docs.microsoft.com/en-us/windows/desktop/coreaudio/capturing-a-stream
docs.microsoft.com/en-us/windows/desktop/coreaudio/capturing-a-stream
0
Нет, не так про continue. Там еще switch внутри цикла. break — переход в точку после switch, а continue — в конец цикла. И do используется именно как цикл — в этом if инкрементируется счетчик, который инициализируется перед do. Так что про ошибку все правильно.
+1
А не проще ли тогда было бы написать
?
....
if (!x) {
....
}
?
+2
Давайте уж тогда весь контекст рассмотрим:
Абсолютно очевидно, что идея continue была выполнить еще одну итерацию цикла, который, все правильно, в нормальных условиях должен иметь только один проход
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 была выполнить еще одну итерацию цикла, который, все правильно, в нормальных условиях должен иметь только один проход
+3
Если выносить Switch case в отдельную функцию и использовать в ней return вместо break, то мир становится проще, приятнее и более читабельным.
0
Чем закончилась история с Tizen?
+1
по поводу проверки после выделения памяти. всё обьясняется просто:
для олд-скульных С пересевших на С++, использовали malloc-free и перенесли проверку на выделение уже в С++ new.
А С++, привыкшие к new, скорее всего придерживались уже устоявшегося malloc-free в старых модулях, ну и не заморачиваются с проверками.
Это всё потому что люди пишут на несуществующем языке С/С++ вместо С или С++.
для олд-скульных С пересевших на С++, использовали malloc-free и перенесли проверку на выделение уже в С++ new.
А С++, привыкшие к new, скорее всего придерживались уже устоявшегося malloc-free в старых модулях, ну и не заморачиваются с проверками.
Это всё потому что люди пишут на несуществующем языке С/С++ вместо С или С++.
+3
Правильная проверка должна быть такой:if (f->mode == O_RDONLY && expect_zeros) {
Думаю так правильнее, т.к. mode скорее всего может содержать дополнительные флаги, вроде O_NOFOLLOW
if (f->mode & O_RDONLY == O_RDONLY && expect_zeros) {
0
f->mode & O_RDONLY == O_RDONLYИ получается, что 0 == 0. Т.е. условие всегда истинно. «Отличная» правка :).
+2
Конец рабочего дня даёт о себе знать :) Тем не менее, просто сравнивать с O_RDONLY тоже некорректно из-за возможных дополнительных флагов
0
А мне почему-то кажется, что имелось в виду что-то такое
и проверка
должна была проверять разрешённость именно чтения. Хотя комментарий нам и говорит явно про «read-only», он мог писаться постфактум и быть просто навеян названием константы O_RDONLY. Тут без остального контекста сложно понять. Хотя, конечно, Ваше предположение выглядит проще.
#define O_NORIGHTS 0
#define O_READ 1
#define O_WRITE 2
#define O_READWRITE 3
и проверка
f->mode & O_READ
должна была проверять разрешённость именно чтения. Хотя комментарий нам и говорит явно про «read-only», он мог писаться постфактум и быть просто навеян названием константы O_RDONLY. Тут без остального контекста сложно понять. Хотя, конечно, Ваше предположение выглядит проще.
0
Т.е., оперируя старыми названиями констант, тянет заменить
скорее на
if (f->mode & O_RDONLY && expect_zeros)
скорее на
if ( (f->mode == O_RDONLY || f->mode == O_RDWR) && expect_zeros)
+1
Успокойтесь, всё уже придумано за нас. Имелось в виду
Человеку легко догадаться, что где-то там должна быть маска и найти её посмотрев в .h файл (и то, как видим не всем). А вот роботу…
if (f->mode & O_ACCMODE == O_RDONLY && expect_zeros)
Хороший пример для Andrey2008, когда он отбивается на тему: «ошибки мы нашли, а вот как правильно их пофиксить — это уже разработчики думать нужны».Человеку легко догадаться, что где-то там должна быть маска и найти её посмотрев в .h файл (и то, как видим не всем). А вот роботу…
+2
компилятор вправе удалить вызов функции memset, если после этого буфер больше не используется
Скоро компиляторы научатся определять, какие программы не будут использоваться. Для начала перестанут компилировать «Hello, world».
+4
Интересно, а код pvs-студии прогоняли через неё же? )
-1
Мощно!
0
Android работает на linux, а на linux по-умолчанию (когда включён overcommit) malloc никогда не возвращает NULL. Проверять на NULL возвращаемое значение в таком случае бессмысленно. Если память на самом деле «не выделилась» к моменту её использования то система убъёт процесс. Никакого всё равно NULL не будет.
-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 — всё-таки некорректно.+8
#if GENERIC_TARGET
Дело в том, что макрос GENERIC_TARGET не определён
А на #if с неопределенным макросом анализатор указывает?
0
Нет. Зачем? Неопределённый макрос — это стандартный паттерн программирования. Мы стараемся подобных беспощадных диагностик не делать.
0
Э-э-э… «Стандартный паттерн» — это #ifdef/#ifndef. Использование в #if макроса, который может быть не определен — явный моветон, ибо легко пропустить/исказить символ в имени, и большинство компиляторов не обратит на это внимания. Слава богу, MS VC++ давно имеет на этот случай отдельное предупреждение.
0
С такими ситуациями, к сожалению, ничего нельзя сделатьВот это неверно. Человек же может распознать, что ошибки нет, значит, можно и анализатор научить.
Но гораздо проще написать «тут ничего нельзя сделать», чем признаться, что для отлова таких ошибок надо полностью переделывать концепцию, обрабатывая исходники до препроцессора.
-3
НЛО прилетело и опубликовало эту надпись здесь
Жаль, не указали версию Android, которую проверяли.
0
НЛО прилетело и опубликовало эту надпись здесь
Анализатор PVS-Studio выполняет анализ потока данных (пример) и символьные вычисления. Символьные вычисления, например, позволяют найти следующее бессмысленное условие:
Также всем желающим предлагаю познакомиться с докладом моего коллеги Павла Беликова "Как работает анализ Data Flow в статическом анализаторе кода".
Что касается, приведённого примера, да здесь пока в 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 ложное срабатывание. Его избежать можно, но сложно. Со временем сделаем.
0
НЛО прилетело и опубликовало эту надпись здесь
Спасибо за интересный вопрос. Сейчас объясню.
Здесь представлены примеры ошибок, связанные с неправильным использованием пользовательских функций. Обратите внимание на проекты: Geant4, OpenSSL, Spring Engine, Bind.
По поводу ложных срабатываний. Мы много работаем, чтобы уменьшить их количество. Однако, они были, есть и будут.
В данном случае, для подавления ложных срабатываний я рекомендую использовать рядом с объявлением функции комментарий "//-V:cml_find:666" или полностью отключить диагностику V666. Подробнее о работе с ложными срабатываниями.
зачем включать это для пользовательских функций?Это позволяет находить ошибки даже при использовании тех функций, о которых анализатор ничего не знает. Как именно работает эта эмпирическая диагностика, описано в документации: V666.
Здесь представлены примеры ошибок, связанные с неправильным использованием пользовательских функций. Обратите внимание на проекты: Geant4, OpenSSL, Spring Engine, Bind.
По поводу ложных срабатываний. Мы много работаем, чтобы уменьшить их количество. Однако, они были, есть и будут.
В данном случае, для подавления ложных срабатываний я рекомендую использовать рядом с объявлением функции комментарий "//-V:cml_find:666" или полностью отключить диагностику V666. Подробнее о работе с ложными срабатываниями.
0
когда PVS «разучится» видеть несуществующие проблемы в таком коде
Подобные анализаторы сами по себе никогда не должны «разучаться видеть» любые потенциальные проблемы. Должна быть только возможность динамически отключать предупреждения. Анализатор (как и любой хороший компилятор) должен уметь увидеть все, что хоть как-то способно создать помеху работоспособности.
0
НЛО прилетело и опубликовало эту надпись здесь
Я думаю, у Вас весьма завышенные ожидания, особенно от Cppcheck. Там, где хочется увидеть преимущество, вполне может быть наоборот недоработка и неумение что-то делать. Хороший пример на эту тему: "Почему я не люблю синтетические тесты". Там как раз про PVS-Studio и Cppcheck.
0
Да, тут действительно нет — это я неверно интерпретировал.
Основной смысл моего высказывания был в том, чтобы анализатор кода мог найти любую потенциальную ошибку, даже весьма нетривиальную. Погасить ложное срабатывание гораздо проще, чем пропустить реальное.
Основной смысл моего высказывания был в том, чтобы анализатор кода мог найти любую потенциальную ошибку, даже весьма нетривиальную. Погасить ложное срабатывание гораздо проще, чем пропустить реальное.
0
a = b = c = d = e = 0;
Кажется, что b, c, d, e все-таки обнулятся, ведь их значение сразу же используется в следующей операции присваивания как аргумент. В статье указано обратное. К тому же PVS-Studio: V1001 приведена всего один раз и по поводу только 'a' или в логе есть еще сообщения по этому месту.
0
Анализатор ругается только на 'a'. Если удалить 'a =', он начнёт ругаться на 'b' и так далее.
Да, теоретически, не используется только 'a'. Но на практике, компилятор удаляет всю цепочку присваиваний. Я даже не поленился составить тестовый пример, чтобы показать здесь ассемблерный код. Но пока я добился, что компилятор в силу оптимизаций не превращал вообще всё в тыкву, пример сильно вырос. Поэтому не буду утомлять читателей, просто поверьте, что компилятор (в моём случае Visual C++) удаляет всю цепочку a = b = c = d = e = 0;.
Да, теоретически, не используется только 'a'. Но на практике, компилятор удаляет всю цепочку присваиваний. Я даже не поленился составить тестовый пример, чтобы показать здесь ассемблерный код. Но пока я добился, что компилятор в силу оптимизаций не превращал вообще всё в тыкву, пример сильно вырос. Поэтому не буду утомлять читателей, просто поверьте, что компилятор (в моём случае Visual C++) удаляет всю цепочку a = b = c = d = e = 0;.
0
Можно пример в. Ideone или gist виде? Даже большой нетыквенный интересен!
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;
}
+2
Интересно, есть ли какая-то польза от подобного рода программ (анализаторов кода) в языках программирования вроде Rust'а, у которых потребность «писать правильно и безопасно» вынесена как идея для самого языка.
0
На собеседовании в качестве одного из первых вопросов соискателю я задаю следующий: «Что напечатает функция printf и почему?»
Изредка я даже демонстрирую, что этот код, собранный с помощью Visual C++, выводит на экран «6,5», чем ставлю в полный тупик новичков, слабых знанием и духом :).
какая-то наркомания. Хотя каждому свое.
На Java же логичные:
- printf("%d,%d", i++, i++) = 5,6
- printf("%d,%d", ++i, ++i) = 6,7
0
Кстати, а как давно проводился этот анализ?
Например, ошибка из a2dp_vendor_ldac.cc была исправлена месяц назад (26 июня)…
Например, ошибка из a2dp_vendor_ldac.cc была исправлена месяц назад (26 июня)…
0
Где-то месяца 2 назад. К сожалению, меня всё время что-то отвлекало и процесс работы над статьёй в этот раз сильно затянулся.
А то что ошибка была поправлена, лишний раз свидетельствует в пользу регулярного использования анализатора кода. Если бы использовался анализатор, то ошибка была бы исправлена не 1 месяца назад, а 2 месяца назад (или когда он там появилась...).
А то что ошибка была поправлена, лишний раз свидетельствует в пользу регулярного использования анализатора кода. Если бы использовался анализатор, то ошибка была бы исправлена не 1 месяца назад, а 2 месяца назад (или когда он там появилась...).
0
Интересно, а ваш анализатор учитывает что код может компилироваться с флагом -fno-exeptions? Не будет ли new T выдавать nullptr в таком случае?
0
Аномалия выявляется в этом коде сразу двумя диагностиками:
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
0
Почему возникает V560 ?
0
Потому, что правая часть условия всегда будет ложной. Иначе, мы просто не дойдём до этой правой части. См. short-circuit evaluation.
0
Правильно ли я понимаю, что для "if (x>1 || x>3)" будет создана V560, а для "if (x>3 || x>1)" — не будет?
+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
0
Как всегда крутая статья. Не хотите проверить c++ части github.com/facebook/react-native и github.com/facebook/yoga. Возможно будет интересно сравнить с xamarin forms или qt.
0
Хотел спросить в контексте статьи Зло живёт в функциях сравнения что вы думаете про автогенерацию функций сравнения средствами IDE и препроцессоров (например Lombok в случае Java)?
В таком случае опечатки полностью исключаются, минусы которые я вижу в этом решении это непотимальный порядок сравнения полей (генератор не знает что поле devTeam большой массив, а qaTeam маленький массив) и риск со стороны программиста включить в сравнение лишние поля — например id объекта.
В таком случае опечатки полностью исключаются, минусы которые я вижу в этом решении это непотимальный порядок сравнения полей (генератор не знает что поле devTeam большой массив, а qaTeam маленький массив) и риск со стороны программиста включить в сравнение лишние поля — например id объекта.
0
Я всегда использую delete для массивов примитивов (ну и unique_ptr a = new int[10]). По моему единственный смысл в delete[] только в вызове vector_deleting_destructor, что не применимо для базовых типов.
В отсутствие конструкторов и деструкторов new и delete аналогичны вызовам
operator delete(operator new(17));
Исключением может быть гипотетическая система в которой new int; и new int[2] реализованы разными алгоритмами, что, по моему, лишено смысла.
В отсутствие конструкторов и деструкторов new и delete аналогичны вызовам
operator delete(operator new(17));
Исключением может быть гипотетическая система в которой new int; и new int[2] реализованы разными алгоритмами, что, по моему, лишено смысла.
0
del
0
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален