Комментарии 163
3. Цель статьи — реклама PVS-Studio.
Обычно такие фразы являются последними и пользователь закрывает статью и читает что-то другое. Уже привык что реклама не может быть интересной по определению.
Но ваша статья действительна интересна, даже если не работаешь на С++, все равно интересно видеть узкие места для того что бы в будущем следить за ними.
Спасибо за статью, побольше бы такой рекламы!
Думаю, в данном случае эта фраза даже лишняя. Давно пора привыкнуть к тому, что статьи про PVS-Studio это реклама.
Не лишняя, не все понимают, судя по комментариям. Регулярно встречаются люди, которые считают, что разработчики им должны что-то, потому что они взяли их проект для проверки.
НЛО прилетело и опубликовало эту надпись здесь
Если вдаваться в детали, то это стоит воспринимать не как демонстрацию возможностей программы, а скорее как демонстрацию проделанной человеком работы. Потому что проанализировать over9000 предупреждений и отобрать из них значимые — довольно большая работа, как мне кажется. Стоит только гадать, сколько времени на это ушло при анализе ядра Linux.
НЛО прилетело и опубликовало эту надпись здесь
На беглый осмотр предупреждений и выписывание примеров для статьи у меня ушло около 6 часов чистого времени. Точно не засекал.
а на получение предупреждений?
Дольше. Как и написание статьи
Призываю SvyatoslavMC для ответа на вопрос. Получение лога это полностью его заслуга.
Призываю SvyatoslavMC для ответа на вопрос. Получение лога это полностью его заслуга.
Сборка ядра в виртуальной машине длилась примерно 5 часов, следовательно, в процессе проверки на каждый процесс комплиятора запускалось по 2 процесса: препроцессирование и анализ. Примерное время было оценено в 1,5-2 раза больше сборки, поэтому всё было запущено на ночь. С утра мы открыли лог в PVS-Sutuio Standalone и начали анализ результатов.
Нет, я про время, потраченное до запуска сбора :)
а на получение предупреждений?
На каждом препроцессированном файле запускался процесс PVS-Studio, и каждый такой процесс писал предупреждения в результирующий лог, один для всех. Если это не есть получение предупреждений, то поясните.
Для получения предупреждений надо было написать скрипт, заставить работать систему сборки с ним — я про это время спрашивал.
Ну так про это в начале статьи написано.[Resolved]
там не указано время, на это потраченное :)
что именно было сделано написано, а сколько времени ушло?
что именно было сделано написано, а сколько времени ушло?
Время сбора параметров компиляции = время сборки. Время препроцессирования и анализа каждого файла = ~2*(время сборки). Ни одного скрипта не использовалось.
Зачем выводить «незначимые» предупреждения (и что это вообще такое)?
Спасибо за статью, побольше бы такой рекламы!Ну не все согласны. :) Мои статьи временами вызывают жуткий butthurt.
Интересно, почему разработчики того же ядра или другие люди связанные с Linux, свободной, чистокровно открытой платформой не гнушаются Coverity, с помощью которой постоянно проверяет ядро мейнтейнерами? Ведь это чистая воды поприетарщина которая от вас отличается лишь масштабом финансирования, да да, у них гранты для проверки опенсорса, и есть свой сервис которые это делает и выдает только результат. Когда вы пишите достаточно интересные статьи. Я понимаю вам просто денег и соответственно времени нет чтобы рекламироваться в таком масштабе, а делаете это по своему.
Но я не понимаю этих людей которым выдают результаты с указанием на ошибки, а они это признают рекламой и отказываются от них только из-за этого. Жесть.
Но я не понимаю этих людей которым выдают результаты с указанием на ошибки, а они это признают рекламой и отказываются от них только из-за этого. Жесть.
Не уверен, что эти комментаторы с reddit являются разработчиками ядра.
почему разработчики того же ядра или другие люди связанные с Linux, свободной, чистокровно открытой платформой не гнушаются Coverity, с помощью которой постоянно проверяет ядро мейнтейнерами? Ведь это чистая воды поприетарщина которая от вас отличается лишь масштабом финансирования
Coverity отличается от PVS-Studio очень сильно, а не «лишь масштабом финансирования». Coverity пишет предупреждения в стиле «А вот если условие в if (...) в строке 120 будет true, а счетчик i в for (i =… ) в строке 130 будет 7, то в строке 140 в выражении a[i] у вас будет выход за границы массива, потому что массив объявлен в строке 50 как int a[N], а N определен в строке 10 как int N = 5». PVS-Studio так делать не умеет.
То что вы написали и есть следствие причины.
Ваше описание очень напомнило мне html-лог статического анализатора Clang. Все его собщения выглядят как «если… если… если», довольно не просто анализировать такие результаты, и тем более доказать ошибку, потому что на примере проекта Wine, я видел несколько сотен сообщений, которые связаны с пропуском какой-нибудь логики в операторах «if» или «switch», но раз таких мест реально столько много, то, возможно, не предполагается, что этот код получает управление. По моему опыту общего у Clang и PVS-Studio не нашлось, поэтому статья была крупная и разносторонняя, но с PVS-Studio на много легче. Coverity честно не видел, но приведёный вами пример — один в один Clang.
Если Tatyanazaxarova — сотрудник, то такие посты нарушение правил реддита.
Наконец-то написали на lkml. Все ждал когда там появится ссылка на статью. Интересно будет почитать комментарии разрабов.
lkml.org/lkml/2015/1/3/54
lkml.org/lkml/2015/1/3/54
Имхо наибольший butthurt вызвало качество перевода (я тут не специалист, на много англоязычного народу жалуется что трудно читать — попробуйте Grammarly или может стоит воспользоваться услугами тех.переводчиков и/или пруфридеров) и нарушение правил Reddit'a (может действительно стоит оформлять статью как sponsoring link в таком случае или не публиковаться на реддите).
Согласен. Не увидел там баттхерта, пара человек написала, что их напряг плохой английский и что сложно было читать статью.
Еще пара написала, что это должен быть sponsor link.
Перевод для русского человека выглядит нормальным, но для англоязычного… Там беда с временем и с порядком слов.
Аналог на русском:
Что проверили мы
Мы брать линукс ядро с…
Последнее стабильное ядро было проверять
Пока я писать эту статью, ядро 3.19-rc1 было уже разработать.
И так далее.
Еще пара написала, что это должен быть sponsor link.
Перевод для русского человека выглядит нормальным, но для англоязычного… Там беда с временем и с порядком слов.
Аналог на русском:
Что проверили мы
Мы брать линукс ядро с…
Последнее стабильное ядро было проверять
Пока я писать эту статью, ядро 3.19-rc1 было уже разработать.
И так далее.
«Linux kernel source code was checked and is checked by everything and anything.» — англоговорящему такие обороты как by everything and anything реально голову взрывают ))
Не говоря уж о грамматических ошибках типа «there are no possibility».
Зря сэкономили на вычитке английского текста!
Не говоря уж о грамматических ошибках типа «there are no possibility».
Зря сэкономили на вычитке английского текста!
Это нас выдрессировали безвкусной рекламой повсюду. А так — посетите как-нибудь, или посмотрите записи «ночь пожирателей рекламы».
Вообще в подобных случаях достаточно написать в начале параграф «Раскрытие принадлежности» («Disclosures of Affiliation»), где явно указать, что авторами статьи являются представители заинтересованной стороны, а целью — демонстрация возможностей программного продукта. Явное указание целей статьи — залог верного восприятия адекватным читателем и подспорье в ответах неадекватному.
Слово «реклама» употреблять в таком случае совсем необязательно, т.к. оно несёт негативную коннотацию (в т.ч.: гиперболизация, искажение фактов, эмоциональность в ущерб информативности и пр.).
С другой стороны, любое маркетинговое усилие должно учитывать возможные последствия для репутации. Максимизация полезной отдачи для адресной аудитории (FOSS-сообщества в данном случае) от каждой подобной статьи помогает формировать позитивный имидж продукта/компании. Если, к примеру, отправлять подробные багрепорты после каждой статьи и способствовать максимально быстрому исправлению найденных ошибок, ну и, конечно, внимательнее подходить к переводу статьи на иностранные языки, серия статей получит куда более позитивный отклик и, в итоге, больший охват. Не утверждаю, впрочем, что это верная стратегия для всех случаев.
Слово «реклама» употреблять в таком случае совсем необязательно, т.к. оно несёт негативную коннотацию (в т.ч.: гиперболизация, искажение фактов, эмоциональность в ущерб информативности и пр.).
С другой стороны, любое маркетинговое усилие должно учитывать возможные последствия для репутации. Максимизация полезной отдачи для адресной аудитории (FOSS-сообщества в данном случае) от каждой подобной статьи помогает формировать позитивный имидж продукта/компании. Если, к примеру, отправлять подробные багрепорты после каждой статьи и способствовать максимально быстрому исправлению найденных ошибок, ну и, конечно, внимательнее подходить к переводу статьи на иностранные языки, серия статей получит куда более позитивный отклик и, в итоге, больший охват. Не утверждаю, впрочем, что это верная стратегия для всех случаев.
Всё впорядке
static int sha384_ssse3_final(struct shash_desc *desc, u8 *hash)
{
u8 D[SHA512_DIGEST_SIZE];
...
memset(D, 0, SHA512_DIGEST_SIZE);
...
}
Поясните.
Ну у вас сообщение — «Опасное использование memset», Которое подразумивает то, что может быть задета память не относящаяся к переменной D. Но в данном случае это не так, размер D такой же как и тот что задается в memset. А то что он больше нужного размера для этой ф-ии это совсем другая история.
P.S. Минусующии, объяснитесь пожалуйста.
P.S. Минусующии, объяснитесь пожалуйста.
Вы статью точно внимательно прочитали? Там же после этого куска кода написано:
«Опасное использование memset» это лишь общее описание типов ошибок. В заголовке ведь не указано точный тип ошибок, верно?
Или в вашем понимании ошибки memset могут быть лишь связанные с порчей памяти?
Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'D' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha512_ssse3_glue.c 222
«Опасное использование memset» это лишь общее описание типов ошибок. В заголовке ведь не указано точный тип ошибок, верно?
Или в вашем понимании ошибки memset могут быть лишь связанные с порчей памяти?
RtlSecureZeroMemory в линуксе?!
Вы статью точно внимательно прочитали? Там же после этого куска кода написано:
PVS-Studio неуместно рекомендует использовать функцию RtlSecureZeroMemory(), так как ориентирован на Windows. В Linux такой функции, конечно, нет. Но здесь главное предупредить, а подобрать нужную функцию уже не сложно.
Сколько раз уже давал ссылку на свой пост по этому поводу. Прочитайте.
Думаю в ядре имеются, похожие на те что в комменте, макросы или функции реализации подобного функционала. Или вы хотите сказать что это только лишь Windows-specific компиляторов (msvc) трабла?
Думаю в ядре имеются, похожие на те что в комменте, макросы или функции реализации подобного функционала. Или вы хотите сказать что это только лишь Windows-specific компиляторов (msvc) трабла?
Ну и для всех кто заинтересовался данным неприятным моментом с оптимизациями компилятора рекомендую прочитать хорошую статью на эту тему:
«MSC06-C. Beware of compiler optimizations»
www.securecoding.cert.org/confluence/display/seccode/MSC06-C.+Beware+of+compiler+optimizations
п.с. карму подпортили хз почему, ссылку пришлось так вставить.
И убедится в том что это не потенциальная угроза, а вполне себе реальная.
Для затравки оттуда:
Subclause 5.1.2.3 of the C Standard [ISO/IEC 9899:2011] states:
In the abstract machine, all expressions are evaluated as specified by the semantics. An actual implementation need not evaluate part of an expression if it can deduce that its value is not used and that no needed side effects are produced (including any caused by calling a function or accessing a volatile object).
This clause gives compilers the leeway to remove code deemed unused or unneeded when building a program. Although this functionality is usually beneficial, sometimes the compiler removes code that it thinks is not needed but has been added for a specific (often security-related) purpose.
«MSC06-C. Beware of compiler optimizations»
www.securecoding.cert.org/confluence/display/seccode/MSC06-C.+Beware+of+compiler+optimizations
п.с. карму подпортили хз почему, ссылку пришлось так вставить.
И убедится в том что это не потенциальная угроза, а вполне себе реальная.
Для затравки оттуда:
Subclause 5.1.2.3 of the C Standard [ISO/IEC 9899:2011] states:
In the abstract machine, all expressions are evaluated as specified by the semantics. An actual implementation need not evaluate part of an expression if it can deduce that its value is not used and that no needed side effects are produced (including any caused by calling a function or accessing a volatile object).
This clause gives compilers the leeway to remove code deemed unused or unneeded when building a program. Although this functionality is usually beneficial, sometimes the compiler removes code that it thinks is not needed but has been added for a specific (often security-related) purpose.
(Надо на такой случай будет заказать нарисовать картинку facepalm с участием единорога)
Предлагаю прочитать статью.
А вообще это очень хорошо демонстрирует пользу от статического анализа. Не видит человек ошибок, не видит. Правда печалит, даже если явно в статье рассказано про ошибку, всё равно её не видят. Боюсь многие полезные диагностики анализатора списываются как ложные предупреждения. Эх.
Предлагаю прочитать статью.
А вообще это очень хорошо демонстрирует пользу от статического анализа. Не видит человек ошибок, не видит. Правда печалит, даже если явно в статье рассказано про ошибку, всё равно её не видят. Боюсь многие полезные диагностики анализатора списываются как ложные предупреждения. Эх.
Да не то оно подразумевает!
ПРОЧИТАЙТЕ СТАТЬЮ!
Вкратце: компилятор видит последний memset и может в целях оптимизации его выкинуть.
Значит, в памяти останется шматок дайджеста.
От этого ничего нигде не упадёт.
Но если хакер каким-то образом доберётся до памяти — он может целенаправленно разыскать такой шматок и использовать в своих грязных целях. В этом и состоит опасность!
ПРОЧИТАЙТЕ СТАТЬЮ!
Вкратце: компилятор видит последний memset и может в целях оптимизации его выкинуть.
Значит, в памяти останется шматок дайджеста.
От этого ничего нигде не упадёт.
Но если хакер каким-то образом доберётся до памяти — он может целенаправленно разыскать такой шматок и использовать в своих грязных целях. В этом и состоит опасность!
Решето!
Andrey2008, а вы последний мой комментарий здесь видели? Если не сочли нужным отвечать, то ладно, просто вдруг не заметили :-)
Эта статья хорошая. Были интересные ошибки, которых я раньше в ваших статьях не видел.
Интересно, а V519 срабатывает только если данные присваивания идут совсем подряд или между ними может быть другой код? А если там вызов функции, будет сработка? Вроде такого кода:
Эта статья хорошая. Были интересные ошибки, которых я раньше в ваших статьях не видел.
Интересно, а V519 срабатывает только если данные присваивания идут совсем подряд или между ними может быть другой код? А если там вызов функции, будет сработка? Вроде такого кода:
a->b = fn1();
a->b = fn2();У меня цифр под рукой нет. Всё в офисе. Решил, что отвечу после праздников.
По поводу диагностики V519. Она «звучит» в описании так:
По поводу диагностики V519. Она «звучит» в описании так:
/*
Генерирует предупреждение V519.
Одному и тому же объекту дважды подряд присваиваются значения.
Высока вероятность опечатки.
Пример:
x = 1;
x = 2;
Исключения:
1) Объект участвует во втором выражении в правой части:
x = 1;
x = x + Foo();
2) Объект является результатом выражения с участием функции.
(На самом деле отбрасываем больше случаев. Если слева выражение,
которое представляет не унарную операцию разыменования '*' или
не обращение к элементу массива - то мы не анализируем дальше).
GetRef() = 1;
GetRef() = 1;
3) Слева используется инкремент или декремент.
*p++ = 1;
*p++ = 1;
4) Для индексации к элементам используется неконстантное выражение.
int i;
p[i] = 1;
Foo(); //Foo может менять i.
p[i] = 2;
5) Не ругаться, если первый раз переменной типа указатель (или класс) присваивается 0.
p = nullptr;
p = new int[n];
6) Если слева переменная имеет особые исключительные имена. Имена:
gLibNcFTP_Uses_Me_To_Quiet_Variable_Unused_Warnings (используетсяв макросе LIBNCFTP_USE_VAR)
7) Если в одном блоке более одной ситуации, когда подряд присваивается значение одной и
той-же переменной, то это безопасно. Пример:
res = RegQueryValueEx(...);
res = RegQueryValueEx(...);
res = RegQueryValueEx(...);
8) Если первая строка это объявление переменной.
Если в первой строке мы объявляем ссылку:
int &x = a;
x = 2;
9) Если первая строка это объявление переменной. И это не Си файл.
10) Если первая строка это объявление переменной.
Инициализируем литералом/константой 0, -1 или "".
Возможно, есть приведение к какому-то типу: HRESULT hr = ((HRESULT)0L);
11) Если между двумя присваиваниями упоминается данный объект:
A = 10;
x = A;
A = 20;
12) Если между двумя присваиваниями имеется:
*case;
*default;
*break;
*continue;
*goto
*return
*delete
*высказывание ((void)0); Такое бывает, когда исчезает assert().
*пустое высказывание ';'. Такое бывает, когда исчезает что-то типа VivaAssert().
*throw
13) Если между двумя присваиваниями имеется вызов неитерабельной функции и при этом объект
является глобальным (или ссылкой, или статической переменной,
и мы не разыменовываем указатель или брался адрес этой переменной ).
Примеры:
1) A = B; A = FOO();
2) A = B; FOO(); A = C;
Исключение из исключения, случай, когда справа одинаковый вызов функции:
A = z.F(1);
A = z.F(1); //Скорее всего это copy-paste.
В общем Исключение N13 не срабатывет, если 2 строки совпадают.
3) И ещё отдельно рассматриваем такой случай:
temp = strchr(hoststr, '/');
*temp = '\0';
hostlen = strlen( hoststr );
*temp = '/';
14) Между двумя присваиваниями есть #if/#else/#endif
15) Всегда безопасно, если первый раз присваиваем -1.
При этом между первым и вторым присваиванием есть хоть один вызов функции или оператор new.
16) Создаётся список с помощью выражения вида: t = t->next = ptr;
Не ругаемся, если записываем результат присваивания в переменную A, являющуюся левой частью для A->B.
Уровни:
По умолчанию уровень 2.
Если первая строка это объявление переменной - уровень 3.
На 3 уровень, если переменная имеет имя: hr, hres, err, result
Если это массив и строки рядом, то на 1 уровень.
*/Во, спасибо, это многое проясняет. На мой взгляд этот комментарий даже больше несёт рекламной пользы, чем сама статья. По нему видно, сколько работы проделано за какой-то диагностикой. Бывает сравнительно легко написать детектор нового вида багов, но потом начинается самая сложная работа — найти идеальный баланс между false-positive и false-negative и расставить приоритеты.
Мне кажется, было бы круто написать цикл статей в другом разрезе — не про конкретный опенсорсный проект со списком багов, а про конкретный вид багов с примерами из разных проектов. Рассказать хотя бы вкратце, откуда взялись наиболее интересные исключения. Конкретно здесь можно рассказать, что такое неитерабельные функции и как вы определяете, может ли Foo менять i. Показать, какие ложные сработки тем не менее остались и какие полезные сработки вы потеряли, введя исключения. Думаю, помимо рекламной пользы написание такой статьи позволит взглянуть на старый детектор свежим взглядом и, возможно, доработать его :-)
Мне кажется, было бы круто написать цикл статей в другом разрезе — не про конкретный опенсорсный проект со списком багов, а про конкретный вид багов с примерами из разных проектов. Рассказать хотя бы вкратце, откуда взялись наиболее интересные исключения. Конкретно здесь можно рассказать, что такое неитерабельные функции и как вы определяете, может ли Foo менять i. Показать, какие ложные сработки тем не менее остались и какие полезные сработки вы потеряли, введя исключения. Думаю, помимо рекламной пользы написание такой статьи позволит взглянуть на старый детектор свежим взглядом и, возможно, доработать его :-)
Респект. Мне больше всего понравился пункт 6. Видно, что разрабы реально хотят пофиксить все репорты. Так держать!
Как найду время, куплю ваш продукт для нас. Но надо найти время, чтобы его встроить в наш workflow, а это, как у всех, непросто сделать…
Как найду время, куплю ваш продукт для нас. Но надо найти время, чтобы его встроить в наш workflow, а это, как у всех, непросто сделать…
Предвижу возражение в духе: «но ведь работает!». Возможно и работает. Но, я думаю, ядро Linux это не то место, где можно полагаться на такой подход. Код лучше переписать.
Постоянно просматриваю ваши статьи и не раз видел подобное, в большинстве случаев, так же как с выкидыванием проверок на NULL при оптимизации(например), чаще всего это выглядит как оправдание не профессионализма или лени.
Но хотелось написать не об этом. А о том чаще всего ваши замечания, точнее даже явные ошибки/уязвимости в статьях остаются неисправленными. А цитату о «небезопасных сдвигах» привести как пример того что действительно важные проекты должны иметь ввиду и исправлять.
Я вот к чему, возьму за пример таких проектов FFMPEG. Постоянно просматриваю их зеркало репозитория на githube и часто замечаю, в комментарии коммита присутствует строка «CID********», а это заметил пару лет назад. Потом как-то вы в своих статьях упоминали про Coverity, я заинтересовался и понял что мейнтейнеры постоянно проверяют свой проект с помощью этого анализатора, точнее у Coverity есть такой сервис который в итоге выводит список подозрительных мест по типу вашего формата.
Так вот, даже такие мелочи как «небезопасные сдвиги» которые работали правильно, а может быть иногда и нет, в течении 10 и более лет, они стараются исправить и учесть маловероятные варианты. В большинстве случаев на все это указывает Coverity.
Судя по таким исправлениям, предупреждения вашего анализатора и Coverity пересекаются процентов на 80% наверно.
Хотел как-то посмотреть на список возможных замечаний как у вас но у них ничего не нашел.
Кстати, достаточно легко отследить исправления на замечания Coverity отфильтровав коммиты FFMPEG по тексту «CID»(для удобства советую TortoiseGit). Судя по логу они начали исправлять найденные замечания еще с 2006г. Думаю, можно найти несколько интересных идей для новых диагностик.
Не могу ответить, насколько мы пересекаемся с Coverity по диагностикам. Сам не знаю и самому интересно.
Знаю только, что мы явно дешевле. Собственно мы пытаемся занять нишу где-то между Gimpel PC-Lint и Coverity.
Знаю только, что мы явно дешевле. Собственно мы пытаемся занять нишу где-то между Gimpel PC-Lint и Coverity.
А, вот вопросик назрел. Если мы перейдем к вам, можно будет выбросить pc-lint, вы насколько его покрываете? А то придется писать комменты вида
//lint -e… //V… (не помю вашего формата)
если этого можно избежать, мы бы с радостью…
//lint -e… //V… (не помю вашего формата)
если этого можно избежать, мы бы с радостью…
Не проводили такое сравнение. Но не думаю, что сильно пересекаемся. PC-Lint во многом направлен на определённые стандарты кодирования и не важно насколько они «совместимы» с современной жизнью. Надо поддержать стандарт кодирования X и они его поддерживают. Например, в PC-Lint есть правило №1063 для выявление кода вида:
Беда — зацикливание на бесконечном вызове конструктора для вызова конструктора. В правильном коде надо передавать аргумент по ссылке: ClassX(const ClassX &x).
Вроде как полезная диагностика. Однако мы её не поддерживаем, и не собираемся поддерживать. И как следствие никогда не будем пересекаться.
Всё дело в том, что это диагностика может быть полезна только для древних или убогих компиляторов. Даже VS2005 просто не компилирует этот код, выдавая ошибку:
Этот пример я привёл для того, чтобы показать, что понятие пересечение весьма тонкое.
В общем, я не знаю ответ. Нужно проводить сравнение, подобное этому: Пересечение PVS-Studio и Cppcheck. Но пока на это нет сил.
Предлагаю попробовать демонстрационную версию и оценить возможности и шум. Мы можем помочь с устранением ложных предупреждений для вашего проекта. Плюс хочу отметить, что в цену лицензии входит поддержка. Мы помогаем людям интегрировать инструмент в процесс разработки. Иногда, это подсказки по настройкам. Иногда, это специфичные опции (пример). Иногда, это правки анализатора для уменьшения ложных срабатываний определённого рода.
Так что пробуйте и пишите нам. Пообщаемся в почте по разным вопросам.
По поводу ложных срабатываний хочу ещё обратить внимание на следующее:
ClassX(const ClassX x) { m_v = x.m_v; }Беда — зацикливание на бесконечном вызове конструктора для вызова конструктора. В правильном коде надо передавать аргумент по ссылке: ClassX(const ClassX &x).
Вроде как полезная диагностика. Однако мы её не поддерживаем, и не собираемся поддерживать. И как следствие никогда не будем пересекаться.
Всё дело в том, что это диагностика может быть полезна только для древних или убогих компиляторов. Даже VS2005 просто не компилирует этот код, выдавая ошибку:
error C2652: 'ClassX' : illegal copy constructor: first parameter must not be a 'ClassX'
Этот пример я привёл для того, чтобы показать, что понятие пересечение весьма тонкое.
В общем, я не знаю ответ. Нужно проводить сравнение, подобное этому: Пересечение PVS-Studio и Cppcheck. Но пока на это нет сил.
Предлагаю попробовать демонстрационную версию и оценить возможности и шум. Мы можем помочь с устранением ложных предупреждений для вашего проекта. Плюс хочу отметить, что в цену лицензии входит поддержка. Мы помогаем людям интегрировать инструмент в процесс разработки. Иногда, это подсказки по настройкам. Иногда, это специфичные опции (пример). Иногда, это правки анализатора для уменьшения ложных срабатываний определённого рода.
Так что пробуйте и пишите нам. Пообщаемся в почте по разным вопросам.
По поводу ложных срабатываний хочу ещё обратить внимание на следующее:
- Предлагаю познакомиться со статьёй "Работа с ложными срабатываниями в PVS-Studio и CppCat"
- Предлагаю познакомиться с "Новый механизм подавления ненужных сообщений анализатора". Можно не исправляя старые ошибки, начать использовать анализатор и искать ошибки только в свежем коде.
«Далее была написана небольшая утилита на C++, которая для каждого запущенного процесса компилятора сохраняла командную строку, текущую директорию и переменные окружения.»
А на bash подменить команду gcc, сохранить все что надо и запустить обычный gcc сильно сложнее было?
А на bash подменить команду gcc, сохранить все что надо и запустить обычный gcc сильно сложнее было?
Кстати, а проверялось ядро же с дефолтными для Ubuntu настройками конфига? И тут нужно сказать что тогда большая часть вручную подключаемых модулей/вариантов реализации тех или иных технологий просто не участвовали в компиляции… Вот если бы проверили с конфигом где установлено максимально возможное кол-во всего и вся, тогда можно было бы говорить что проверили все ядро. Но насколько я знаю, включить все в конфиге можно лишь вручную. С помощью menuconfig вроде можно включить лишь рендумные настройки, но это не то все равно. Или может есть способ кроме вручную?
В апрстрим баги заслали?
То есть не заслали. Вместо того, чтобы оформить их как баги (то есть помочь сообществу) — вывалили как дамп без структурирования и сопровождающей информации.
Почему-то мне захотелось влепить статье минус. Реклама-рекламой, пользы сообществу — ноль.
Почему-то мне захотелось влепить статье минус. Реклама-рекламой, пользы сообществу — ноль.
Зато ваш комментарий принес радость и пользу всему миру. То-то Вы потрудились над ним…
Над чем я должен был «трудиться»? Над проприетарной поделкой? А смысл?
С опенсорсом же я вполне много работаю: bugs.launchpad.net/~george-shuklin
С опенсорсом же я вполне много работаю: bugs.launchpad.net/~george-shuklin
Мы проверили более 200 открытых проектов. В одной только базе на данный момент выписано 1732 предупреждения. Не про все проекты мы писали статьи. Нет смысла писать, если найдено всего 5 ляпов. Но разработчики уведомлены об этих предупреждениях.
Заморачиваться с каждым предупреждением по отдельности мы не будем. И тем более не будем делать патчи. Это колоссальный объем работ, который нам ничего не даст. Про статью узнают сотни или тысячи людей. А об открытых багах или патчах узнают десятки. При этом предлагается потратить в N раз больше времени. Это нецелесообразно. Простите, но мир жесток. :)
Заморачиваться с каждым предупреждением по отдельности мы не будем. И тем более не будем делать патчи. Это колоссальный объем работ, который нам ничего не даст. Про статью узнают сотни или тысячи людей. А об открытых багах или патчах узнают десятки. При этом предлагается потратить в N раз больше времени. Это нецелесообразно. Простите, но мир жесток. :)
кококо реклама кококо сообщество кококо не оформлено как баги
Такое ощущение, что разработчики PVS кому-то что-то должны. Вам указали на ошибки, разобрали их, просто разжевали так, что их понял даже я, хотя к Линуксу отношусь прохладно, а вы, вместо того, чтобы самому дооформить как баги и радоваться, вы просто кукарекаете и так и сидите с этими багами.
Меня всегда поражало, как в кококопенсорсных программах продолжают из релиза в релиз перекладывать баги, просто потому, что никто не оформляет. Ну правильно, проще поныть, что никто не оформляет, чем оформить и написать фикс. Сколько там 12309™ фиксили? 4 года, кажется?
Такое ощущение, что я вам что-то должен. Например, высказывать уважение.
Возможно вы удивитесь тому, сколько багов живет от релиза к релизу в кукаркекомерчесских проектах, только потому, что нельзя их вот так просто взять и посмотреть, и потыкать носом в ошибки.
Я прекрасно знаю, сколько живут баги в той же макоси, и за любовь исправлять давно известные не в следующем релизе, а «когда-нибудь сделаем» просто их ненавижу.
Но в случае с опенсорсом то самое сообщество может уже готовые, найденные баги и их причины записать в багтрекер и в следующем релизе получить исправленную версию. Но нет, у нас же сообщество диванных кукаретиков, нам не надо исправленную версию, нам надо поныть.
Но в случае с опенсорсом то самое сообщество может уже готовые, найденные баги и их причины записать в багтрекер и в следующем релизе получить исправленную версию. Но нет, у нас же сообщество диванных кукаретиков, нам не надо исправленную версию, нам надо поныть.
Смешно читать обвинения в деванизьме от, судя по всему, очередного диванного эксперта. Опенсорс проекты — это не только университетские романтики, пишущие код для удовольствия и для которых нет проблем закрыть баг, состоящий из двух строчек. Серьезные опенсорс проекты имеют свои бюджеты, дедлайны и менеджеров, кроме того, десятки тысяч (или миллионов, в случае с линуксом) строчек кода. И у того же линукса багтрекер имеет сотни тикетов, на которые нужны ресурсы и время. А просто так, самостоятельно внести исправления в код, без предварительного согласования с разработчиком, достаточно проблематично. Разные группы разработчиков закрывают тикеты по-разному, в зависимости от загруженности. Если проект активно развивается и в него добавляется множество фич, то и его багтрекер растет в геометрической прогрессии. Если проект достиг определенного потолка в разработке, то у разработчиков есть больше времени на решение накопившихся багов.
Спасибо кэп, но aulandsdalen, судя по всему, писал конкретно о тех людях, не будем показывать пальцем, назовем его «сообщество»(как выше в посте), которые жалуются что автор данной статьи со своей «поприетарной поделкой» не удосужился оформить все найденные потенциальные баги хотя бы как отдельные багрепорты и что это сообщество вместо того чтобы их оформить, фактически скопипастить их с англ. версии статьи и отослать в нужное место в нужном формате.
П.С. Сам являюсь этим сообществом, чаще всего отправляю патчи в небольшие опенсорс проекты(с десяток таких наверно), из крупных пару патчей было принято в kernel, ну и в мой любимый Qt.
П.С. Сам являюсь этим сообществом, чаще всего отправляю патчи в небольшие опенсорс проекты(с десяток таких наверно), из крупных пару патчей было принято в kernel, ну и в мой любимый Qt.
Дело в том, что в «сообществе», а тем более хабросообществе, это является нормальной практикой — отправлять разработчикам баги, дыры и т.п. Ребята похвастались, что нашли ошибки и даже отправили их «вроде туда, я не разобрался». Понятно, что они ничего ни кому не должны, но негативный осадок остался.
Оххххх…
aulandsdalen:
Такое ощущение, что разработчики PVS кому-то что-то должны. Вам указали на ошибки, разобрали их, просто разжевали так, что их понял даже я, хотя к Линуксу отношусь прохладно, а вы, вместо того, чтобы самому дооформить как баги и радоваться, вы просто кукарекаете и так и сидите с этими багами.
Andrey2008:
Заморачиваться с каждым предупреждением по отдельности мы не будем. И тем более не будем делать патчи. Это колоссальный объем работ, который нам ничего не даст. Про статью узнают сотни или тысячи людей. А об открытых багах или патчах узнают десятки. При этом предлагается потратить в N раз больше времени. Это нецелесообразно. Простите, но мир жесток. :)
Теперь вопрос, зачем все это обсуждение если все свелось к этому: (мое видиние данной ситуации)
— Вы отправили соответствующие багрепорты в нужное место в нужном формате?
— Нет, написали правда один багрепорт на оф. багзиллу…
— Ах нет, арррр. Как вы могли в своем подсайте написать отчет о найденых багах в нашем святая святых Linux kernel'e и при этом еще издевательски открыто пишите что это реклама в начале статьи!?
— Нууу… у нас просто нет ресурса и времени на это, так как мы посчитали что это обойдется нам достаточно дорого в человеко/часах, а выхлопа практически никакого не будет.
— Я вас не уважаю, думаю, надо влепить минус.
— Вы сами можете оформить подробно описанные баги, чтобы сэкономить время можете скопировать текст из англ. версии статьи.
— Почему Я это должен делать!? А? У меня вон по ссылке и так стопицот вываленых дампов на багтрекер убубнты с указанием версии и где упало.
— Почему вы так культурно кукарекаете? Почему тратите время на комментарии если могли бы за это время отослать эти злополучные багрепорты?
— Ты, диванный эксперт, хоть знаешь что такое крупный опенсор…
Ну разве не так? :) Старый добрый хабр, старый добрый срач.
aulandsdalen:
Такое ощущение, что разработчики PVS кому-то что-то должны. Вам указали на ошибки, разобрали их, просто разжевали так, что их понял даже я, хотя к Линуксу отношусь прохладно, а вы, вместо того, чтобы самому дооформить как баги и радоваться, вы просто кукарекаете и так и сидите с этими багами.
Andrey2008:
Заморачиваться с каждым предупреждением по отдельности мы не будем. И тем более не будем делать патчи. Это колоссальный объем работ, который нам ничего не даст. Про статью узнают сотни или тысячи людей. А об открытых багах или патчах узнают десятки. При этом предлагается потратить в N раз больше времени. Это нецелесообразно. Простите, но мир жесток. :)
Теперь вопрос, зачем все это обсуждение если все свелось к этому: (мое видиние данной ситуации)
— Вы отправили соответствующие багрепорты в нужное место в нужном формате?
— Нет, написали правда один багрепорт на оф. багзиллу…
— Ах нет, арррр. Как вы могли в своем подсайте написать отчет о найденых багах в нашем святая святых Linux kernel'e и при этом еще издевательски открыто пишите что это реклама в начале статьи!?
— Нууу… у нас просто нет ресурса и времени на это, так как мы посчитали что это обойдется нам достаточно дорого в человеко/часах, а выхлопа практически никакого не будет.
— Я вас не уважаю, думаю, надо влепить минус.
— Вы сами можете оформить подробно описанные баги, чтобы сэкономить время можете скопировать текст из англ. версии статьи.
— Почему Я это должен делать!? А? У меня вон по ссылке и так стопицот вываленых дампов на багтрекер убубнты с указанием версии и где упало.
— Почему вы так культурно кукарекаете? Почему тратите время на комментарии если могли бы за это время отослать эти злополучные багрепорты?
— Ты, диванный эксперт, хоть знаешь что такое крупный опенсор…
Ну разве не так? :) Старый добрый хабр, старый добрый срач.
Я написал сюда: bugzilla.kernel.org. Может это не то место, но я больше не понял куда надо. :)
Я вам страшную тайну открою, но в огромном числе проектов для вычисления смещения члена структуры используется макрос вида
#define OFFSETOF(t,m) ((size_t)&((t*)0)->m)
И никогда это не считалось криминалом
#define OFFSETOF(t,m) ((size_t)&((t*)0)->m)
И никогда это не считалось криминалом
При использовании gcc лучше использовать __builtin_offsetof.
В стандарте C99 есть
Кстати, из имеющихся у меня компиляторов gcc и clang используют
Короче подключайте
offsetof (из заголовочного файла stddef.h). Лучше всего использовать именно его, а не __builtin_offsetof или ещё что‐то, и только при использовании компиляторов не поддерживающих C99 использовать #define.Кстати, из имеющихся у меня компиляторов gcc и clang используют
__builtin_offsetof, pcc и tcc используют ((size_t)&((t*)0)->m) (только используют всё же более приличные имена параметров), ekopath использует и то, и то (я недостаточно знаком с этим компилятором, чтобы сказать, когда реально используется: __builtin_offsetof из /opt/ekopath/lib/5.0.1/include/stddef.h, а когда другой вариант из /opt/ekopath/include/5.0.1/stl/ansi/_cstddef.h, но pathcc -E на простейшую програмку выводит __builtin_offsetof).Короче подключайте
stddef.h и используйте offsetof. Разработчики компиляторов лучше знают, как это надо реализовать. __builtin_offsetof в стандарте нет.А это и не криминал. В статье написано немного про другое.
Думаю, это немного другое. Здесь работают не с внешним указателем, а я явно с 0. Скорее всего компилятор догадывается зачем такое нужно и сделает работающий код. А вот для переменной он может оптимизировать код и сделать его нерабочим. Но в любом случае лучше так не делать и использовать средства, которые есть в системных заголовочных файлах. Они сделаны с учетом работы компилятора.
Вот ещё хороший пример на тему, что вроде как работает, но лучше не надо: this == 0.
Вот ещё хороший пример на тему, что вроде как работает, но лучше не надо: this == 0.
Продолжение этой темы: Размышления над разыменованием нулевого указателя.
Спасибо, полезный продукт. А можно тотже «заряд», но уже по ядру FreeBSD?
просто по заявлению bsd сообщества, их ядро чутли не идеально (по количеству ошибок на обьем кода), ну как все наверное знают. интересно что скажет ваше приложение.
НЛО прилетело и опубликовало эту надпись здесь
Там куча варнингов при сборке шлангом ядра с включёнными варнингами.Позвольте с этим не согласиться. Бо́льшая часть кода FreeBSD собирается с включёнными варнингами и
-Werror. Интереса ради пересобрал ядро (r276106), вот пара цифр:$ grep '^cc ' /tmp/buildkernel.log | wc -l
3452
$ grep warning: /tmp/buildkernel.log | wc -l
56
$ cc -v
FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
Target: i386-unknown-freebsd11.0
Thread model: posix
Итого: 56 предупреждений (16 из них в contributed коде типа zfs) на 3452 вызова компилятора, или 1.62%.
НЛО прилетело и опубликовало эту надпись здесь
Проверил свой «велосипед» PVS-Studio, нашел несколько косяков, подумал, да, действительно они могли бы быть серьезными уязвимостями, если бы он не был велосипедом написанным для себя на один раз…
НЛО прилетело и опубликовало эту надпись здесь
я так понимаю если они неотчищают память где находится хэш, то к ниму можно както добраться? т.е. по сути закладка.
Помните Heartbleed где атакуюшему возвращались рендумные участки памяти системы с этой уязвимостью, а теперь представьте сколько раз программа в которой компилятор выбросил такую очистку, например вашего пароля от аккаунта, могла оставить в памяти эти данные которые утекли злоумышленнику, собственно на это и расчет был. Конечно были, есть и будут другие способы получения доступа к памяти подверженного какой-либо уязвимость, но этот пример просто идеально тут вписался.
Навряд закладки, вы даже не представляете насколько часто можно увидеть одни и те же ляпы с этим злосчастным мемсетом…
Навряд закладки, вы даже не представляете насколько часто можно увидеть одни и те же ляпы с этим злосчастным мемсетом…
а расскажите, как вообще у вас дела с false positive и вот этим вот всем?
Возможно будут интересны эти ссылки:
www.viva64.com/ru/b/0263/
www.viva64.com/ru/b/0289/
Если это не то, то прошу задать уточняющие вопросы.
www.viva64.com/ru/b/0263/
www.viva64.com/ru/b/0289/
Если это не то, то прошу задать уточняющие вопросы.
Вы, конечно, молодцы, но когда читаешь названия некоторых ошибок/предупреждений, понимаешь, что вам есть куда улучшаться.
Range intersections are possible within conditional expressions.
Что это такое? Почему бы не написать проще: Неиспользуемые ветки условных выражений.
А следующее:
Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'.
Пишите проще: Возможно неверное вычисление из-за использования приоритетов операций по-умолчанию. 'A = B < C' вычисляется как 'A = (B < C)
The use of 'if (A) {...} else if (A) {...}' pattern was detected.
Тут уже лучше. Но почему бы не написать: Проверяется одинаковое условие в разных ветках составного условного выражения.
Range intersections are possible within conditional expressions.
Что это такое? Почему бы не написать проще: Неиспользуемые ветки условных выражений.
А следующее:
Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'.
Пишите проще: Возможно неверное вычисление из-за использования приоритетов операций по-умолчанию. 'A = B < C' вычисляется как 'A = (B < C)
The use of 'if (A) {...} else if (A) {...}' pattern was detected.
Тут уже лучше. Но почему бы не написать: Проверяется одинаковое условие в разных ветках составного условного выражения.
Второй и третий случай в Вашем объяснении мне показался более запутанным, а в первом случае так вообще «пересечение условий» != «неиспользованные ветки».
Я в FindBugs в аналогичном первому случаю баге написал «Useless condition: it's known that {2} at this point», где {2} — это то, что заведомо истинно во второй ветке. В FindBugs с правильной формулировкой, кстати, сложнее. Было бы понятнее написать «Useless condition: {условие, которое в исходнике} is always {true/false} at this point», но так как это анализатор байткода, он не всегда может достоверно восстановить, что было в исходнике. Может оказаться, что в коде проверка if(x > 2), а в байткоде компилятор решит поменять на x <= 2. В PVS-Studio такой проблемы нет. У нас, кстати, с английским тоже есть проблемы, так что патчи к файлу сообщений от знающих людей принимаются :-)
Проверьте пожалуйста Pulseaudio, а то они сами не могут.
Лучше systemd :)
Просмотрел свеженькую версию прям из git репозитория. Увидел лишь несколько действительно стремных мест. Итак.
1----
«V590» «Consider inspecting this expression. The expression is excessive or contains a misprint.» «sink-input.c» [«1881»]
if ((state == PA_SINK_INPUT_DRAINED || state == PA_SINK_INPUT_RUNNING) &&
!(i->thread_info.state == PA_SINK_INPUT_DRAINED || i->thread_info.state != PA_SINK_INPUT_RUNNING))
pa_atomic_store(&i->thread_info.drained, 1);
Достаточно странная проверка.
В случае когда i->thread_info.state == PA_SINK_INPUT_RUNNING и первое условие(до &&) TRUE, то вторая часть тоже TRUE и тогда установится флаг «drained».
В случае когда i->thread_info.state == PA_SINK_INPUT_DRAINED, вторая часть — FALSE
В случае когда (i->thread_info.state != PA_SINK_INPUT_DRAINED && i->thread_info.state != PA_SINK_INPUT_RUNNING), вторая часть —
FALSE. А кроме этих двух состояний там еще 3.
Фактически вторую часть можно было бы упростить до:
if ((state == PA_SINK_INPUT_DRAINED || state == PA_SINK_INPUT_RUNNING)
&& i->thread_info.state == PA_SINK_INPUT_RUNNING)
Но так ли это? Я не знаю.
2----
«V593» «Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'.» «alsa-util.c» [«446»]
if ((err = snd_pcm_sw_params_current(pcm, swparams) < 0)) {
pa_log_warn(«Unable to determine current swparams: %s\n», pa_alsa_strerror(err));
return err;
}
Ну, тут все ясно, опечатка с позицией скобки. Ниже, кстати код где аналогично проверяется результат функции — корректен.
3----
«V646» «Consider inspecting the application's logic. It's possible that 'else' keyword is missing.» «pacmd.c» [«336»]
if (watch_socket->revents & POLLHUP) {
…
} if (watch_socket->revents & POLLOUT) {
…
}
Опечатка. отсутствует «else». Но никак на работу это не повлияет так как флаги POLLHUP и POLLOUT по своей природе являются взаимоисключающие.
linux.die.net/man/3/poll
POLLOUT — Normal data may be written without blocking.
POLLHUP — The device has been disconnected. This event and POLLOUT are mutually-exclusive; a stream can never be writable if a hangup has occurred. However, this event and POLLIN, POLLRDNORM, POLLRDBAND, or POLLPRI are not mutually-exclusive. This flag is only valid in the revents bitmask; it shall be ignored in the events member.
4----
«V556» «The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }.» «module-tunnel.c» [«694»]
Упрощенно:
switch ((pa_source_state_t) state) {
case PA_SOURCE_SUSPENDED:
case PA_SOURCE_IDLE:
case PA_SOURCE_RUNNING:
case PA_SOURCE_UNLINKED:
case PA_SOURCE_INIT:
case PA_SINK_INVALID_STATE:
Смотрим на последнюю строчку — PA_SINK_INVALID_STATE. Из pa_source_state_t enum'a есть аналогичный, который тут должен быть — PA_SOURCE_INVALID_STATE. Но тут все работает как задумано, оба имеют значение "-1". Вот, если вдруг кто-то чего поменяет… Это будет совсем другая история.
5----
«V636» «The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;.» «module-raop-sink.c» [«385»]
Самое интересное предупреждение.
uint32_t silence_overhead = 0;
pa_memchunk silence_tmp;
pa_memchunk_reset(&silence_tmp);
silence_tmp.memblock = pa_memblock_new(u->core->mempool, 4096);
silence_tmp.length = 4096;
p = pa_memblock_acquire(silence_tmp.memblock);
memset(p, 0, 4096);
pa_memblock_release(silence_tmp.memblock);
pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
pa_assert(0 == silence_tmp.length);
silence_overhead = silence_tmp.length — 4096;
silence_ratio = silence_tmp.length / 4096; raop, &silence_tmp, &silence);
где эта длина может измениться. Давайте посмотрим на функцию:
github.com/pulseaudio/pulseaudio/blob/master/src/modules/raop/raop_client.c#L472
Меняется только в одном месте:
raw->length -= 4;
Значит «silence_tmp.length» будет меньше 4096 или равно, но навряд.
Получается «silence_ratio», когда silence_tmp.length < 4096, будет равно 0.
Но это еще не все, давайте взглянем на строчку выше от той на которую указал анализатор:
silence_overhead = silence_tmp.length — 4096;
Ойойой… Тут же переполнение беззнакового типа в случае когда «silence_tmp.length < 4096».
Тут не хватает проверки и соответствующей логики или же в pa_raop_client_encode_sample()
необходимо не отнимать. а прибавлять:
raw->length -= 4; ---> raw->length += 4;
P.S. У кого есть время — оформите пожалуйста на бегтрекере. Времени на это не осталось.
P.P.S. Не ругайте за оформление коммента, теги не работают.
1----
«V590» «Consider inspecting this expression. The expression is excessive or contains a misprint.» «sink-input.c» [«1881»]
if ((state == PA_SINK_INPUT_DRAINED || state == PA_SINK_INPUT_RUNNING) &&
!(i->thread_info.state == PA_SINK_INPUT_DRAINED || i->thread_info.state != PA_SINK_INPUT_RUNNING))
pa_atomic_store(&i->thread_info.drained, 1);
Достаточно странная проверка.
В случае когда i->thread_info.state == PA_SINK_INPUT_RUNNING и первое условие(до &&) TRUE, то вторая часть тоже TRUE и тогда установится флаг «drained».
В случае когда i->thread_info.state == PA_SINK_INPUT_DRAINED, вторая часть — FALSE
В случае когда (i->thread_info.state != PA_SINK_INPUT_DRAINED && i->thread_info.state != PA_SINK_INPUT_RUNNING), вторая часть —
FALSE. А кроме этих двух состояний там еще 3.
Фактически вторую часть можно было бы упростить до:
if ((state == PA_SINK_INPUT_DRAINED || state == PA_SINK_INPUT_RUNNING)
&& i->thread_info.state == PA_SINK_INPUT_RUNNING)
Но так ли это? Я не знаю.
2----
«V593» «Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'.» «alsa-util.c» [«446»]
if ((err = snd_pcm_sw_params_current(pcm, swparams) < 0)) {
pa_log_warn(«Unable to determine current swparams: %s\n», pa_alsa_strerror(err));
return err;
}
Ну, тут все ясно, опечатка с позицией скобки. Ниже, кстати код где аналогично проверяется результат функции — корректен.
3----
«V646» «Consider inspecting the application's logic. It's possible that 'else' keyword is missing.» «pacmd.c» [«336»]
if (watch_socket->revents & POLLHUP) {
…
} if (watch_socket->revents & POLLOUT) {
…
}
Опечатка. отсутствует «else». Но никак на работу это не повлияет так как флаги POLLHUP и POLLOUT по своей природе являются взаимоисключающие.
linux.die.net/man/3/poll
POLLOUT — Normal data may be written without blocking.
POLLHUP — The device has been disconnected. This event and POLLOUT are mutually-exclusive; a stream can never be writable if a hangup has occurred. However, this event and POLLIN, POLLRDNORM, POLLRDBAND, or POLLPRI are not mutually-exclusive. This flag is only valid in the revents bitmask; it shall be ignored in the events member.
4----
«V556» «The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }.» «module-tunnel.c» [«694»]
Упрощенно:
switch ((pa_source_state_t) state) {
case PA_SOURCE_SUSPENDED:
case PA_SOURCE_IDLE:
case PA_SOURCE_RUNNING:
case PA_SOURCE_UNLINKED:
case PA_SOURCE_INIT:
case PA_SINK_INVALID_STATE:
Смотрим на последнюю строчку — PA_SINK_INVALID_STATE. Из pa_source_state_t enum'a есть аналогичный, который тут должен быть — PA_SOURCE_INVALID_STATE. Но тут все работает как задумано, оба имеют значение "-1". Вот, если вдруг кто-то чего поменяет… Это будет совсем другая история.
5----
«V636» «The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;.» «module-raop-sink.c» [«385»]
Самое интересное предупреждение.
uint32_t silence_overhead = 0;
pa_memchunk silence_tmp;
pa_memchunk_reset(&silence_tmp);
silence_tmp.memblock = pa_memblock_new(u->core->mempool, 4096);
silence_tmp.length = 4096;
p = pa_memblock_acquire(silence_tmp.memblock);
memset(p, 0, 4096);
pa_memblock_release(silence_tmp.memblock);
pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
pa_assert(0 == silence_tmp.length);
silence_overhead = silence_tmp.length — 4096;
silence_ratio = silence_tmp.length / 4096; raop, &silence_tmp, &silence);
где эта длина может измениться. Давайте посмотрим на функцию:
github.com/pulseaudio/pulseaudio/blob/master/src/modules/raop/raop_client.c#L472
Меняется только в одном месте:
raw->length -= 4;
Значит «silence_tmp.length» будет меньше 4096 или равно, но навряд.
Получается «silence_ratio», когда silence_tmp.length < 4096, будет равно 0.
Но это еще не все, давайте взглянем на строчку выше от той на которую указал анализатор:
silence_overhead = silence_tmp.length — 4096;
Ойойой… Тут же переполнение беззнакового типа в случае когда «silence_tmp.length < 4096».
Тут не хватает проверки и соответствующей логики или же в pa_raop_client_encode_sample()
необходимо не отнимать. а прибавлять:
raw->length -= 4; ---> raw->length += 4;
P.S. У кого есть время — оформите пожалуйста на бегтрекере. Времени на это не осталось.
P.P.S. Не ругайте за оформление коммента, теги не работают.
Блин, что за парсер такой… теги не пашут, зато строки с "< ----" съедает.
В последнем замечании там я написал:
int32_t silence_overhead = 0;
pa_memchunk silence_tmp;
pa_memchunk_reset(&silence_tmp);
silence_tmp.memblock = pa_memblock_new(u->core->mempool, 4096);
silence_tmp.length = 4096;
p = pa_memblock_acquire(silence_tmp.memblock);
memset(p, 0, 4096);
pa_memblock_release(silence_tmp.memblock);
pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
pa_assert(0 == silence_tmp.length);
silence_overhead = silence_tmp.length — 4096;
silence_ratio = silence_tmp.length / 4096; < =====
Вот на эту строку указывает анализатор. Значение silence_tmp.length меняется немного выше при вызове функции:
pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
В последнем замечании там я написал:
int32_t silence_overhead = 0;
pa_memchunk silence_tmp;
pa_memchunk_reset(&silence_tmp);
silence_tmp.memblock = pa_memblock_new(u->core->mempool, 4096);
silence_tmp.length = 4096;
p = pa_memblock_acquire(silence_tmp.memblock);
memset(p, 0, 4096);
pa_memblock_release(silence_tmp.memblock);
pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
pa_assert(0 == silence_tmp.length);
silence_overhead = silence_tmp.length — 4096;
silence_ratio = silence_tmp.length / 4096; < =====
Вот на эту строку указывает анализатор. Значение silence_tmp.length меняется немного выше при вызове функции:
pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
Опечатка. отсутствует «else».
Не факт, возможно просто if не перенесён на новую строку после скобки. Тогда логика выглядит нормальной.
Для 2,4 попробую сделать патч и ещё cppcheck прогнать.
>Опечатка. отсутствует «else». Но никак на работу это не повлияет так как флаги POLLHUP и POLLOUT по своей природе являются взаимоисключающими.
а вот не факт, только если мы на 100% уверены, что биты этих флагов _никогда_ не будут заданы одновременно, или могут обе ветки сработать. ну а в одном бите(тогда была бы 100% взаимоисключаемость) они лежать не могут, потому что тогда кому-то пришлось бы быть нулем и использовать просто побитовое и не вышло бы
а вот не факт, только если мы на 100% уверены, что биты этих флагов _никогда_ не будут заданы одновременно, или могут обе ветки сработать. ну а в одном бите(тогда была бы 100% взаимоисключаемость) они лежать не могут, потому что тогда кому-то пришлось бы быть нулем и использовать просто побитовое и не вышло бы
Факт, по крайней мере так говорит официальная документация. Иначе, предположим, что вместо POLLOUT был бы один из (POLLIN, POLLRDNORM, POLLRDBAND, POLLPRI). Тогда вполне вероятно что логика может отработать некорректно, то есть отработать и там и там. Судя по остальному коду, оформлении и т.д. той функции, это просто удачное стечение обстоятельств что забыли else как раз там где это не критично.
Ещё раз спасибо за отчёт. Отправил четвёртый патч в pulseaudio, исправляет проблему 1. Писал о ней в IRC разработчикам, но видимо, ни у кого не нашлось времени. Посмотрел git blame и вернул код, который был раньше:
Выглядит более логично.
Проблема 5 вроде как неактуальна, потому что написана версия 2 модуля raop.
- !(i->thread_info.state == PA_SINK_INPUT_DRAINED || i->thread_info.state != PA_SINK_INPUT_RUNNING))
+ !(i->thread_info.state == PA_SINK_INPUT_DRAINED || i->thread_info.state == PA_SINK_INPUT_RUNNING))
Выглядит более логично.
Проблема 5 вроде как неактуальна, потому что написана версия 2 модуля raop.
НЛО прилетело и опубликовало эту надпись здесь
Тем временем по вашим замечаниям уже начали высылать патчи, к примеру:
lkml.org/lkml/2015/1/4/137 и другие от этого автора.
Остановился на этом так как у меня возник вопрос.
С одним из фиксом замечания из статьи в этом патче еще исправлялась похожая проблема, но ее PVS не выловил.
Вот участок кода с ошибкой.
github.com/torvalds/linux/blob/master/drivers/scsi/ipr.c#L3993
Приведу комментарий Торвальдса на этот счет:
> Especially since one very strange piece of code seems to be written in
> such a way that a NUL needs to be placed where a NUL is present already.
Actually, it's worse than that. This:
> len = snprintf(fname, 99, "%s", buf);
> — fname[len-1] = '\0';
is complete garbage, since the return value of snprintf() is not the
length of the result, but length of what the result *would* have been.
So if the string doesn't fit in 99 bytes, it will actively corrupt
some random memory after the string. It's not writing zero to what was
already zero, it's corrupting memory.
Так как практически никогда snprintf не использовал полез читать о нем, например тут www.cplusplus.com/reference/cstdio/snprintf/
«If the resulting string would be longer than n-1 characters, the remaining characters are discarded and not stored, but counted for the value returned by the function.» Собственно то что и написал Торвальдс.
Ок, действительно плохо получается. Решил проверить что и как, написал пример — ideone.com/6FWa37
Думаю, объяснять что к чему там не надо. Очень каверзный момент с этим «snprintf».
Гугл так же выдал пару ссылок о этой функции на российские сайты, я на автомате клацнул, пролистал, ничего нового. Но тут глаз остановился на комментарии из этой статьи…
demin.ws/blog/russian/2013/01/28/use-snprintf-on-different-platforms/
Походу, такого рода ошибки можно найти будет где угодно если они даже в ядре нашлись.
А теперь у меня вопрос для разработчиков PVS, реально ли такие специфические вещи типа как с этим snprintf добавить в анализатор? Так как эта функция так же будет поддерживаться начиная с MSVC 2014
blogs.msdn.com/b/vcblog/archive/2014/06/03/visual-studio-14-ctp.aspx
C99 library features: Most of the remaining C99 library features are implemented. snprintf is implemented, the printf and scanf families of functions now support the new C99 format string improvements, the strtod and scanf families of functions now support hexadecimal floating-point and library conformance is better improved by software updates and adjustments.
П.С. Почему-то не работают теги
lkml.org/lkml/2015/1/4/137 и другие от этого автора.
Остановился на этом так как у меня возник вопрос.
С одним из фиксом замечания из статьи в этом патче еще исправлялась похожая проблема, но ее PVS не выловил.
Вот участок кода с ошибкой.
github.com/torvalds/linux/blob/master/drivers/scsi/ipr.c#L3993
Приведу комментарий Торвальдса на этот счет:
> Especially since one very strange piece of code seems to be written in
> such a way that a NUL needs to be placed where a NUL is present already.
Actually, it's worse than that. This:
> len = snprintf(fname, 99, "%s", buf);
> — fname[len-1] = '\0';
is complete garbage, since the return value of snprintf() is not the
length of the result, but length of what the result *would* have been.
So if the string doesn't fit in 99 bytes, it will actively corrupt
some random memory after the string. It's not writing zero to what was
already zero, it's corrupting memory.
Так как практически никогда snprintf не использовал полез читать о нем, например тут www.cplusplus.com/reference/cstdio/snprintf/
«If the resulting string would be longer than n-1 characters, the remaining characters are discarded and not stored, but counted for the value returned by the function.» Собственно то что и написал Торвальдс.
Ок, действительно плохо получается. Решил проверить что и как, написал пример — ideone.com/6FWa37
Думаю, объяснять что к чему там не надо. Очень каверзный момент с этим «snprintf».
Гугл так же выдал пару ссылок о этой функции на российские сайты, я на автомате клацнул, пролистал, ничего нового. Но тут глаз остановился на комментарии из этой статьи…
demin.ws/blog/russian/2013/01/28/use-snprintf-on-different-platforms/
Походу, такого рода ошибки можно найти будет где угодно если они даже в ядре нашлись.
А теперь у меня вопрос для разработчиков PVS, реально ли такие специфические вещи типа как с этим snprintf добавить в анализатор? Так как эта функция так же будет поддерживаться начиная с MSVC 2014
blogs.msdn.com/b/vcblog/archive/2014/06/03/visual-studio-14-ctp.aspx
C99 library features: Most of the remaining C99 library features are implemented. snprintf is implemented, the printf and scanf families of functions now support the new C99 format string improvements, the strtod and scanf families of functions now support hexadecimal floating-point and library conformance is better improved by software updates and adjustments.
П.С. Почему-то не работают теги
НЛО прилетело и опубликовало эту надпись здесь
Если честно, то я не понял комментарий. Если бы выражение не было подозрительным, я бы его и не выписал. :) Оно странное в разных смыслах.
Я думаю, имелось в виду следующее. Ваша диагностика проанализировав первые два выражения в условии решила, что одно из них лишнее (сравнение с нулем). А если бы взяли еще и третье выражение, то получилось бы, что все оно целиком всегда истинно. Если первое — это просто странно (но не критично, и, наверное, предупреждения такого рода можно отключить), то второе — почти наверняка ошибка.
Очевидно ошибка есть. По логике этого выражения тут два варианта должно быть.
1. if (module_type == 0 || module_type == PORTS || module_type == MODEM)
2. или if (module_type == 0 || (module_type != PORTS && module_type != MODEM) )
По логике кода github.com/torvalds/linux/blob/master/drivers/staging/dgap/dgap.c#L1022
я пока не могу понять какой вариант подходит.
1. if (module_type == 0 || module_type == PORTS || module_type == MODEM)
2. или if (module_type == 0 || (module_type != PORTS && module_type != MODEM) )
По логике кода github.com/torvalds/linux/blob/master/drivers/staging/dgap/dgap.c#L1022
я пока не могу понять какой вариант подходит.
НЛО прилетело и опубликовало эту надпись здесь
Такая диагностика есть. И скорее всего, она выводилась, но я её проглядел или к тому моменту уже выписал V590.
НЛО прилетело и опубликовало эту надпись здесь
Есть ли в PVS проверка инициализации переменных перед их использованием?
Не знаю про PVS-студию, но вообще-то такая проверка есть практически в любом современном компиляторе, разве что не включена по умолчанию. То есть, конечно, PVS тоже может на это ругаться, но мне кажется, нацелен несколько на иной класс ошибок (которые компиляторами обычно не ловятся).
Встретилось такое: по таймеру вызывается функция
при первом вызове подразумевалось, что a == 0, чего не было на самом деле, т.к «a» не была проинициализирована в обьекте. Компилятор VS2012 не ругался (настройки стандартные).
void foo(Obj *obj) {
if (obj->a > 0) { --obj->a; return; }
if (какое-то условие && obj->a <= 0) {
obj->a = 300;
//другие действия
}
}
при первом вызове подразумевалось, что a == 0, чего не было на самом деле, т.к «a» не была проинициализирована в обьекте. Компилятор VS2012 не ругался (настройки стандартные).
Кажется, понимаю, о чем вы. Действительно, вряд ли компилятор будет анализировать весь жизненный путь объекта и следить, в какие функции и в какой последовательности он передается и как там используется. А вот PVS-студия могла бы (если еще не может).
Под «переменными» обычно понимают локальные переменные функции. Компиляторы часто работают на уровне функций, так что значения локальных переменных более-менее контролируют. В отличие от аргументов и глобальной памяти, где с их точки зрения может лежать вообще что угодно, вписывающееся в тип данных.
А вот проверки, подобные приведённому вами примеру, как раз могут быть выполнены статическим анализом (пока он в состоянии проследить значения аргументов при всех вызовах функции). Или динамически с помощью разнообразных утилит вроде Valgrind, если логика программы слишком сложна для статического анализа.
А вот проверки, подобные приведённому вами примеру, как раз могут быть выполнены статическим анализом (пока он в состоянии проследить значения аргументов при всех вызовах функции). Или динамически с помощью разнообразных утилит вроде Valgrind, если логика программы слишком сложна для статического анализа.
Да, проверка есть (573, 614). Но нет, не найдёт. В данном случае вы хотите волшебного от статического анализа. Ему очень сложно отследить возможные варианты потоков управления, а часто просто невозможно. Представите, например, что инициализация переменных зависит от данных прочитанных из файла. Для таких случаев лучше подходит динамический анализ. Подробности на эту тему: Статический и динамический анализ кода.
Но тут «a» используется только в одной функции и сразу же в первой её строке, без всяких условий. Вроде бы ничего динамического, инициализация всегда после использования.
Это вы знаете, что
a больше нигде не используется. Компилятор или анализатор же видят, что в функцию передается указатель на некий объект. Чтобы им прийти к тому же выводу, что вы, им понадобится полностью отследить время жизни объекта, где, кем и как он используется. Это теоретически возможно в относительно простых случаях (как ваш), но вообще-то уже на грани возможностей анализатора. Такие ошибки намного лучше отлавливаются тем же валгриндом (динамически).
Нигде не нашел полный лог вашей проверки. Только частные случаи:
www.viva64.com/external-pictures/txt/Linux-V595.txt
www.viva64.com/external-pictures/txt/Linux-V610.txt
Я плохо искал, или вы его не предоставляете?
www.viva64.com/external-pictures/txt/Linux-V595.txt
www.viva64.com/external-pictures/txt/Linux-V610.txt
Я плохо искал, или вы его не предоставляете?
Не предоставляем.
Вопрос задают уже не первый раз. Решил пояснить развёрнуто.
Не выкладываем не из-за вредности или жадности. Некоторые открывают XML файл в текстовом редакторе и начинают ругаться, что с таким логом им неудобно работать. Результатом являются неадекватные негативные комментарии. Другие знают, как открыть файл в PVS-Studio или PVS-Studio Standalone, но не догадываются отфильтровать в настройках папку PNG, ZLIB, BOOST и опять-таки оставляют негативные комментарии. Есть масса других способов сделать что-то неправильно или понять что-то неправильно и ругаться в интернете, твиттере и прочих людных местах :). Решили, что больше не будем выкладывать полные логи. Так нам проще жить и не сражаться в бессмысленными замечаниями и письмами. Бессмысленная трата времени и сил на написание ответов, пояснений. Кто надо, тот сам (или с нашей помощью) может получить лог. Это хороший фильтр.
Не выкладываем не из-за вредности или жадности. Некоторые открывают XML файл в текстовом редакторе и начинают ругаться, что с таким логом им неудобно работать. Результатом являются неадекватные негативные комментарии. Другие знают, как открыть файл в PVS-Studio или PVS-Studio Standalone, но не догадываются отфильтровать в настройках папку PNG, ZLIB, BOOST и опять-таки оставляют негативные комментарии. Есть масса других способов сделать что-то неправильно или понять что-то неправильно и ругаться в интернете, твиттере и прочих людных местах :). Решили, что больше не будем выкладывать полные логи. Так нам проще жить и не сражаться в бессмысленными замечаниями и письмами. Бессмысленная трата времени и сил на написание ответов, пояснений. Кто надо, тот сам (или с нашей помощью) может получить лог. Это хороший фильтр.
Быть может кому-то ещё будет интересно обсуждение этой статьи, которое развернулось на LINUX.ORG.RU: www.linux.org.ru/news/kernel/11210932
Обсуждение этой статьи несколько раз выпиливалось с Linux.Org.Ru.
Вот сохраненная копия: webhamster.ru/site/page/index/articles/comp/323
Вот сохраненная копия: webhamster.ru/site/page/index/articles/comp/323
Если кому-то нравится наш сайт, статьи и вообще наша деятельность и он активный участник Wikipedia, то прошу помощь отстоять нашу страничку: en.wikipedia.org/wiki/PVS-Studio
И мы будем с новыми силами радовать всех багостатьями. :)
И мы будем с новыми силами радовать всех багостатьями. :)
Собственно обсуждение: en.wikipedia.org/wiki/Wikipedia:Articles_for_deletion/PVS-Studio
Насколько я понял, проблемы две: «недостоверные источники» и «пользователь не указал работодателя». Вопрос: зачем спорить про то, заплатит ли работодатель за статью на Wikipedia, если можно просто добавить страничку http://en.wikipedia.org/wiki/User:PaulEremeeff с требуемой информацией и спорить уже только про источники?
Ещё одна большая статья, только о проверке ядра FreeBSD: PVS-Studio покопался в ядре FreeBSD
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
PVS-Studio покопался во внутренностях Linux (3.18.1)