Emby — довольно известный медиасервер, наравне с Plex и Kodi. В этой статье мы попробуем проверить его исходный код при помощи статического анализатора PVS-Studio. На официальном сайте Emby есть маленькая пометочка "Built with ReSharper". Что же, так даже интереснее.
PVS-Studio работает в 64-битных системах на Windows, Linux и macOS. Он умеет искать ошибки в исходном коде программ, написанных на языках С, C++, C# и Java.
Emby — медиасервер, исходный код которого можно найти на GitHub. Он позволяет организовывать трансляции и дает возможность получить доступ к медиаконтенту (видео, аудио, фото) с любого устройства. Выжимка из официального сайта Emby:
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'c != '<'' to the left and to the right of the '&&' operator. HttpListenerRequest.Managed.cs 49
internal void SetRequestLine(string req)
{
....
if ((ic >= 'A' && ic <= 'Z') ||
(ic > 32 && c < 127 && c != '(' && c != ')' && c != '<' && // <=
c != '<' && c != '>' && c != '@' && c != ',' && c != ';' && // <=
c != ':' && c != '\\' && c != '"' && c != '/' && c != '[' &&
c != ']' && c != '?' && c != '=' && c != '{' && c != '}'))
continue;
....
}Анализатор обнаружил дважды повторяющееся подвыражение c != '<'. Возможно, в этот код закралась ошибка, и на месте '<' должно было находиться нечто иное. Или, что более вероятно, второе подвыражение лишнее и его надо просто удалить.
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'SmbConstants.AttrHidden' to the left and to the right of the '|' operator. SmbComDelete.cs 29
internal SmbComDelete(string fileName)
{
Path = fileName;
Command = SmbComDelete;
_searchAttributes = SmbConstants.AttrHidden |
SmbConstants.AttrHidden |
SmbConstants.AttrSystem;
}Анализатор опять нашел опечатку, связанную с повторяющимися подвыражениями. Забегая вперед, скажу, что в этом проекте что-то многовато подобных ошибок, допущенных по невнимательности. Ничуть не упрекаю в этом создателей Emby, все мы иногда бываем рассеянными подобным образом (примеры, примеры, примеры), и, чтобы перестраховаться от подобных ошибок, как раз и существует статический анализ.
Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. SqliteItemRepository.cs 5648
private QueryResult<Tuple<BaseItem, ItemCounts>> GetItemValues(....)
{
....
if (typesToCount.Length == 0)
{
whereText += " And CleanName In (Select CleanValue
from ItemValues where "
+ typeClause + " AND ItemId in (select guid from TypedBaseItems"
+ innerWhereText + "))";
}
else
{
//whereText += " And itemTypes not null";
whereText += " And CleanName In (Select CleanValue
from ItemValues where "
+ typeClause + " AND ItemId in (select guid from TypedBaseItems"
+ innerWhereText + "))";
}
....
}А это очень похоже на проблему, появившуюся из-за копипасты, т.к. тела блоков if и else полностью идентичны. Зачем нужно было проверять размеры массива typesToCount, если вне зависимости от количества элементов, записанных в нём, будет выполнен один и тот же код - известно только разработчикам.
Предупреждение PVS-Studio: V3005 The '_validProviderIds' variable is assigned to itself. BaseNfoParser.cs 77
private Dictionary<string, string> _validProviderIds;
....
public void Fetch(....)
{
....
_validProviderIds = _validProviderIds = new Dictionary<....>(....);
....
}Опять же опечатка, которая повлекла за собой присвоение значения переменной самой себе. Стоит перепроверить этот участок кода.
Предупреждение PVS-Studio: V3008 The 'Chapters' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 29, 28. Title.cs 29
public Title(uint titleNum)
{
ProgramChains = new List<ProgramChain>();
Chapters = new List<Chapter>();
Chapters = new List<Chapter>();
TitleNumber = titleNum;
}Невнимательность и опечатки, опять... Переменной Chapters дважды присваивается значение. Понятно, что ничем плохим этот лишний код не грозит, но тем не менее. Задерживаться на этом примере смысла нет, давайте пойдём дальше.
Предупреждение PVS-Studio: V3013 It is odd that the body of 'Read' function is fully equivalent to the body of 'Write' function (407, line 415). BaseSqliteRepository.cs 407
public static IDisposable Read(this ReaderWriterLockSlim obj)
{
return new WriteLockToken(obj);
}
public static IDisposable Write(this ReaderWriterLockSlim obj)
{
return new WriteLockToken(obj);
}
private sealed class WriteLockToken : IDisposable
{
private ReaderWriterLockSlim _sync;
public WriteLockToken(ReaderWriterLockSlim sync)
{
_sync = sync;
sync.EnterWriteLock();
}
public void Dispose()
{
if (_sync != null)
{
_sync.ExitWriteLock();
_sync = null;
}
}
}Тела функций Read и Write абсолютно идентичны друг другу, о чем и говорит текст предупреждения анализатора. Метод EnterWriteLock нужен для того, чтобы выполнить вход в блокировку в режиме записи. Для режима чтения стоит воспользоваться методом EnterReadLock, допускающим возможность одновременного чтения ресурса несколькими потоками.
Разработчику стоит повнимательнее присмотреться к этому участку кода, возможно, тут допущена ошибка. Тем более, в коде встретился следующий класс, который не используется:
private sealed class ReadLockToken : IDisposable
{
private ReaderWriterLockSlim _sync;
public ReadLockToken(ReaderWriterLockSlim sync)
{
_sync = sync;
sync.EnterReadLock();
}
public void Dispose()
{
if (_sync != null)
{
_sync.ExitReadLock();
_sync = null;
}
}
}Предупреждение PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless SkiaEncoder.cs 537
public string EncodeImage(string inputPath,
DateTime dateModified,
string outputPath,
bool autoOrient,
ImageOrientation? orientation,
int quality,
ImageProcessingOptions options,
ImageFormat selectedOutputFormat)
{
if (string.IsNullOrWhiteSpace(inputPath))
{
throw new ArgumentNullException("inputPath");
}
if (string.IsNullOrWhiteSpace(inputPath))
{
throw new ArgumentNullException("outputPath");
}
....
}Тут, видимо, разработчик скопипастил первые четыре строки, но забыл поменять проверяемую переменную из inputPath в outputPath. Далее в коде есть несколько строк, в которых используется outputPath без проверки на null, что может привести к генерации исключения.
Предупреждения PVS-Studio:
private bool processWebSocketFrame(WebSocketFrame frame)
{
return frame.IsCompressed && _compression == CompressionMethod.None
? processUnsupportedFrame(....)
: frame.IsFragmented
? processFragmentedFrame(frame)
: frame.IsData
? processDataFrame(frame)
: frame.IsPing
? processPingFrame(frame)
: frame.IsPong
? processPongFrame(frame)
: frame.IsClose
? processCloseFrame(frame)
: processUnsupportedFrame(....);
}
private bool processUnsupportedFrame(....)
{
processException(....);
return false;
}
private bool processDataFrame(WebSocketFrame frame)
{
....
return true;
}
private bool processPingFrame(WebSocketFrame frame)
{
var mask = Mask.Unmask;
return true;
}
private bool processPongFrame(WebSocketFrame frame)
{
_receivePong.Set();
return true;
}
private bool processCloseFrame(WebSocketFrame frame)
{
var payload = frame.PayloadData;
close(payload, !payload.ContainsReservedCloseStatusCode, false);
return false;
}На моем счету не так уж и много, по сравнению с остальной командой PVS-Studio, проверенных анализатором проектов, и, возможно, это как-то связано с тем, что я впервые вижу, чтобы на 13 строк кода было выдано аж 7 предупреждений анализатора (чуть больше, чем на каждую вторую строчку). Поэтому считаю, что этот код заслуживает попасть в статью. Далее я распишу, на что конкретно ругается анализатор.
Надеюсь, я вас не утомила. Дальше будет попроще.
Предупреждение PVS-Studio: V3085 The name of 'RtpHeaderBytes' field in a nested type is ambiguous. The outer type contains static field with identical name. HdHomerunUdpStream.cs 200
public class HdHomerunUdpStream : LiveStream, IDirectStreamProvider
{
....
private static int RtpHeaderBytes = 12;
public class UdpClientStream : Stream
{
private static int RtpHeaderBytes = 12;
private static int PacketSize = 1316;
private readonly MediaBrowser.Model.Net.ISocket _udpClient;
bool disposed;
....
}
....
}Во вложенном классе UdpClientStream используется поле с названием, аналогичным тому, что используется во внешнем классе HdHomerunUdpStream. Это не является ошибкой, но это хороший повод проверить, все ли с этим кодом в порядке. Проблема его в том, что есть шанс перепутать эти две переменные, и код будет работать не так, как было задумано разработчиком, хотя и не вызовет нареканий со стороны компилятора.
Предупреждения PVS-Studio:
public class Lmhosts
{
public static NbtAddress GetByName(string host)
{
lock (typeof(Lmhosts))
{
return GetByName(new Name(host, 0x20, null));
}
}
internal static NbtAddress GetByName(Name name)
{
lock (typeof(Lmhosts))
{
....
}
}
}Анализатор предупреждает о том, что в этом фрагменте кода используется небезопасная блокировка. Так использовать lock нежелательно по причине того, что к объекту, используемому для блокировки, имеется общий доступ и он может быть использован для блокировки в другом месте без ведома разработчика, использовавшего этот объект в первый раз. Итогом станет возникновение взаимоблокировки на один и тот же объект.
В идеале для блокировки стоит использовать приватное поле, ну, например, вот так:
private Object locker = new Object();
public static NbtAddress GetByName(string host)
{
lock (locker)
{
return GetByName(new Name(host, 0x20, null));
}
}Предупреждение PVS-Studio: V3142 Unreacheble code detected. It is possible that an error is present. HdHomerunHost.cs 621
protected override async Task<ILiveStream> GetChannelStream(....)
{
....
var enableHttpStream = true;
if (enableHttpStream)
{
mediaSource.Protocol = MediaProtocol.Http;
var httpUrl = channelInfo.Path;
// If raw was used, the tuner doesn't support params
if (!string.IsNullOrWhiteSpace(profile) &&
!string.Equals(profile, "native",
StringComparison.OrdinalIgnoreCase))
{
httpUrl += "?transcode=" + profile;
}
mediaSource.Path = httpUrl;
return new SharedHttpStream(....);
}
return new HdHomerunUdpStream(....);
}Анализатор указывает на то, что последняя строка в этом фрагменте кода не выполнится никогда. И зачем разработчик объявляет и присваивает переменной enableHttpStream значение true и затем сразу же проверяет эту переменную в операторе if?
Возможно, этот код просто избыточен. В любом случае, его стоит перепроверить.
Предупреждение PVS-Studio: V3083 Unsafe invocation of event 'RefreshStarted', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ProviderManager.cs 943
public void OnRefreshStart(BaseItem item)
{
....
if (RefreshStarted != null)
{
RefreshStarted(this, new GenericEventArgs<BaseItem>(item));
}
}В этом методе используется потенциально небезопасный вызов обработчика события RefreshStarted, на что и указывает анализатор.
Сейчас объясню, почему этот вызов RefreshStarted в коде анализатор посчитал небезопасным. Допустим, в момент между проверкой на null и непосредственными вызовом обработчика события в теле if произведена отписка от события в другом потоке. И, если подписчиков больше нет, RefreshStarted примет значение null. Но в другом потоке, где проверка на null уже произошла, будет выполнена следующая строка:
RefreshStarted(this, new GenericEventArgs<BaseItem>(item));Это приведёт к возникновению исключения NullReferenceException.
Предупреждение PVS-Studio: V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 142, 152. LocalImageProvider.cs 142
private void PopulateImages(....)
{
....
// Logo
if (!isEpisode && !isSong && !isPerson)
{
added = AddImage(....);
if (!added)
{
added = AddImage(....);
}
}
// Art
if (!isEpisode && !isSong && !isPerson)
{
AddImage(....);
}
....
}В этом фрагменте кода рядом друг с другом находятся два оператора if с одинаковыми условиями. При этом тела у операторов if различаются. Затрудняюсь сказать, является это ошибкой или избыточным кодом. Возможно, всё хорошо и автор просто хотел таким образом логически разделить два различных действия, одно из которых связано с неким "Logo", а второе с неким "Art".
Предупреждение PVS-Studio: V3041 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;. LiveTvManager.cs 1085
private async Task RefreshChannelsInternal(....)
{
....
double progressPerService = _services.Length == 0
? 0
: 1 / _services.Length;
....
}В этом коде присутствует операция деления целочисленных типов данных. Полученное значение неявно преобразуется к типу с плавающей точкой, что крайне подозрительно.
Собственно, переменная progressPerService окажется равна 1.0 только в случае, если _services.Length = 1. При любых других значениях _services.Length в результате будет получаться 0.0.
Видимо, здесь должно быть написано так:
double progressPerService = _services.Length == 0
? 0
: (double)1 / _services.Length;Предупреждение PVS-Studio: V3050 Possibly an incorrect HTML. The </u> closing tag was encountered, while the </i> tag was expected. SrtParserTests.cs 64
public void TestParse()
{
var expectedSubs =
new SubtitleTrackInfo
{
TrackEvents = new SubtitleTrackEvent[]
{
....
new SubtitleTrackEvent
{
Id = "6",
StartPositionTicks = 330000000,
EndPositionTicks = 339990000,
Text =
"This contains nested <b>bold,
<i>italic, <u>underline</u> and
<s>strike-through</s></u></i></b> HTML tags"
},
....
}
};
}Обратите внимание на строчку "<u>underline</u>". В ней уже есть закрывающий тег </u>. Далее мы видим вот такой текст: </s></u></i></b> HTML tags"
Тут закрывающий тег </u> лишний, на что и указывает анализатор.
Предупреждение PVS-Studio: V3051 An excessive type check. The object is already of the 'Exception' type. SmbFileInputStream.cs 107
protected internal virtual IOException SeToIoe(SmbException se)
{
IOException ioe = se;
Exception root = se.GetRootCause();
if (root is TransportException)
{
ioe = (TransportException)root;
root = ((TransportException)ioe).GetRootCause();
}
if (root is Exception)
{
ioe = new IOException(root.Message);
ioe.InitCause(root);
}
return ioe;
}А вот тут, честно говоря, не совсем понятно, чего вообще хотел добиться разработчик. Анализатор предупреждает, что во втором операторе if выполняется проверка объекта root на совместимость со своим же типом. Скорее всего, это просто избыточный код, однако выглядит он подозрительно, разработчику стоит присмотреться к нему.
Разработчики во всех смыслах проделали большую работу, создавая Emby (проект имеет 215 539 строк кода, 4,6% - комментарии). Они молодцы, правда. Однако анализатор тоже постарался, выдав предупреждений уровня High – 113, Medium – 213 и Low – 112. Некоторая часть была ложными срабатываниями, но большинство ошибок не попало в статью из-за того, что они были похожи друг на друга. Например, одних только предупреждений V3022 (выражение всегда ложно/истинно) набралось 106 штук. Можно, конечно, проверить, какая часть из них была ложными срабатываниями, а оставшееся добавить в статью, но из этого получится очень нудный текст, который читать никто не станет.
Надеюсь, у меня получилось продемонстрировать пользу статического анализатора при разработке. Разумеется, единоразовых проверок недостаточно, и статический анализатор необходимо использовать на постоянной основе. Более подробно об этом можно прочитать в статье "Godot: к вопросу о регулярном использовании статических анализаторов кода".