Исправления срабатываний статического анализатора.

[Maxim Dounin]

Hello!

On Tue, Oct 04, 2022 at 11:33:14PM +0700, Eugene Grosbein wrote:

> 04.10.2022 20:11, Evgeniy Berdnikov пишет:
> > On Tue, Oct 04, 2022 at 12:00:57PM +0000, Korobov Vladimir via nginx-ru wrote:
> >>    После проверки исходного кода статическим анализатором (Svace
> >>    https://www.ispras.ru/technologies/svace/) выделено несколько потенциально
> >>    опасных мест, закрывающихся приложенным патчем.
> > 
> >  Тупое выбрасывание кусков кода при проверке указателя на NULL не только
> >  не решает проблему, но создаёт более опасную ситуацию, когда код приложения
> >  может работать неверно, но ничего об этом не сообщать, так что поймать баг
> >  станет очень трудно. Лучше сегфолт в в точно локализованном месте, чем
> >  глюки непонятно где и непонятно отчего.
> > 
> >  При потенциальной возможности зануления указателя следует ловить и
> >  обрабатывать такое исключение. В противном случае нет смысла в проверке.
> >  Задача же не в ублажении тупых анализаторов, а в правильной работе кода.
> 
> Как пользователь разнообразного софта, могу доложить, что сегфолт очень фиговая обработка исключений.
> Проверка указателя на NULL перед разадресацией в том случае, когда нельзя гарантировать что он не NULL,
> практически всегда благо. Другой вопрос, что потом делать, если вдруг: молча восстановиться и ехать дальше,
> или не молча, а с сообщением в лог, или выдать даже stack trace и выйти. Но что угодно лучше сырого сегфолта.

Так о том и речь, что в данном случае есть, потенциально, два 
варианта:

- Статический анализатор прав, и в рассматриваемых местах возможно 
  появление нулевых указателей.  Тогда ситуацию надо обрабатывать, 
  а не тупо игнорировать кусок кода.  То есть патч плохой.

- Статический анализатор не прав, и в рассматриваемых местах 
  появление нулевых указателей невозможно (ну, кроме случая memory 
  corruption).  Тогда добавлять проверки с игнорированием кода опять 
  же неправильно: они ничего не будут делать в штатном режиме, а в 
  случае memory corruption - ещё и увеличивают шанс получить code 
  execution вместо segmentation fault.  То есть патч опять же 
  плохой.

Ситуаций, когда предлагаемый патч может быть хорошим - не 
прослеживается.

Вообще, по моему опыту - в большинстве случаев статические 
анализаторы, даже лучшие, приносят мусор.  Но иногда случается и 
что-нибудь полезное, поэтому жалобы какого-нибудь Coverity - 
регулярно отсматриваются.  Но надо понимать, что анализ жалоб 
какого-либо статического анализатора - это кропотливый труда, а не 
взять и заткнуть тупыми проверками всё, на что оно вдруг решило 
пожаловаться по каким-то своим внутренним причинам.

-- 
Maxim Dounin
http://mdounin.ru/