Комментарии 53
Мне больше интересно, что у них за компилятор такой. Предупреждения на "нет возврата в non-void функции" и "переменная используется неинициализированной" вроде бы уже любой нормальный компилятор сам выдает.
SequenceNumberManager &operator=(SequenceNumberManager &&Other) {
NextSequenceNumber = std::move(Other.NextSequenceNumber);
FreeSequenceNumbers = std::move(Other.FreeSequenceNumbers);
}И неинициализированные переменные тоже:
static Error mapNameAndUniqueName(....) {
....
size_t BytesLeft = IO.maxFieldLength();
if (HasUniqueName) {
....
if (BytesNeeded > BytesLeft) {
size_t BytesToDrop = (BytesNeeded - BytesLeft);
size_t DropN = std::min(N.size(), BytesToDrop / 2);
size_t DropU = std::min(U.size(), BytesToDrop - DropN);
....
}
} else {
size_t BytesNeeded = Name.size() + 1;
StringRef N = Name;
if (BytesNeeded > BytesLeft) {
size_t BytesToDrop = std::min(N.size(), BytesToDrop); // <=
N = N.drop_back(BytesToDrop);
}
error(IO.mapStringZ(N));
}
....
}Ндаа. Знаете, смеяться как-то не очень хочется.
Но это как-то совсем странно. Предупреждения отключены что ли у них? Или их просто не читают?
- Мы ведь не зря берём деньги за PVS-Studio :). Мы рассматриваем больше ошибочных паттернов и больше конструкций, в которых ошибка может проявить себя. Соответственно мы опережаем возможности компилятора. Компиляторы развиваются, и возможно сейчас Clang уже смог бы сам в себе обнаружить эту ошибку. Но ведь и мы на месте не стоим и за это время научились делать много всякого нового :). См. про технологии и развитие.
- Мы очень серьёзно работаем над сокращением количества ложных срабатываний. Многие анализаторы и компиляторы сильно не заморачиваются на эту тему. Вроде диагностика есть и ладно. Но фича в том, что если ложных срабатываний много, то сообщения этого типа просто перестают смотреть или отключают. Поэтому мы часто замечаем то, что просто не смотрят в силу неудобства. Эта мысль подробнее.
- Программисты считают, что не допускают таких ошибок :).
Ну, с первым можно слегка поспорить — ибо есть некоторые предупреждения, которые компиляторы выдают надежнее, чем PVS (например, "не все поля класса были проинициализированы в конструкторе").
Но в целом вы правы, конечно.
Обмануть анализатор всегда можно, если захотеть.
Если не ошибаюсь, Haiku до сих пор собирают компилятором GCC v2.9, чтобы иметь совместимость с BeOS. А когда она будет достигнута, будет совершен переход на что-то свежее (хотя бы не мертвое).
Полагаться на анализатор - такое себе. А неинициализированные переменные - вещь. У меня даже проект был - сделай так, чтобы софт работал одинаково на разных компах.
Кстати, а почему насчет std::move не написано, там можно так накосорезить и найти ошибку чрезвычайно сложно.
delete fBigIcon;
fBigIcon = NULL;
fVectorIcon = NULL; // <=
free(fVectorIcon); // <=Из этого куска не ясно, но есть подозрение что вместо free нужно было вызывать delete.
Скорее всего, в коде перепутали операторы '|' и '||'. Такая ошибка приводит к лишним вызовам функции SetDecoratorSettings.
А по-моему, это обычное применение паттерна Observer. Интересно, если бы запись была вида "changed |= listener->SetDecoratorSettings(window, settings);"
сработало бы предупреждение?
Учитывая специфику проекта — что это ОС, которую пишут с 2002 года — это не так плохо.
За это время стандарт С++ неоднократно менялся.
case HASH_EMPTY_DELETE:
// TODO: destructors are not called!
delete entry->value;
break;
case HASH_EMPTY_FREE:
free(entry->value);
break;Ну вот это же натуральный бардак, как-будто разные люди в рандомном порядке подходили к столу и просто дописывали куски кода как вздумается.
Или вот такое
void* _GetBuffer()
{
....
void* buffer = malloc(fBufferSize);
if (buffer == NULL && !fBuffers.AddItem(buffer)) {
free(buffer);
throw std::bad_alloc();
}
return buffer;
}malloc/free никаких о ctor/dtor не знают и они не вызываются, а это верный путь для того чтобы с разбегу запрыгнуть на грабли. Ну и печальный коммент "// TODO: destructors are not called!" как итог. И правда, почему же деструкторы не вызываются.
Никаким использованием C функций не оправдывается вообще.
Во втором примере кроме описанной в статье ошибки пара malloc-free вроде нормально используется.
Вызов конструктора это жесть. Они из мира Джавы что ли пришли?
Переменная rval не была инициализирована при объявлении, поэтому её сравнение с нулевым значением приведёт к неопределённому результату.
Это при условии, если выполнится тело if, а выполнится оно только если кто-то вызовет longjmp(), но так как longjmp() явно не выполнится до того как rval инициализируется в следующем за if оператором for, то это и не ошибка вовсе:
...
if (sigsetjmp(toplevel, 1)) {
if (connected)
disconnect(0, NULL);
if (rval > 0)
rval = argpos + 1;
return (rval);
}
(void)xsignal(SIGINT, intr);
(void)xsignal(SIGPIPE, lostpeer);
/*
* Loop through as long as there's files to fetch.
*/
for (rval = 0; (rval == 0) && (argpos < argc); argpos++) {
...
Да, запутано и не «в лучших традициях», но всё же rval не будет иметь неопределенного значения. Видимо, ваш анализатор не учитывает как выполняется setjmp().
PS: Было бы очень здорово если бы при публикации примеров были ссылки на оригинальные файлы (хотя бы пути в исходниках), а то пойди найди этот fetch.c в коде haiku — в мастере его уже нет…
Я читал в переписке разработчиков, что ftp компонент они просто выкинут из-за ошибок и возьмут другой. К привязке к коду не могу сказать. Но вы можете легко восстановить файл по истории коммитов.
Но Вы свою ошибку анализа setjmp(), а, быть может, и иных методов обработки исключений, исправлять запланировали ли?
Да, запутано и не «в лучших традициях», но всё же
Наверняка что-то осталось. Но если Ваш код нельзя описать этим предложением, то результаты анализа будут лучше. Как и мнение коллег по цеху)
Согласно стандартов C/C++, а setjmp() часть стандартов, этот фрагмент кода не содержит обращения к неинициализированной переменной и имеет вполне определённое поведение.
Какая-то антиреклама получается.
P.S. А, скажем, в таком коде:
int cnt;
try {
for(cnt = 0; cnt < n; cnt++) {
...
}
} catch(...) {
return(cnt);
}
Он тоже обнаружит неинициализированную переменную?!
В теории до for может вылететь эксепшон и в катче вернется косячный результат.
Хм, как бы, исключение это же не асинхронный сигнал или прерывание.
Если бы cnt имел бы тип не int, а нечто, определяющее оператор присваивания без квалификатора noexcept, то могут быть разные варианты. Хотя, и в этом случае экземпляр класса всё равно будет инициализирован.
Но оператор присваивания для типа int не имеет права вызывать исключения.
Код всегда можно вставить перед фором и не обратить внимание на неинициализацию инта. Eссно позже, когда замысел благополучно забыт.
if(0 == setjmp(buf, 1)) {
for(cnt = 0; cnt < n; cnt++) {
...
}
} else {
return(cnt);
}
А путаница — setjmp()/longjmp() имеют массу подводных камней (да ещё и в сочетании с обработчиками сигналов), их использование как poor man's exception handling несколько опасно и чревато боком. Впрочем, если принять во внимание остальной код из Haiku, это наименьшая из их проблем…
Вообще-то после вызова delete содержимое памяти по указанном адресу останется прежним, поэтому его еще можно безопасно использовать. Проблемы могут возникнуть только после следующего вызова new.
</irony-mode-off>
содержимое памяти по указанном адресу останется прежнимне факт, но скорее всего
его еще можно безопасно использоватьпри некоторых условиях — да. Поскольку память выделяется страницами, если на той-же странице ещё остались выделенные блоки то ко всему содержимому страницы ещё можно «безопасно» обращаться. Увы это не долго продержится, и если это был последний блок в странице, то первое же обращение выдаст ошибку
Проблемы могут возникнуть только после следующего вызова newне только, если страницу освободят то возникнут до этого, если удаляли ещё несколько объектов, то и после new могут не возникнуть.
Самое плохое, что в таком «особом» режиме обращения к освобождённой памяти программа может очень долго работать без ошибок. А когда ошибка возникнет — то её не так просто повторить. Есть способы отловить такую ошибку — как простые, например менеджер памяти, который выделяет по странице на объект (считаем что объект меньше страницы), так и более продвинутые.
Большое спасибо за развернутое разъяснение. Абсолютно согласен с вами насчет того, что это не просто выстрел в ногу, это скорее мина в рюкзаке, готовая взорваться от любой встряски.
Помнится, когда еще не было таких хороших компиляторов и статических анализаторов, я писал свои кастомные free, malloc, delete и new, где принудительно после выделения памяти, заполнял ее символами ‘+’, а после освобождения — символами ‘-‘, и это позволяло быстро обнаружить такие косяки, как и выход за границы буфера.
Потом, когда я все эти косяки отловил, то переписал весь свой код на смартпоинтерах, чтобы вообще нигде явно не вызывать ни delete, ни free.
А есть сложные, интересные ошибки? Такие что с первого взгляда и не кажутся ошибками, а скорее ложными срабатываниями анализатора, или например ошибки которые сложно обнаружить «методом пристального взгляда» но анализатор их находит? Конечно такую подборку сделать сложно, но кажется, она была бы полезна всем. В том числе и в качестве качественной рекламы.
Заранее прошу прощения если такая подборка уже есть, за всем не уследить.
Как выстрелить себе в ногу в C и C++. Сборник рецептов Haiku OS