# Анализ исходного кода Emby при помощи анализатора PVS\-Studio

Emby — довольно известный медиасервер, наравне с Plex и Kodi\. В этой статье мы попробуем проверить его исходный код при помощи статического анализатора PVS\-Studio\. На официальном сайте Emby есть маленькая пометочка "Built with ReSharper"\. Что же, так даже интереснее\.

![0705_Emby_ru/image1.png](https://import.viva64.com/docx/blog/0705_Emby_ru/image1.png)

## Анализатор кода PVS\-Studio

PVS\-Studio работает в 64\-битных системах на Windows, Linux и macOS\. Он умеет искать ошибки в исходном коде программ, написанных на языках С, C\+\+, C\# и Java\.

## Проверяемый проект

Emby — медиасервер, исходный код которого можно найти на [GitHub](https://github.com/MediaBrowser/Emby)\. Он позволяет организовывать трансляции и дает возможность получить доступ к медиаконтенту \(видео, аудио, фото\) с любого устройства\. Выжимка из официального сайта [Emby](https://emby.media/): 

* Emby автоматически конвертирует ваши медиафайлы на лету для воспроизведения на любом устройстве;
* Обширные функции родительского контроля позволяют легко контролировать доступ к контенту, что будет актуально для семей с маленькими детьми;
* Emby организует ваши медиафайлы в простые и элегантные презентации\. Они никогда не будут выглядеть одинаково;
* Потоковая передача данных с синхронизацией в облаке;
* Легко поделиться своими медиафайлами с друзьями и семьей;
* И многое другое\.

## Участки кода, привлекшие внимание при рассмотрении отчета анализатора

**Предупреждение PVS\-Studio:** [V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'c \!\= '<'' to the left and to the right of the '&&' operator\. HttpListenerRequest\.Managed\.cs 49

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'SmbConstants\.AttrHidden' to the left and to the right of the '\|' operator\. SmbComDelete\.cs 29

```cpp
internal SmbComDelete(string fileName)
{
  Path = fileName;
  Command = SmbComDelete;
  _searchAttributes = SmbConstants.AttrHidden |
                      SmbConstants.AttrHidden |
                      SmbConstants.AttrSystem;
}
```

Анализатор опять нашел опечатку, связанную с повторяющимися подвыражениями\. Забегая вперед, скажу, что в этом проекте что\-то многовато подобных ошибок, допущенных по невнимательности\. Ничуть не упрекаю в этом создателей Emby, все мы иногда бываем рассеянными подобным образом \([примеры](https://pvs-studio.ru/ru/blog/examples/v501/), [примеры](https://pvs-studio.ru/ru/blog/examples/v3001/), [примеры](https://pvs-studio.ru/ru/blog/examples/v6001/)\), и, чтобы перестраховаться от подобных ошибок, как раз и существует статический анализ\.

**Предупреждение PVS\-Studio:** [V3004](https://pvs-studio.ru/ru/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. SqliteItemRepository\.cs 5648

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3005/) The '\_validProviderIds' variable is assigned to itself\. BaseNfoParser\.cs 77

```cpp
private Dictionary<string, string> _validProviderIds;
....
public void Fetch(....)
{
  ....
  _validProviderIds = _validProviderIds = new Dictionary<....>(....);
  ....
}
```

Опять же опечатка, которая повлекла за собой присвоение значения переменной самой себе\. Стоит перепроверить этот участок кода\.

**Предупреждение PVS\-Studio:** [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'Chapters' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 29, 28\. Title\.cs 29

```cpp
public Title(uint titleNum)
{
    ProgramChains = new List<ProgramChain>();
    Chapters = new List<Chapter>();
    Chapters = new List<Chapter>();
    TitleNumber = titleNum;
}
```

Невнимательность и опечатки, опять\.\.\. Переменной _Chapters_ дважды присваивается значение\. Понятно, что ничем плохим этот лишний код не грозит, но тем не менее\. Задерживаться на этом примере смысла нет, давайте пойдём дальше\.

**Предупреждение PVS\-Studio:** [V3013](https://pvs-studio.ru/ru/docs/warnings/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 

```cpp
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_](https://docs.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.enterwritelock?view=netframework-4.8) нужен для того, чтобы выполнить вход в блокировку в режиме записи\. Для режима чтения стоит воспользоваться методом _EnterReadLock_, допускающим возможность одновременного чтения ресурса несколькими потоками\.

Разработчику стоит повнимательнее присмотреться к этому участку кода, возможно, тут допущена ошибка\. Тем более, в коде встретился следующий класс, который не используется:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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:** 

* V3022 Expression 'processUnsupportedFrame\(frame, CloseStatusCode\.PolicyViolation, null\)' is always false\. WebSocket\.cs 462
* V3022 Expression 'processCloseFrame\(frame\)' is always false\. WebSocket\.cs 461
* V3022 Expression 'frame\.IsClose ? processCloseFrame\(frame\) : processUnsupportedFrame\(frame, CloseStatusCode\.PolicyViolation, null\)' is always false\. WebSocket\.cs 460
* V3022 Expression 'processPongFrame\(frame\)' is always true\. WebSocket\.cs 459
* V3022 Expression 'processPingFrame\(frame\)' is always true\. WebSocket\.cs 457
* V3022 Expression 'processDataFrame\(frame\)' is always true\. WebSocket\.cs 455
* V3022 Expression is always false\. WebSocket\.cs 448

```cpp
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 предупреждений анализатора \(чуть больше, чем на каждую вторую строчку\)\. Поэтому считаю, что этот код заслуживает попасть в статью\. Далее я распишу, на что конкретно ругается анализатор\.

1. Вначале считается выражение _frame\.IsCompressed_ _&&_ _\_compression_ _\=\=_ _CompressionMethod\.None_ и, если выражение истинно, будет выполнен метод _processUnsupportedFrame_, который в любом случае возвращает _false_ \(отсюда и первое предупреждение анализатора\)\. Если же оно было ложно, то перейдем к следующему пункту\.
1. Далее проверяется значение _frame\.IsFragmented_\. Тут нет никаких проблем, поэтому перейдем дальше\.
1. Тут проверяется значение _frame\.IsData_\. Если истинно, то метод _processDataFrame_ в любом случае возвратит _true_\. Отсюда второе предупреждение анализатора\.
1. Потом проверяем _frame\.IsPing_\. Истинно – _processPingFrame_ возвратит _true_\. Это третье предупреждение анализатора\.
1. Смотрим на _frame\.IsPong_\. Тут все аналогично предыдущему\.
1. И последнее: _frame\.IsClose_\. _processCloseFrame_ и _processUnsupportedFrame _в любом случае возвращают _false_\.

 

![0705_Emby_ru/image2.png](https://import.viva64.com/docx/blog/0705_Emby_ru/image2.png)

Надеюсь, я вас не утомила\. Дальше будет попроще\.

**Предупреждение PVS\-Studio:** [V3085](https://pvs-studio.ru/ru/docs/warnings/v3085/) The name of 'RtpHeaderBytes' field in a nested type is ambiguous\. The outer type contains static field with identical name\. HdHomerunUdpStream\.cs 200

```cpp
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:** 

* V3090 Unsafe locking on a type\. All instances of a type will have the same 'Type' object\. Lmhosts\.cs 49
* V3090 Unsafe locking on a type\. All instances of a type will have the same 'Type' object\. Lmhosts\.cs 57

```cpp
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_ нежелательно по причине того, что к объекту, используемому для блокировки, имеется общий доступ и он может быть использован для блокировки в другом месте без ведома разработчика, использовавшего этот объект в первый раз\. Итогом станет возникновение взаимоблокировки на один и тот же объект\.

В идеале для блокировки стоит использовать приватное поле, ну, например, вот так: 

```cpp
private Object locker = new Object();

public static NbtAddress GetByName(string host)
{
  lock (locker)
  {
    return GetByName(new Name(host, 0x20, null));
  }
}
```

**Предупреждение PVS\-Studio:** [V3142](https://pvs-studio.ru/ru/docs/warnings/v3142/) Unreacheble code detected\. It is possible that an error is present\. HdHomerunHost\.cs 621

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3083/) Unsafe invocation of event 'RefreshStarted', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. ProviderManager\.cs 943

```cpp
public void OnRefreshStart(BaseItem item)
{
  ....

  if (RefreshStarted != null)
  {
    RefreshStarted(this, new GenericEventArgs<BaseItem>(item));
  }
}
```

В этом методе используется потенциально небезопасный вызов обработчика события _RefreshStarted_, на что и указывает анализатор\. 

Сейчас объясню, почему этот вызов _RefreshStarted_ в коде анализатор посчитал небезопасным\. Допустим, в момент между проверкой на _null_ и непосредственными вызовом обработчика события в теле _if_ произведена отписка от события в другом потоке\. И, если подписчиков больше нет, _RefreshStarted_ примет значение _null_\. Но в другом потоке, где проверка на _null_ уже произошла, будет выполнена следующая строка: 

```cpp
RefreshStarted(this, new GenericEventArgs<BaseItem>(item));
```

Это приведёт к возникновению исключения _NullReferenceException_\.

**Предупреждение PVS\-Studio:** [V3029](https://pvs-studio.ru/ru/docs/warnings/v3029/) The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 142, 152\. LocalImageProvider\.cs 142

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
private async Task RefreshChannelsInternal(....)
{
  ....
  double progressPerService = _services.Length == 0
                ? 0
                : 1 / _services.Length;
  ....
}
```

В этом коде присутствует операция деления целочисленных типов данных\. Полученное значение неявно преобразуется к типу с плавающей точкой, что крайне подозрительно\.

Собственно, переменная _progressPerService_ окажется равна 1\.0 только в случае, если _\_services\.Length \= 1_\. При любых других значениях _\_services\.Length_ в результате будет получаться _0\.0_\.

Видимо, здесь должно быть написано так:

```cpp
double progressPerService = _services.Length == 0
                ? 0
                : (double)1 / _services.Length;
```



**Предупреждение PVS\-Studio:** [V3050](https://pvs-studio.ru/ru/docs/warnings/v3050/) Possibly an incorrect HTML\. The </u\> closing tag was encountered, while the </i\> tag was expected\. SrtParserTests\.cs 64

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3051/) An excessive type check\. The object is already of the 'Exception' type\. SmbFileInputStream\.cs 107

```cpp
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% \- комментарии\)\. Они молодцы, правда\. Однако анализатор тоже постарался, выдав предупреждений [уровня](https://pvs-studio.ru/ru/docs/manual/0007/) High – 113, Medium – 213 и Low – 112\. Некоторая часть была ложными срабатываниями, но большинство ошибок не попало в статью из\-за того, что они были похожи друг на друга\. Например, одних только предупреждений [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) \(выражение всегда ложно/истинно\) набралось 106 штук\. Можно, конечно, проверить, какая часть из них была ложными срабатываниями, а оставшееся добавить в статью, но из этого получится очень нудный текст, который читать никто не станет\.

Надеюсь, у меня получилось продемонстрировать пользу статического анализатора при разработке\. Разумеется, единоразовых проверок недостаточно, и статический анализатор необходимо использовать на постоянной основе\. Более подробно об этом можно прочитать в статье "[Godot: к вопросу о регулярном использовании статических анализаторов кода](https://pvs-studio.ru/ru/blog/posts/cpp/0594/)"\.