Артём Ровенский

24 Май 2023

Теги:

#CSharp

RavenDB и PVS-Studio: коллаборация, от которой выигрывают все

24 Май 2023

Автор: Артём Ровенский

Начало коллаборации
Ошибки и исправления
Выводы

Небольшая история про сотрудничество PVS-Studio и RavenDB. PVS-Studio — статический анализатор для улучшения кода. RavenDB — Open Source база данных. Как поиск ошибок в одном проекте приводит к улучшению сразу двух? Ответим на этот вопрос, а также посмотрим на исправления найденных проблем и комментарии разработчиков.

1052_Collaboration_of_RavenDB_and_PVSStudio_ru/image1.png

Начало коллаборации

Перед началом истории напомню про RavenDB и PVS-Studio. RavenDB — это Open Source документноориентированная ACID база данных, использующая подход NoSQL. PVS-Studio — это инструмент статического анализа, направленный на улучшение качества, защищённости (SAST) и безопасности кода для C, C++, C# и Java.

Команда RavenDB решила использовать PVS-Studio для проверки кода проекта. Мы выступили со встречным предложением: сначала провести разовый анализ, разобрать несколько наиболее интересных ошибок и рассказать о них. В итоге так и решили сделать.

Мы выкачали исходники и принялись анализировать. Стоит сказать, что код написан довольно хорошо и читаемо. Разработчиков за это можно только похвалить. После проверки мы отправили команде RavenDB 10 наиболее подозрительных, по нашему мнению, фрагментов кода.

Ответ не заставил себя долго ждать. Head of Development Павел Пекрол прислал нам фидбэк по предупреждениям PVS-Studio и создал pull request c исправлениями описанных проблем.

Разработчики RavenDB очень быстро отреагировали на подозрительные фрагменты кода и исправили найденные ошибки. Давайте пройдёмся по самым интересным из них, разберём ошибки и посмотрим, как разработчики исправили код.

Ошибки и исправления

Ошибка 1

[Flags]
public enum TypeAttributes
{
  ....
  NotPublic = 0,
  ....
}

private static bool CheckIfAnonymousType(Type type)
{
  // hack: the only way to detect anonymous types right now
  return type.IsDefined(typeof(CompilerGeneratedAttribute), false)
      && type.IsGenericType && type.Name.Contains("AnonymousType")
      && (type.Name.StartsWith("<>") || type.Name.StartsWith("VB$"))
      && type.Attributes.HasFlag(TypeAttributes.NotPublic);        // <=
}

V3180 The 'HasFlag' method always returns 'true' because the value of 'TypeAttributes.NotPublic' is '0', and it is passed as the method's argument. ExpressionStringBuilder.cs 2117

Метод HasFlag всегда возвращает значение true, если в качестве аргумента принимает флаг со значением 0 (см. документацию). Получается, что часть выражения всегда равна true и не имеет смысла.

Код исправили, удалив проверку с HasFlag.

Коммит с исправлением.

Ошибка 2

private JsValue Spatial_Distance(JsValue self, JsValue[] args)
{
  if (args.Length < 4 && args.Length > 5) // <=
    throw new ArgumentException("....");
}

V3022 Expression 'args.Length < 4 && args.Length > 5' is always false. Probably the '||' operator should be used here. ScriptRunner.cs 930

Значение выражения args.Length не может быть одновременно меньше 4 и больше 5, следовательно, это условие всегда ложно. Исправление очевидно — поменять '&&' на '||'.

Однако разработчики RavenDB заметили ещё одну ошибку. Как оказалось, верхней границей для количества аргументов должно быть значение 6, а не 5.

Исправленный код выглядит так:

private static JsValue Spatial_Distance(JsValue self, JsValue[] args)
{
  if (args.Length is < 4 or > 6)
    throw new ArgumentException("....");
}

Коммиты с исправлениями: раз, два.

Ошибка 3

public override DynamicJsonValue ToJson()
{
  var json = base.ToJson();
  json[nameof(DataDirectory)] = DataDirectory;
  json[nameof(JournalStoragePath)] = DataDirectory;      // <=
  json[nameof(SnapshotRestore)] = SnapshotRestore.ToJson();
  return json;
}

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'JournalStoragePath' variable should be used instead of 'DataDirectory' RestoreResult.cs 24

Исходя из того, что в json[nameof(DataDirectory)] записывается значение свойства DataDirectory, в json[nameof(JournalStoragePath)] должно записываться значение свойства JournalStoragePath. Именно так разработчики и исправили ошибку:

json[nameof(JournalStoragePath)] = JournalStoragePath;

Коммит с исправлением.

Ошибка 4

public override int GetHashCode()
{
  unchecked
  {
    var hashCode = MinimumRevisionsToKeep?.GetHashCode() ?? 0;
    hashCode = (hashCode * 397) ^ 
      MinimumRevisionAgeToKeep?.GetHashCode() ?? 0; // <=
    hashCode = (hashCode * 397) ^ 
      Disabled.GetHashCode();
    hashCode = (hashCode * 397) ^
      PurgeOnDelete.GetHashCode();
    return hashCode;
  }
}

V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. RevisionsCollectionConfiguration.cs 40

Здесь PVS-Studio указывает на то, что возможна ошибка с приоритетом оператора '??'. И действительно, приоритет у оператора '^' выше, чему у '??'. Если MinimumRevisionAgeToKeep будет иметь значение null, то результат вычисления hashCode в выделенной строке всегда будет равен 0. Для исправления достаточно взять выражение с '??' в скобки.

Кстати, разработчики заметили ещё одну ошибку. При вычислении hashCode забыли про свойство MaximumRevisionsToDeleteUponDocumentUpdate.

Итоговый исправленный код:

public override int GetHashCode()
{
  unchecked
  {
    var hashCode = MinimumRevisionsToKeep?.GetHashCode() ?? 0;
    hashCode = (hashCode * 397) ^ 
      (MinimumRevisionAgeToKeep?.GetHashCode() ?? 0);
    hashCode = (hashCode * 397) ^ 
      (MaximumRevisionsToDeleteUponDocumentUpdate?.GetHashCode() ?? 0);
    hashCode = (hashCode * 397) ^ 
      Disabled.GetHashCode();
    hashCode = (hashCode * 397) ^ 
      PurgeOnDelete.GetHashCode();
    return hashCode;
  }
}

Коммит с исправлением.

Слова руководителя разработки Павла Пекрола про эту ошибку:

Point 4 - gold :)

Ошибка 5

public override void VisitMethod(MethodExpression expr)
{
  if (expr.Name.Value == "id" && expr.Arguments.Count == 0)
  {
    if (expr.Arguments.Count != 1)   // <=
    {
      throw new InvalidOperationException("....");
    }
    _sb.Append("this");
  }
}

V3022 Expression 'expr.Arguments.Count != 1' is always true. JavascriptCodeQueryVisitor.cs 188

Проверка expr.Arguments.Count != 1 выполняется сразу же после проверки expr.Arguments.Count == 0. Очевидно, что если значение expr.Arguments.Count равно 0, то оно также не равно 1. Если поток выполнения заходит в верхний оператор if, то всегда будет бросаться исключение. Как следствие, _sb.Append("this") никогда не выполнится.

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

Исправлением стало удаление оператора if с выбрасыванием исключения.

Коммит с исправлением.

Выводы

В названии статьи написано, что выигрывает каждый. В чём выиграл RavenDB? Естественно, что каждый продукт как-то развивается и совершенствуется. В этот раз улучшение произошло не через новые фичи, а через повышение качества исходного кода. В чём же выиграл PVS-Studio? Во время просмотра отчёта об ошибках мы нашли несколько ложных предупреждений. Некоторые из них мы уже исправили, а до некоторых ещё предстоит добраться.

Руководитель разработки RavenDB Павел Пекрол:

Going over the list made me realize that devs are only humans and no matter how many tests you have there will always be bugs. The tool is very helpful in finding obvious and non-obvious bugs.

Не могу не согласиться. Разработчик просто не может полностью покрыть код тестами. Если вы заботитесь о качестве кода и проекта, то стоит использовать различные инструменты, которые помогут сделать разработку проще.

Избавляйтесь от багов и делайте свой код безопаснее с помощью PVS-Studio! Удобно и просто храните и обрабатывайте данные вместе с RavenDB!

Приятного использования RavenDB и PVS-Studio!

#CSharp