Приблизительно раз в полгода нам пишет кто-то из сотрудников компании Yandex, интересуется лицензированием PVS-Studio, качает триал и пропадает. Это нормально, мы привыкли к медленным процессам продажи нашего анализатора в крупные компании. Однако, раз представился повод, будет не лишним передать разработчикам Yandex привет и напомнить об инструменте PVS-Studio.
Честно скажу, что статья получилась во многом случайным образом. Нам уже предлагали проверить ClickHouse, но как-то эта идея забылась. На днях, гуляя по просторам интернета, я вновь встретил упоминание ClickHouse и заинтересовался этим проектом. В этот раз я решил не откладывать и проверить этот проект.
ClickHouse
ClickHouse — это столбцовая СУБД для OLAP (online обработки аналитических запросов). ClickHouse был разработан в Яндексе для решения задач Яндекс.Метрики. ClickHouse позволяет выполнять аналитические запросы по обновляемым данным в режиме реального времени. Система линейно масштабируемая и способна работать с триллионами записей и петабайтами данных. В июне 2016-го года ClickHouse был выложен в open-source под лицензией Apache 2.0.
- Сайт: clickhouse.yandex.
- Страница в Wikipedia: ClickHouse.
- Статья на сайте Habrahabr: Яндекс открывает ClickHouse.
- Репозиторий на сайте GitHub.com: yandex/ClickHouse.
Анализ проекта с помощью PVS-Studio
Я проверял исходный код ClickHouse, взятый из репозитория 14 августа 2017 года. Для проверки я использовал beta-версию анализатора PVS-Studio v6.17. К моменту публикации статьи уже состоялся релиз этой версии.
Из проверки были исключены следующие каталоги:
- ClickHouse/contrib
- ClickHouse/libs
- ClickHouse/build
- также были исключены различные тесты, например, ClickHouse/dbms/src/Common/tests
Размер оставшегося исходного кода на языке C++ равен 213 KLOC. При этом, 7.9% строк являются комментариями. Получается, что собственного кода, который был проверен, совсем немного: около 196 KLOC.
Как видите, проект ClickHouse имеет меленький размер. При этом качество кода однозначно высокое и у меня не получится написать шокирующую статью. Всего анализатор выдал 130 предупреждений (General analysis, High и Medium сообщения).
Про количество ложных срабатываний дать ответ затрудняюсь. Много предупреждений, которые нельзя формально назвать ложными, но при этом и практической пользы от них нет. Проще всего, это будет пояснить, приведя пример.
int format_version;
....
if (format_version < 1 || format_version > 4)
throw Exception("Bad checksums format version: " + ....);
if (format_version == 1) return false;
if (format_version == 2) return read_v2(in);
if (format_version == 3) return read_v3(in);
if (format_version == 4) return read_v4(in);
return false;Анализатор обращает внимание на то, что если начнётся вычисляться выражение (format_version == 4), то оно будет всегда истинно. Как видите, выше есть проверка, что если значение format_version выходит за пределы [1..4], то генерируется исключение. А оператор return false вообще никогда не выполняется.
Формально, анализатор прав и непонятно, как обосновать, что это ложное срабатывание. С другой стороны, видно, что этот код корректен и просто написан с «запасом надёжности».
В подобных случаях, предупреждения анализатора можно подавить различными способами или переписать код. Например, можно написать так:
switch(format_version)
{
case 1: return false;
case 2: return read_v2(in);
case 3: return read_v3(in);
case 4: return read_v4(in);
default:
throw Exception("Bad checksums format version: " + ....);
}Ещё есть предупреждения, про которые я просто не могу сказать: указывают они на ошибку или нет. Я не знаком с проектом и понятия не имею, как должны работать некоторые фрагменты кода. Рассмотрим такой случай.
Есть некая область видимости с 3 функциями:
namespace CurrentMemoryTracker
{
void alloc(Int64 size);
void realloc(Int64 old_size, Int64 new_size);
void free(Int64 size);
}Названия формальных аргументов функций подсказывают, что функции должны принимать на вход какие-то размеры. Подозрение у анализатора вызывают случаи, когда, например, в функцию alloc передаётся не размер структуры, а размер указателя.
using Large = HyperLogLogCounter<K, Hash, UInt32, DenominatorType>;
Large * large = nullptr;
....
CurrentMemoryTracker::alloc(sizeof(large));Анализатор не знает, ошибка это или нет. Я тоже не знаю, но на мой взгляд, этот код подозрительный.
В общем, про такие случаи я говорить не буду. Если у разработчиков ClickHouse возникнет интерес, они смогут самостоятельно проверить проект и изучить список предупреждений более подробно. Я же в статье рассмотрю только те фрагменты кода, которые показались мне наиболее интересными.
Интересные фрагменты кода
1. CWE-476: NULL Pointer Dereference (3 ошибки)
bool executeForNullThenElse(....)
{
....
const ColumnUInt8 * cond_col =
typeid_cast<const ColumnUInt8 *>(arg_cond.column.get());
....
if (cond_col)
{
....
}
else if (cond_const_col)
{
....
}
else
throw Exception(
"Illegal column " + cond_col->getName() + // <=
" of first argument of function " + getName() +
". Must be ColumnUInt8 or ColumnConstUInt8.",
ErrorCodes::ILLEGAL_COLUMN);
....
}Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'cond_col' might take place. FunctionsConditional.h 765
Здесь неправильно обрабатывается ситуация, когда возникает ошибка. Вместо генерации исключения, произойдёт разыменование нулевого указателя.
Для создания сообщения об ошибке, происходит вызов функции: cond_col->getName(). Этого делать нельзя, так как указатель cond_col будет нулевым.
Аналогичная ошибка обнаруживается здесь: V522 Dereferencing of the null pointer 'cond_col' might take place. FunctionsConditional.h 1061
Рассмотрим другую вариацию на тему использования нулевого указателя:
void processHigherOrderFunction(....)
{
....
const DataTypeExpression * lambda_type =
typeid_cast<const DataTypeExpression *>(types[i].get());
const DataTypes & lambda_argument_types =
lambda_type->getArgumentTypes();
if (!lambda_type)
throw Exception("Logical error: .....",
ErrorCodes::LOGICAL_ERROR);
....
}Предупреждение PVS-Studio: V595 The 'lambda_type' pointer was utilized before it was verified against nullptr. Check lines: 359, 361. TypeAndConstantInference.cpp 359
В начале указатель lambda_type разыменовывается, а только затем осуществляется его проверка. Чтобы исправить код, нужно переместить проверку указателя чуть выше:
if (!lambda_type)
throw Exception("Logical error: .....",
ErrorCodes::LOGICAL_ERROR);
const DataTypes & lambda_argument_types =
lambda_type->getArgumentTypes();2. CWE-665: Improper Initialization (1 ошибка)
struct TryResult
{
....
explicit TryResult(Entry entry_)
: entry(std::move(entry)) // <=
, is_usable(true)
, is_up_to_date(true)
{
}
....
Entry entry;
....
}V546 Member of a class is initialized by itself: 'entry(entry)'. PoolWithFailoverBase.h 74
Из-за опечатки, член entry инициализируется сам собой и в результате на самом деле остаётся неинициализированным. Чтобы исправить код, надо добавить подчеркивание:
: entry(std::move(entry_))3. CWE-672: Operation on a Resource after Expiration or Release (1 ошибка)
using Strings = std::vector<std::string>;
....
int mainEntryClickhousePerformanceTest(int argc, char ** argv)
{
....
Strings input_files;
....
for (const String filename : input_files) // <=
{
FS::path file(filename);
if (!FS::exists(file))
throw DB::Exception(....);
if (FS::is_directory(file))
{
input_files.erase( // <=
std::remove(input_files.begin(), // <=
input_files.end(), // <=
filename) , // <=
input_files.end() ); // <=
getFilesFromDir(file, input_files, recursive);
}
else
{
if (file.extension().string() != ".xml")
throw DB::Exception(....);
}
}
....
}Предупреждение PVS-Studio: V789 Iterators for the 'input_files' container, used in the range-based for loop, become invalid upon the call of the 'erase' function. PerformanceTest.cpp 1471
Контейнер input_files используется в range-based for loop. При этом, внутри цикла, контейнер может изменяться из-за удаления некоторых элементов. Если читателю не понятно, почему так делать нельзя, предлагаю ознакомиться с описанием диагностики V789.
4. CWE-563: Assignment to Variable without Use ('Unused Variable') (1 ошибка)
struct StringRange
{
const char * first;
const char * second;
....
StringRange(TokenIterator token_begin, TokenIterator token_end)
{
if (token_begin == token_end)
{
first = token_begin->begin; // <=
second = token_begin->begin; // <=
}
TokenIterator token_last = token_end;
--token_last;
first = token_begin->begin; // <=
second = token_last->end; // <=
}
};Анализатор выдаёт два предупреждения:
- V519 The 'first' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 26, 33. StringRange.h 33
- V519 The 'second' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 27, 34. StringRange.h 34
При определённом условии вначале переменной first и переменной second присваивается значение token_begin->begin. Далее значение этих переменных в любом случае вновь изменяется. Скорее всего этот код содержит какую-то логическую ошибку или в нём чего-то не хватает. Например, возможно забыт оператор return:
if (token_begin == token_end)
{
first = token_begin->begin;
second = token_begin->begin;
return;
}5. CWE-570: Expression is Always False (2 ошибки)
DataTypePtr
getReturnTypeImpl(const DataTypes & arguments) const override
{
....
if (!((.....))
|| ((left_is_string || left_is_fixed_string) && (.....))
|| (left_is_date && right_is_date)
|| (left_is_date && right_is_string)
|| (left_is_string && right_is_date)
|| (left_is_date_time && right_is_date_time) // 1
|| (left_is_date_time && right_is_string) // 1
|| (left_is_string && right_is_date_time) // 1
|| (left_is_date_time && right_is_date_time) // 2
|| (left_is_date_time && right_is_string) // 2
|| (left_is_string && right_is_date_time) // 2
|| (left_is_uuid && right_is_uuid)
|| (left_is_uuid && right_is_string)
|| (left_is_string && right_is_uuid)
|| (left_is_enum && right_is_enum && .....)
|| (left_is_enum && right_is_string)
|| (left_is_string && right_is_enum)
|| (left_tuple && right_tuple && .....)
|| (arguments[0]->equals(*arguments[1]))))
throw Exception(....);
....
}В этом условии три подвыражения повторяются дважды. Предупреждения PVS-Studio:
- V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_date_time && right_is_date_time)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057
- V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_date_time && right_is_string)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057
- V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_string && right_is_date_time)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057
Возможны два варианта. Первый — ошибки нет, условие просто избыточное и его можно упростить. Второй — здесь ошибка и какие-то условия не проверяются. В любом случае, авторам стоит проверить этот фрагмент кода.
Рассмотрим ещё один случай, когда условие всегда ложно.
static void ipv6_scan(const char * src, unsigned char * dst)
{
....
uint16_t val{};
unsigned char * colonp = nullptr;
while (const auto ch = *src++)
{
const auto num = unhex(ch);
if (num != -1)
{
val <<= 4;
val |= num;
if (val > 0xffffu) // <=
return clear_dst();
saw_xdigit = 1;
continue;
}
....
}Предупреждение PVS-Studio: V547 Expression 'val > 0xffffu' is always false. The value range of unsigned short type: [0, 65535]. FunctionsCoding.h 339
При парсинге строки, содержащей IPv6 адрес, некоторые некорректные IPv6 адреса будут восприняты как правильные. Ожидается, что между разделителями могут быть записаны числа в шестнадцатеричном формате со значением не более FFFF. Если число больше, то адрес должен считаться некорректным. Для выявления этой ситуации в коде имеется проверка "if (val > 0xffffu)". Но она не работает. Переменная val имеет тип uint16_t, а значит не может быть больше 0xFFFF. В результате, функция будут «проглатывать» некорректные адреса. В качестве очередной части адреса будут выступать 4 последние 16-ричные числа до разделителя.
6. CWE-571. Expression is Always True (1 ошибка)
static void formatIP(UInt32 ip, char *& out)
{
char * begin = out;
for (auto i = 0; i < 3; ++i)
*(out++) = 'x';
for (size_t offset = 8; offset <= 24; offset += 8)
{
if (offset > 0) // <=
*(out++) = '.';
/// Get the next byte.
UInt32 value = (ip >> offset) & static_cast<UInt32>(255);
/// Faster than sprintf.
if (value == 0)
{
*(out++) = '0';
}
else
{
while (value > 0)
{
*(out++) = '0' + value % 10;
value /= 10;
}
}
}
/// And reverse.
std::reverse(begin, out);
*(out++) = '\0';
}Предупреждение PVS-Studio: V547 Expression 'offset > 0' is always true. FunctionsCoding.h 649
Условие "offset > 0" выполняется всегда, поэтому всегда добавляется точка. Мне кажется, ошибки здесь нет и проверка просто лишняя. Хотя конечно я не уверен. Если это всё-таки не ошибка, то проверку следует удалить, чтобы она не смущала других программистов и статические анализаторы кода.
Заключение
Возможно, разработчики проекта смогут найти ещё ряд ошибок, просматривая предупреждения анализатора, которые не нашли отражения в статье. Я же закончу повествование, тем более что для того чтобы «передать привет», мне хватило материала :).
В целом хочу отметить высокое качество кода разработчиков проекта ClickHouse. Впрочем, какими высококлассными не были бы разработчики, они не застрахованы от ошибок, и данная статья вновь это демонстрирует. Статический анализатор кода PVS-Studio поможет предотвратить многие ошибки. Наибольший эффект от статического анализа разработчики получают при написании нового кода. Нет смысла тратить время на отладку ошибок, которые можно обнаружить анализатором сразу после проверки нового кода.
Приглашаю всех скачать и попробовать PVS-Studio.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Give my Best Regards to Yandex Developers
Комментарии (82)
tyomitch
05.09.2017 15:35+47С другой стороны, видно, что этот код корректен и просто написан с «запасом надёжности».
Как раз наоборот: этот код так и манит, добавляя/удаляя очередную версию формата, исправить одну из проверок, и забыть исправить вторую.
switchлучше не только тем, что «затыкает» предупреждение, но и «запасом надёжности» в свете будущих изменений кода.
vesper-bot
05.09.2017 15:36Кажется, в ваших обзорах ещё не попадалась диагностика V789. Редкий вид (с) :)
SvyatoslavMC
05.09.2017 15:52У этого проекта достаточно свежий и современный код. Такое не очень часто встречается в проверяемых нами проектах. Обычно у проекта уже есть какое-нибудь наследие, и разрабокта продолжается в том же стиле.
Andrey2008 Автор
05.09.2017 16:01В данном случае, думаю еще влияет то, что диагностика V789 весьма свежая и появилась недавно в PVS-Studio 6.16 (28 июня, 2017).
Dmitry88
05.09.2017 17:05+7Что же за цена, если Yandex не может себе позволить купить себе анализатор?
DenimTornado
05.09.2017 17:11+4Быть может, просто не хотят…
Dmitry88
05.09.2017 17:13-1Ну, не знаю. По идее Яндекс должен обложиться всевозможными анализаторами кода.
Nakosika
05.09.2017 21:59-9Все эти анализаторы кроме пользы добавляют гемор в виде замедления билда, ложных срабатываний, информационного шума и ложного чувства безопасности. Для себя я решил что профессионалам чем меньше таких тулзов тем лучше — гемор легко перевешивает пользу. А нубасам такое тоже особо не поможет — им бы учиться писать юнит тесты, а не тратить время на затыкание варнингов. Возможно, в Яндексе думают так же.
Хотя если тесты гонять перед релизом, то гемор будет меньше, и можно будет выцедить нормальное соотношение время/ошибки, так что хз.
Дисклеймер: пвс я не пробовал.
Andrey2008 Автор
05.09.2017 22:11+31Шикарный комментарий. Спасибо. Я возьму его в рамочку и буду использовать как собирательный образ заблуждений, касающихся статического анализа кода. И дискутировать на эту тему в статьях и конференциях.
P.S. Смайлик ставить не буду, так как я совершенно серьезен. И даже напишу на тему этого комментария маленькую статью-ответ.khim
05.09.2017 22:54+10Не забудьте ещё про ваших реальных конкурентов (не де-юре, а де-факто): динамические анализаторы (ASAN, MSAN, TSAN, UBSAN). У них, конечно, есть беда: срабатывают не так быстро и ресурсов требуют больше. Но зато у них есть и преимущество: если уж *SAN чего-то «такое» у вас при фаззинге нашёл, то отбиваться бесполезно — это ошибка, надо чинить. А со статическим анализатором… всякое бывает.
Так что вам нужно влезть в нишу между встроенным в clang/gcc/msvc анализатором (бесплатно и всегда в наличии) и *SAN'ом (сложно и дорого, но зато ложных срабатываний — почти нуль, любую ошибку можно сразу в багтрекер заносить). Так ли велика эта ниша?Andrey2008 Автор
05.09.2017 23:21Ну сфера статических анализаторов, как бы не бедствует: SonarSource, Synopsys (бывший Coverity), Gimpel Software, Rogue Wave Software так далее :).
И собственно непонятно, прочему речь идёт про «нишу между». clang/gcc/msvc это своего рода статические анализаторы. С этим все просто. Они работают вот так. Мы лучше. С динамическими анализаторами всё тоже просто. Здесь нет конкуренции, а есть взаимное дополнение. В чем-то лучше динамические анализаторы, в чем-то статические. Пример: находим ошибки в Valgrind.khim
06.09.2017 01:18+4С этим все просто.
С этим всё совершенно непросто.
Они работают вот так. Мы лучше.
Ну если бы вы работали хуже, то в вас бы вообще смысла не было. Но тут важно заметить что анализ с помощью clang/gcc/msvc у вас уже есть — всегда, по умолчанию. А вот всё остальное — нужно устанавливать/настраивать отдельно. А если у вас в проекте контрибуторы из кучи других компаний? Сказать им «включите -Wall -Werror» — это одно, а отлавливать ошибки с помощью PVS-Studio (который они и запустить-то не могут!) — совсем другое.
С динамическими анализаторами всё тоже просто. Здесь нет конкуренции, а есть взаимное дополнение.
Это всё «халва». От неё слаще во рту не становится, опять-таки.
Как я уже сказал — если динамический анализатор чего-то нашёл, то «отмазаться» не получится: «на спор» я могу сдеать код, в котором не будет ошибки, но который будет вопить на анализаторе — но на практике я такого ни разу не видел.
А у статического анализатора (любого) есть ложные срабатывания. А если ты ещё и локально код проверить не можешь… В общем — так ли велик выигрыш?
Пример: находим ошибки в Valgrind.
Странный пример. Valgrind под Valgrind'ом запустить нельзя, так что понятно, что в нём самом могут остаться ошибки, которые PVS-Studio может найти. Но это не такому большому количеству народу нужно…
domix32
06.09.2017 02:09+2Даже со всеми санитайзерами не всегда понятно как воспроизвоидится некий гейзенбаг. Статический анализатор позволяет перебдеть и, таки, не допустить ситуации, которая спровоцирует баг.
EvgeniyRyzhkov
06.09.2017 04:49+7Вот Вы вроде и логично пишете (так может подумать сторонний наблюдатель). Но нет.
Статический анализ — это ответ на вопрос «Что ЕЩЕ я могу сделать для того, чтобы улучшить качество кода?». Если компания зарабатывает с помощью своего программного кода, то она стремиться всеми силами улучшить его качество. Качество — это расширяемость, масштабируемость, простота поддержки и др. Так вот, «что ЕЩЕ я могу сделать, чтобы улучшить качество кода?». Внедрить статический анализ! Не надо думать, что это ВМЕСТО чего-то. Вместо тестов, вместо код ревью, вместо динамических анализаторов… Это «что ЕЩЕ».
«А вот в компании XXX нах… не сдался ваш PVS-Studio!» И такое бывает конечно. Далеко не все компании, который пишут программный код, зарабатывают на нем деньги. Например, российская оборонка программный код пишет, но деньги они зарабатывают не на нем. Поэтому в оборонных предприятиях России PVS-Studio не используют.
А компания Epic Games зарабатывает на своем коде — движке Unreal Engine. Поэтому когда встал вопрос «Что ЕЩЕ мы можем сделать, чтобы код Unreal Engine стал качественнее?», они внедрили PVS-Studio.khim
06.09.2017 05:12-1Не надо думать, что это ВМЕСТО чего-то. Вместо тестов, вместо код ревью, вместо динамических анализаторов… Это «что ЕЩЕ».
Так не бывает. Если код вы не пишите для участия в каком-либо конкурсе, где качество — критерий успеха, то вы всегда должны выбирать между улучшением качества кода и чем-то ещё. Можно написать ещё пару тестов, а можно — новую фичу дополнить… что выбрать?
Если компания зарабатывает с помощью своего программного кода, то она стремиться всеми силами улучшить его качество.
Вот только «количество ошибок» — далеко не единственный критерий. Могут быть много других. И уменьшения количества ошибок можно достигать разными средствами — можно вложиться в статический анализатор, а можно — в рефакторинг или более подробный style guide. Что принесёт большую отдачу? Результат — сходу не очевиден, вот совсем не очевиден.
Поэтому когда встал вопрос «Что ЕЩЕ мы можем сделать, чтобы код Unreal Engine стал качественнее?», они внедрили PVS-Studio.
А могли бы вместо этого вложиться в фаззинг — но предпочли PVS-Studio. Вопрос — почему и почему Google поступил иначе?EvgeniyRyzhkov
06.09.2017 06:00+1Так не бывает.
То есть вы ЛИБО пишите тесты, ЛИБО делаете код ревью? И сразу же увольняете того разработчика, который просит сделать код ревью для его тестов? Ведь он тратит время!
Вопрос — почему и почему Google поступил иначе?
Google сам разрабатывает статический анализ. В clang. Мы знаем людей, которые делают это.
Oxoron
06.09.2017 09:02+1Можно написать ещё пару тестов, а можно — новую фичу дополнить… что выбрать?
То есть вы ЛИБО пишите тесты, ЛИБО делаете код ревью?
Вы намеренно делаете ложный вывод, доводя до абсурда аргумент khim, или просто некому проревьюить ваш комментарий перед публикацией?
OlegMax
06.09.2017 13:12Вы намеренно делаете ложный вывод, доводя до абсурда аргумент khim
Зачем доводить? Аргумент khim «Так не бывает» и так достаточно абсурден.
khim
06.09.2017 18:32+1То есть вы ЛИБО пишите тесты, ЛИБО делаете код ревью?
Представьте себе. Я не умею одновременно делать и то и другое.
То что я пишу и тесты и делаю код ревью (но в разное время) просто обозначает что и то и другое посчитали достаточно ценной деятельностью — и потому оставили. Если бы выяснилось что написание тестов в 100 раз эффективнее код ревью (или наоборот) — от одной из этих двух вещей отказались бы.
Google сам разрабатывает статический анализ. В clang. Мы знаем людей, которые делают это.
А это-то вообще тут причём? Да, разрабытывает — потому что автоматический, всегда присутствующий, встроенный в компилятор статический анализ — это хорошо. А вот хорошо ли иметь ещё и отдельных анализатор — это большой вопрос.
MacIn
06.09.2017 12:56Можно написать ещё пару тестов, а можно — новую фичу дополнить… что выбрать?
По сути статанализатор это некая замена (условно, конечно) тем самым паре тестов сверху.
qw1
06.09.2017 13:07+5А могли бы вместо этого вложиться в фаззинг — но предпочли PVS-Studio. Вопрос — почему и почему Google поступил иначе?
Я думаю, это очевидно.
У Гугла приоритет — сетевые сервисы, там ошибка, приводящая к DoS или RCE это очень серьёзно.
Для UE такие ошибки некритичные (если падает при загрузке какой-то кривой модельки, никто и не заметит, потому что модельки загружают туда прямые).
Главное в UE — чтобы графика правильно рисовалась и физика моделировалась. Тут фаззинг бессилен. Ну скормишь рандомные данные в движок, получишь какой-то результат. А как проверить, что результат верный?
HSerg
06.09.2017 08:43Сертификационные лаборатории активно пользуются статическими (как и динамическими) анализаторами, так что утверждение не совсем корректно.
EvgeniyRyzhkov
06.09.2017 08:58Давайте не будем про сертификационные лаборатории? Дай им всем Бог здоровья и процветания!
Nakosika
07.09.2017 09:33Пишу на яве, может у вас лучше, я честно написал. :)
Описывал свою ежедневную боль. В нашей компании принято что гит хуки запускают поверки, и понеслась. Ни одной ошибки у меня за год не нашли, а каждый пуш какие-то проблемы. При том открываю приложение, и за пять минут которые тратится на поверки, могу с лету найти три бага — в большой компании баги добавляются с невероятной скоростью.
В общем, пользуйтесь, рад что повеселил, хоть смайлик и не поставили. :D
vlivyur
06.09.2017 09:38А в юнит тестах не бывает ошибок?
SvyatoslavMC
06.09.2017 10:31Бывает и много. Из статей обычно исключаются.
Бывает ещё хуже: как-то исправление настоящей ошибки по нашему отчёту привело к завалу сотни тестов в одном проекте. Отличные тесты были, не правда ли?
khim
05.09.2017 18:56+3Вопрос не в цене анализатора. Вопрос в цене внедрения и поддержки. Ложные срабатывания стоят денег, интеграция стоит времени и т.д. и т.п.
Не думаю что проблема в цене лицензии, Яндекс точно может себе её позволить.humbug
06.09.2017 03:29+5Ну-ну, написал я им, внедрил в линукс билды по триальному ключу, интегрировал, написал маны для соратников. А когда дошёл разговор до платных лицензий, они &??!@"&!/?-&!? Ой, чуть NDA не сдержал.
khim
06.09.2017 03:37То, что у Яндекса есть деньги — вовсе не значит, что они готовы им разбрасываться.
Говорить о цене стоило до всего остального. Ибо вполне может оказаться, что у них, например, в билд-ферме 10000 машин — и что, для каждой лицензию покупать?
Триальная лицензия — это уж точно не для интеграции и для написания манов для сотрудников…humbug
06.09.2017 03:47Да, вы правы. Договариваться стоило на берегу. С другой стороны, хотелось убедить своё начальство аргументами "смотрите, как круто это работает на нашем проекте! Вот нашло! И вот нашло!", но ?&@?!'-/@&?):;@/"?&@?
EvgeniyRyzhkov
06.09.2017 04:42&??!@"&!/?-&!?
Не смог расшифровать. Мы что-то сделали не так? Как мы можем это исправить?
arabesc
06.09.2017 10:51Сделайте исключение сорцов per project, а не глобально, с поддержкой раскрытия переменных окружения из VS-проектов для формирования полных путей вместо масок.
billyevans
05.09.2017 21:39А откуда у вас такая информация? Возможно они coverity используют или еще что-нибудь?
old_bear
05.09.2017 17:39+51Приблизительно раз в полгода нам пишет кто-то из сотрудников компа
нии Yandex, интересуется лицензированием PVS-Studio, качает триал и пропадает.
Видимо у них в плане работ стоит проверка анализатором каждые полгода.
tropinkin
05.09.2017 22:16+5Разработчикам PVS-Studio: очень понимаю сотрудников Yandex — с ценовой политикой на продление лицензий вида "а давайте увеличим стоимость продления лицензии PVS-Studio на рандомную существенную величину" уже не первый год подряд и как результат проблематичностью бюджетирования, также начинаем поглядывать на альтернативы.
EvgeniyRyzhkov
06.09.2017 04:54-1Не понимаю о чем речь. Ценовая политика в плане продлений не менялась много лет. 80% от цены новой лицензии.
AllexIn
06.09.2017 07:11+1А новая цена тоже не менялась много лет?
EvgeniyRyzhkov
06.09.2017 07:15-1Конечно менялась! У нас появился C# анализатор, у нас появился C++ анализатор под Linux, поддержка SonarQube, интеграция с CI и многое другое. Сотни диагностик, сотни страниц документации.
Нет никакой возможности продавать по цене пятиледней давности.
Однако если вы не просто так в комментах пообсуждать, а по делу, то напишите мне и я расскажу о способах справиться с указанной бедой.AllexIn
06.09.2017 07:31+5А почему бы цену продления не делать фиксированной в момент покупки?
Но я так, просто в комментах пообсуждать. Хоть я бы и очень хотел купить ваш продукт себе и на своей работе — возможности такой нет. Для собственного использования слишком жирно, а на работе сейчас тоже бюджета нет.
Так что я вас без коммерческого потенциала отвлекаю.EvgeniyRyzhkov
06.09.2017 09:00-9А почему бы цену продления не делать фиксированной в момент покупки?
Но я так, просто в комментах пообсуждать.
Извините, Хабр — технический ресурс и если мы будем обсуждать нюансы бизнеса, то это будет не интересно аудитории.Oxoron
06.09.2017 09:15+20Извините, Хабр — технический ресурс и если мы будем обсуждать нюансы бизнеса, то это будет не интересно аудитории.
Друг мой, аудитория годами читает Milfgard'a и нюансы бизнеса МосИгры (это как яркий пример, есть еще вернувшийся Мегамозг).
Возвращаясь же к топ-комментарию:
Конечно менялась! У нас появился C# анализатор, у нас появился C++ анализатор под Linux, поддержка SonarQube, интеграция с CI и многое другое. Сотни диагностик, сотни страниц документации.
Эта фраза может произвести двоякое впечатление. Можно возразить что-то вроде "с чего мне платить за SonarQube и С++ анализаторы под Linux? У меня .NET проекты!!!111".
AllexIn
06.09.2017 10:40+16Слушайте, ну мы все взрослые люди и понимаем, что у вас есть определенный бизнес процесс и определенные причины не обсуждать ценовую политику публично.
Но, попытка это прикрыть интересами сообщества выглядит толстым таким лицемерием и воспринимается крайне негативно.
Alexsey
06.09.2017 10:42+12У нас появился C# анализатор, у нас появился C++ анализатор под Linux, поддержка SonarQube, интеграция с CI и многое другое.
«С этого года во всех наших автомобилях установлены крылья и ракетные двигатели, при приезде на ТО мы в обязательном порядке их ставим и вы обязаны за них заплатить даже если они вам не нужны».
Примерно так это звучит. Зачем компании, которая пишет на плюсах под винду версия под Linux и C# анализатор? Сделайте это отдельной лицензируемой функциональностью, кому надо — тот купит.
erlyvideo
05.09.2017 23:05+1а вы код эрланга (виртуальной машины) не проверяли?
Andrey2008 Автор
05.09.2017 23:23+1Пока нет.
P.S. Обновляемый список статей, в которых мы рассказываем об ошибках, найденных с помощью PVS-Studio в открытых проектах.
P.P.S. Предлагать проекты на провреку лучше всего здесь.
lexxmark
06.09.2017 01:52+4А у вас есть диагностика на использование значения, которое переместили?
Что-то вроде этого:
std::vector<int> a = ...; ... auto b = std::move(a); ... auto s = a.size(); // ???AllexIn
06.09.2017 07:32std::move ничего не перемещает.
AllexIn
06.09.2017 07:36Это я к тому, что a остался вполне себе валидным объектом.
Fil
06.09.2017 09:22Но это только стандартная библиотека гарантирует «valid but unspecified state». Для прочих объектов (согласно стандарту) валидность не требуется.
lexxmark
06.09.2017 10:22+3Валидный останется объект или не валидный — не очень важно.
Главное, семантика перемещения. Объект а после перемещения хранит непонятно что (если move реализован через swap, то в а лежит содержимое b), и вроде нет никаких причин работать с а, кроме как присвоить ему новое значение.
Мне кажется это главным изъяном операции перемещения введеной в С++11 — появление объектов с непонятным состоянием, которые по идее должны закончить свою область видимости после перемещения.
Maccimo
06.09.2017 01:56Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод
«Перевод» очень похож на немного изменённый результат google translate.
Учитывая, что англоязычный сегмент рынка больше русскоязычного, такой подход выглядит странно.EvgeniyRyzhkov
06.09.2017 04:51-2Что делать? Подскажите как улучшить ситуацию.
ivlis
06.09.2017 07:08+4Выучить английский; нанять того, кто знает английский. Масса вариантов.
o6CuFl2Q
06.09.2017 06:25+16Спасибо, анализ полезный! Поправили указанные ошибки:
github.com/yandex/ClickHouse/pull/1204
(Одной из ошибок уже не было в master, так как анализировалась более старая версия кода.)
sand14
06.09.2017 07:44+1Анализатор отличная вещь, однако, он не заменит голову программиста, и, прежде чем начинать им пользоваться, хорошо бы писать код так, чтобы компилятор не выдавал стандартных ворнингов (а где вы видели это в больших проектах?).
Банальные вещи: переопределение в наследнике унаследованного невиртуального метода (нарушение полиформизма, и различное поведение, в зависимости от того, к какому типу объектив приведен в статике), смешение знаковых и беззнаковых целых, включая случай еще и разного размера (при желании можно все решить в статике, чтобы не было неожидаемого поведения), тысячи их.
Штука в том, что профессия разработчика/программиста была, есть и будет инженерной, где требуется квалификация и здравый смысл.
А ее всячески пытаются запихнуть в ремесленничество с помощью как организационных, так и технических средств.
Все это нужные вещи, конечно, но делаются не в той последовательности.MacIn
06.09.2017 13:01хорошо бы писать код так, чтобы компилятор не выдавал стандартных ворнингов (а где вы видели это в больших проектах?).
Большие проекты — это сколько по-вашему?
Штука в том, что профессия разработчика/программиста была, есть и будет инженерной, где требуется квалификация и здравый смысл.
А ее всячески пытаются запихнуть в ремесленничество с помощью как организационных, так и технических средств.
Точно так же как и автоматическая коробка передач на атомобиле, стиральна машина-автомат и тому подобное. Низводят людишек до сосотояния операторов.
IronHead
06.09.2017 12:36+3На предыдущей работе писал на чистом «C» среда разработки VisualDSP++, после очередной статьи с вашими хвалебными речами о PVS-Studio решил попробовать протестировать свои проекты. А вот хрен. Не работает PVS-Studio в связке с VisualDSP++. Ну думаю ладно, надо на чем то еще попробовать. Нашел код написанный под AVR опять же на чистом «C». Код писался в «блокноте» и собирался в AVR GCC — то же облом. Ну думаю, дай попробую проекты, которые писал под STM32 на «С» и собирал в IAR. Но и тут какая то лажа.
SvyatoslavMC
06.09.2017 13:03-2Вы выкатили список нестандартных тулчейнов. Понятное дело, что у подобного может не быть заявленной поддержки, и Вы испортили впечатление о продукте на пустом месте. Подобную информацию следует изучать в документации и поддержке заранее.
IronHead
06.09.2017 13:16+4Ок. Идем на сайт PVS-Studio https://www.viva64.com/ru/t/0046/
Читаем:
Другие преимущества статического анализа кода:
…
2. Статический анализ не зависит от используемого компилятора и среды, в которой будет выполняться скомпилированная программа. Это позволяет находить скрытые ошибки, которые могут проявить себя только через несколько лет. Например, это ошибки неопределенного поведения. Такие ошибки могут проявить себя при смене версии компилятора или при ...SvyatoslavMC
06.09.2017 13:34-3Там объёмный текст о преимуществах и недостатках статического анализа кода как методологии. Вы даже заголовок процитировали. Опрометчивая претензия.
IronHead
06.09.2017 13:38+1Быстрый старт в Windows и Linux
…
Отметим только, что в PVS-Studio для Windows и Linux предусмотрены специальные утилиты, собирающие информацию о запусках компилятора. Эти инструменты позволяют быстро выполнить анализ проекта, собираемого любым способом. Вы можете быстро познакомиться с возможностями анализатора, не тратя время на его интеграцию с makefile или сборочным скриптом. См. описание утилиты Standalone (Windows) и pvs-studio-analyzer (Linux).
Это тоже опрометчивая претензия?SvyatoslavMC
06.09.2017 13:48-4Отчасти. Я же не знаю на сколько давно Вы там работали. Эти механизмы достаточно новые, сайт обновляется. Уже поздно копать в эту строну, но я рад, что Вы всё-таки решили ознакомиться с описанием и документацией.
IronHead
06.09.2017 14:03+1Первый раз пробовал PVS-Studio примерно год назад (на DSP++ и GCC), второй раз (на IAR-е) сегодня. Поэтому и пишу, что ничего не изменилось.
AVX
06.09.2017 15:36-3Интересно, а когда-нибудь проверяли исходники PVS-Studio при помощи самой PVS-Studio?
newpy
06.09.2017 20:30Интересно, а не было попыток анализировать с помощью PVS-Studio проекты и код на LISP/Clojure/Racket/Scheme/...?
Mikihiso
Он возможно будет нулевым, но не всегда из-за наличия второго условия. Думаю там просто стоит добавить дополнительную проверку перед выбросом исключения.
Andrey2008 Автор
Когда выполняется «throw Exception» указатель не возможно нулевой, а точно нулевой. Посмотрите повнимательнее на код.
Yarique
А если getName — static функция?
Andrey2008 Автор
znsoft
я не обладаю плюсами, но разве компилятор не выдаст ошибку компиляции при попытке получить статический метод от экземпляра класса, а не от самого класса?
по моему если код компилируется то getName явно не статичен
Yarique
$ g++ -std=c++17 ./testLol.cpp && echo $?
0
$ ./a.out
lol
SvyatoslavMC
$ g++ -std=c++17 ./testLol.cpp && echo $?
0
$ ./a.out
lol
khim
nullptr).