kirill_85 Jun 6 2016 at 15:55

Проверяем исходный код 7-Zip с помощью PVS-Studio

10 min

30K

PVS-Studio corporate blog

+41

Comments 25

aquamakc Jun 6 2016 at 17:47

Проверил свой код на C# PVS`ом. Обратил внимание, что единорог не совсем понимает конструкцию:

if (changeDevNum != null)
changeDevNum(num);

где changeDevNum — событие.
V3083 Unsafe invocation of event 'changeDevNum', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. Driver.cs 246
Не обращая никакого внимания, что здесь-же происходит проверка на null.

-1

gmvbif Jun 6 2016 at 18:15

Думаю подразумевается состояние гонки, в которой другой поток может изменить содержимое changeDevNum. Хотя там и локальная переменная не спасает вроде.

shaman_timon Jun 6 2016 at 18:23

Ну она спасёт, если сделать
var handler = Thread.VolatileRead(ref changeDevNum);
if(handler!=null)
handler(num);
Т.к. в противном случае по стандарту, компилятор может соптимизировать код удалив локальную переменную, но по факту такого нет ни в Mono, ни в .Net

gmvbif Jun 6 2016 at 18:32

Event это же цепочка делегатов? Операции над ней (присваивание и вызов) защищены от модификации из другого потока?

shaman_timon Jun 6 2016 at 18:40

Честно говоря не понял что вы имеете ввиду, Thread.VolatileRead(ref changeDevNum) просто несёт «побочную» функцию защиты от оптимизации компилятора.
А что вы имеете ввиду про защиту от модификаций из другого потока? Можно модифицировать в любом потоке.

gmvbif Jun 6 2016 at 18:58

Я имею ввиду атомарность операций присваивания и вызова event. Может ли другой поток поменять список делегатов, пока они вызываются другим. Не приведет ли это к исключительной ситуации.

shaman_timon Jun 6 2016 at 19:30

Приеду домой проверю, но по-моему, т.к. события и делегаты immutable, то поменять то поменяет список(т.е. создаст новый и заменит), а тот старый выполнится до конца.

Gremlin2 Jun 6 2016 at 18:15

И всё же он прав, между проверкой и вызовом может произойти много чего, например, отписка от события из другого потока.

shaman_timon Jun 6 2016 at 18:15

Просто такой вызов действительно может вызвать NullReferenceException.
Вызов события производят обычно таким образом

var handler = changeDevNum;
if(handler !=null)
handler(num);

Ну или в C# 6.0 handler?.Invoke(num).

aquamakc Jun 6 2016 at 20:38

var handler = changeDevNum;
А это разве не передача по ссылке?

yar3333 Jun 6 2016 at 21:07

Да, присваивание ссылки — её мы как раз и спасаем в локальной переменной, которую (переменную) из другого потока не достать.

aquamakc Jun 7 2016 at 09:22

Проверять сейчас не с руки, но сдаётся мне, этот финт ушами спасает только от излишне активного GC.

-5

steck Jun 6 2016 at 18:49

Мне кажется, как раз понимает и предупреждает о возможной ошибке при многопоточности.

PsyHaSTe Jun 7 2016 at 15:29

Правда, ничего хорошего от этого не произойдет. В лучшем случае переложим ошибку из одного места в другое. Как уже говорилось, эта задача принципиально неразршима. Лично я предпочитаю способ №4

-1

fgrdn Jun 6 2016 at 17:47

проблемы с беззнаковыми возникли скорее всего после какого-то рефакторинга. был int, решили более строго типизировать и накидали uint32. только про проверки забыли.
вообще все, что есть в статье — зачастую присуще любому проекту. новые проекты какое-то время защищены от подобных ошибок, но со временем они могут возникнуть.
PS: я уже подзабыл, но вроде в с++98 можно было проверять возвращаемое значение new на null. это с 03 уже ввели nothrow.

AgentRX Jun 6 2016 at 17:47

> Если оператор new не смог выделить память, то согласно стандарту языка C++, генерируется исключение std::bad_alloc()
1. Можно выставить обработчик ошибок new (см. set_new_handler)
2. Генерацию исключений можно вообще исключить, тогда new вернет nullptr