Глеб Асламов

21 Июл 2023

Теги:

#CSharp #GameDev

Ryujinx: повторная проверка эмулятора Nintendo Switch с помощью PVS-Studio

21 Июл 2023

Автор: Глеб Асламов

О проекте
Разбор ошибок
Заключение

Популярность Nintendo Switch не угасает, эксклюзивные игры получают награды, и желание в них поиграть только растет. Однако возможность опробовать портативную приставку есть не у каждого. Решает эту проблему эмулятор консоли Nintendo Switch — Ryujinx. Сегодня проверим его код с помощью анализатора PVS-Studio.

О проекте

Ryujinx — это эмулятор Nintendo Switch, нацеленный, по заявлению разработчиков, на точность, производительность и удобный интерфейс. Проект написан на C#, активно разрабатывается и доступен в репозитории на GitHub.

Среди эмуляторов Nintendo Switch есть и конкуренты для Ryujinx — это проект Yuzu, написанный на языке C++. Он рассматривался в одной из наших статей по проверке проектов.

В блоге PVS-Studio про эмулятор Ryujinx уже была статья о проверке, но с тех пор прошло много времени: появилось множество новых функций, а с ними и новые ошибки. О них мы сегодня и поговорим.

Разбор ошибок

Невнимательность, опечатки, copy-paste

Подобные ошибки легко пропустить. А ещё они могут как не влиять на работоспособность программы, так и менять её логику — как повезёт.

Issue 1

Давайте и начнём с проверки на внимательность.

Далее приведу большой фрагмент кода. Сможете найти в нём подвох?

public static PrimitiveType Convert(this PrimitiveTopology topology)
{
  switch (topology)
  {
    case PrimitiveTopology.Points:
      return PrimitiveType.Points;
    case PrimitiveTopology.Lines:
       return PrimitiveType.Lines;
    case PrimitiveTopology.LineLoop:
       return PrimitiveType.LineLoop;
    case PrimitiveTopology.LineStrip:
       return PrimitiveType.LineStrip;
    case PrimitiveTopology.Triangles:
       return PrimitiveType.Triangles;
    case PrimitiveTopology.TriangleStrip:
       return PrimitiveType.TriangleStrip;
    case PrimitiveTopology.TriangleFan:
       return PrimitiveType.TriangleFan;
    case PrimitiveTopology.Quads:
       return PrimitiveType.Quads;
    case PrimitiveTopology.QuadStrip:
       return PrimitiveType.QuadStrip;
    case PrimitiveTopology.Polygon:
       return PrimitiveType.TriangleFan;
    case PrimitiveTopology.LinesAdjacency:
       return PrimitiveType.LinesAdjacency;
    case PrimitiveTopology.LineStripAdjacency:
       return PrimitiveType.LineStripAdjacency;
    case PrimitiveTopology.TrianglesAdjacency:
       return PrimitiveType.TrianglesAdjacency;
    case PrimitiveTopology.TriangleStripAdjacency:
       return PrimitiveType.TriangleStripAdjacency;
    case PrimitiveTopology.Patches:
       return PrimitiveType.Patches;
  }
  ....
}

Предупреждение PVS-Studio open icon

V3139 Two or more case-branches perform the same actions. EnumConversion.cs 448

Ошибку не так сложно заметить, если пристально изучать этот фрагмент кода, но захотите ли вы это делать во время работы и не зная, что там ошибка?

Если вдруг у вас заболели глаза, дам подсказку:

switch (topology)
{
  ....
  case PrimitiveTopology.TriangleFan:
    return PrimitiveType.TriangleFan;
  ....
  case PrimitiveTopology.Polygon:
    return PrimitiveType.TriangleFan;
  ....
}

Это классическая ошибка copy-paste. Во второй case-секции должно возвращаться значение PrimitiveType.Polygon. Убедимся в том, что оно есть в перечислении PrimitiveType:

public enum PrimitiveType
{
  ....
  TriangleFan = 6,
  ....
  Polygon = 9,
  ....
}

Copy-paste — это удобно и быстро, но когда фрагмент становится всё больше и больше, также растёт и возможность ошибиться.

Issue 2

Рассмотрим подозрительный фрагмент кода:

private void YesButton_Clicked(object sender, EventArgs args)
{
  ....
  Window.Functions = _mainWindow.Window.Functions =
    WMFunction.All & WMFunction.Close;
  ....
}

Предупреждение PVS-Studio: V3182 The result of 'WMFunction.All & WMFunction.Close' expression is '0'. It is possible that the '|' operator should be used instead. UpdateDialog.cs 69

Как мы видим из предупреждения, анализатор ругается на оператор '&'. Чтобы понять почему, давайте взглянем на перечисление WMFunction:

[Flags]
public enum WMFunction
{
  All = 0x1,
  Resize = 0x2,
  Move = 0x4,
  Minimize = 0x8,
  Maximize = 0x10,
  Close = 0x20
}

Мы имеем дело с битовыми флагами, но в данном случае реализация объединения не будет работать: результатом операции побитового И (&) для значений WMFunction.All и WMFunction.Close будет ноль.

Анализатор сразу даёт нам подсказку о решении данной проблемы. Корректная реализация объединения флагов выглядит следующим образом:

Window.Functions = _mainWindow.Window.Functions =
  WMFunction.All | WMFunction.Close;

Issue 3

Ещё один интересный случай:

private BaseNode ParseSpecialName(....)
{
  switch (....)
  {
    case 'C':
      _position += 2;
      BaseNode firstType = ParseType();
      if (   firstType == null 
          || ParseNumber(true).Length == 0 
          || !ConsumeIf("_"))
      {
        return null;
      }
      BaseNode secondType = ParseType();
      return new CtorVtableSpecialName(secondType, firstType); // <=
  }
}

Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'CtorVtableSpecialName' constructor: 'secondType' and 'firstType'. Demangler.cs 803

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

public CtorVtableSpecialName(BaseNode firstType, BaseNode secondType) :
  base(NodeType.CtorVtableSpecialName)
{
  _firstType  = firstType;
  _secondType = secondType;
}

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

Но как понять со стороны, задумка это или ошибка? Комментарии могли бы помочь, однако их нет.

Примечание. Помимо новых срабатываний остались неисправленные ошибки с прошлой проверки:

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 Demangler.cs 2043
V3013 It is odd that the body of 'PrintLeft' function is fully equivalent to the body of 'PrintRight' function (10, line 18). PackedTemplateParameter.cs 10

Разбор этих ошибок вы можете прочитать в этой статье.

NullReferenceException и все-все-все

Распространённый паттерн — подозрительное взаимодействие с потенциально нулевыми ссылками.

Issue 4

Рассмотрим случай опасного порядка действий:

public Result Initialize(....)
{
  ....
  if (type == ThreadType.User)
  {
    if (owner.AllocateThreadLocalStorage(out _tlsAddress) 
        != Result.Success)
    {
      return KernelResult.OutOfMemory;
    }

    MemoryHelper.FillWithZeros(owner.CpuMemory, 
                               _tlsAddress,
                               KTlsPageInfo.TlsEntrySize);
  }

  if (owner != null)
  {
    Owner = owner;
    owner.IncrementReferenceCount();
    owner.IncrementThreadCount();
    ....
  }
  else
  {
    is64Bits = true;
  }
}

Предупреждение PVS-Studio: V3095 The 'owner' object was used before it was verified against null. Check lines: 169, 174. KThread.cs 169

Нас интересует переменная owner. Она проверяется на null, но перед этим используется в методе без проверок. Этот код выглядит подозрительно.

А вот есть ли здесь ошибка, или между переменными owner и type есть некая связь, и исключения не будет — вопрос. Со стороны сказать сложно, здесь разработчикам должно быть виднее. Однако код в любом случае подозрительный.

Кстати, анализатор сработал не совсем корректно, так как указал на обращение owner.CpuMemory, а не owner.AllocateThreadLocalStorage. Здесь нам есть над чем поработать. :)

Issue 5

А теперь наоборот. Давайте взглянем на следующий код:

private void RenderLoop()
{
  ....
  _renderer?.Window?.SetAntiAliasing(....);
  _renderer?.Window?.SetScalingFilter(....);
  _renderer?.Window?.SetScalingFilterLevel(....);
  ....
  _renderer.Window.SetSize(....);
  ....
}

Предупреждение PVS-Studio: V3125 The '_renderer.Window' object was used after it was verified against null. Check lines: 882, 877. AppHost.cs 882

В этот раз разработчик проверяет _renderer.Window с помощью оператора условного доступа — '?.', но после этого данная проверка опускается.

Сложно сказать, хотел ли этого разработчик или просто забыл добавить проверку.

Примечание. Помимо новых, вновь нашлись неисправленные с прошлой проверки предупреждения:

V3095 The 'IManager.NsdSettings' object was used before it was verified against null. Check lines: 37, 41. FqdnResolver.cs 37
V3125 The 'Owner' object was used after it was verified against null. Check lines: 1308, 1306. KThread.cs 1308
V3105 The 'result' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Client.cs 214

Issue 6

Посмотрим на следующий метод, который анализатор посчитал подозрительным:

public (uint, bool) GetFormat(....)
{
  TextureSpecializationState state = 
    GetTextureSpecState(stageIndex, handle, cbufSlot).Value;
  ....
}

С первого взгляда проблемы не видно, но дьявол, как говорится, кроется в деталях. Обратите внимание на предупреждение:

V3080 Possible null dereference of method return value. Consider inspecting: GetTextureSpecState(...). ShaderSpecializationState.cs 408

Пойдём по пути пользователя анализатора и посмотрим на метод изнутри:

private Box<....> GetTextureSpecState(....)
{
  TextureKey key = new TextureKey(stageIndex, handle, cbufSlot);
  if (_textureSpecialization.TryGetValue(key,
        out Box<TextureSpecializationState> state))
  {
    return state;
  }
  return null;
}

Так как существует сценарий возвращения null, хорошей идеей будет добавить проверку.

Стоит заметить, что подобный код встречается несколько раз:

V3080 Possible null dereference of method return value. Consider inspecting: GetTextureSpecState(...). ShaderSpecializationState.cs 408
V3080 Possible null dereference of method return value. Consider inspecting: GetTextureSpecState(...). ShaderSpecializationState.cs 420
V3080 Possible null dereference of method return value. Consider inspecting: GetTextureSpecState(...). ShaderSpecializationState.cs 431Ф

Если хотите больше узнать о NullReferenceException, советую ознакомиться с этой статьёй. Например, можете проверить, все ли перечисленные способы столкнуться с NRE вы помните.

null из прошлого

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

Issue 7

В предыдущей статье о проверке данного проекта было описано следующее предупреждение:

public void LoadApplication(string path, bool isFirmwareTitle)
{
  ....
  firmwareVersion = _contentManager.GetCurrentFirmwareVersion();
  string message = $"{firmwareVersion.VersionString}.....";
  GtkDialog.CreateInfoDialog($"{firmwareVersion.VersionString} .... ");
  ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'firmwareVersion'. MainWindow.cs 734

Проблема заключалась в том, что переменная firmwareVersion может иметь значение null, но проверка перед использованием отсутствует. Рассмотрим метод GetCurrentFirmwareVersion:

public SystemVersion GetCurrentFirmwareVersion()
{
  LoadEntries();
  lock (_lock)
  {
    ....
    if (....)
    {
      return new SystemVersion(systemVersionFile.AsStream());
    }
    ....
  }
  return null; // <=
}

Есть вероятность возвращения null, что, в свою очередь, может привести к ошибке.

Помимо рассмотренного предупреждения, возникло новое:

public async Task<bool> LoadGuestApplication(){
  ....
  firmwareVersion = ContentManager.GetCurrentFirmwareVersion();


  await ContentDialogHelper.CreateInfoDialog(
    LocaleManager.Instance.UpdateAndGetDynamicValue(
      ....,  
      firmwareVersion.VersionString),                  // <=
    LocaleManager.Instance.UpdateAndGetDynamicValue(
      ...., 
      firmwareVersion.VersionString),                  // <=
    ....);               
  ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'firmwareVersion'. AppHost.cs 541

Стоит добавить, что ниже по коду находится уже безопасное использование переменной firmwareVersion:

public async Task<bool> LoadGuestApplication()
{
  ....
  firmwareVersion = ContentManager.GetCurrentFirmwareVersion();
  ....
  Logger.Notice.Print(
    LogClass.Application, 
    $"Using Firmware Version: {firmwareVersion?.VersionString}");
}

Заключение

Надеемся, эта статья поможет разработчикам улучшить их проект.

Отметим, что с прошлой проверки прошло около двух лет, но некоторые места так и не были исправлены. Советуем разработчикам Ryujinx обратить на них внимание и ознакомиться с предыдущей статьей.

Если вам интересны статьи про проверку игровых проектов, предлагаю ознакомиться с нашим блогом.

А если вас заинтересовала возможность проверить свой код с помощью PVS-Studio, скачать и попробовать его можно здесь.

#CSharp #GameDev