Интересная ошибка в Entity Framework

23 Мар 2017

Автор: Иван Кищенко

В последнее время в качестве хобби и заодно с целью популяризации нашего статического анализатора PVS-Studio мы выполняем проверки open-source проектов и по возможности выпускаем патчи с исправлениями. Сегодня я хотел бы поговорить об одной интересной ошибке, которую я нашёл в проекте Entity Framework.

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

А теперь ближе к делу. Анализатор выдал 2 предупреждения на одной строке:

V3014 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. EFCore ExpressionEqualityComparer.cs 214
V3015 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' EFCore ExpressionEqualityComparer.cs 214

Это, кстати, не редкая ситуация, когда анализатор выдаёт 2, а иногда даже 3 предупреждения на одну ошибку. Все дело в том, что некорректный код может быть аномален сразу с нескольких точек зрения.

Рассмотрим код:

var memberInitExpression = (MemberInitExpression)obj;
....
for (var i = 0; i < memberInitExpression.Bindings.Count; i++)
{
  var memberBinding = memberInitExpression.Bindings[i];
  .... 
  switch (memberBinding.BindingType)
  {
    case ....
    case MemberBindingType.ListBinding:
      var memberListBinding = (MemberListBinding)memberBinding;
      for(var j=0; i < memberListBinding.Initializers.Count; i++)
      {
        hashCode += (hashCode * 397) ^
          GetHashCode(memberListBinding.Initializers[j].Arguments);
      }
      break;
    ....
   }
}

Что же здесь происходит? Как мы видим, у нас есть 2 цикла. В первом используется счетчик с именем i для обхода списка memberInitExpression.Bindings, во втором - с именем j для обхода списка memberListBinding.Initializers. Но по какой-то причине во втором цикле используется счетчик из первого цикла. Мне это место сразу показалось очень подозрительным, поэтому было решено написать небольшой юнит-тест, чтобы проверить, действительно ли это ошибка, или хитрый алгоритм программы.

Код юнит теста:

[ConditionalFact]
public void Compare_member_init_expressions_by_hash_code()
{
    MethodInfo addMethod = typeof(List<string>).GetMethod("Add");

    MemberListBinding bindingMessages = Expression.ListBind(
        typeof(Node).GetProperty("Messages"),
        Expression.ElementInit(addMethod, Expression.Constant(
          "Greeting from PVS-Studio developers!"))
    );

    MemberListBinding bindingDescriptions = Expression.ListBind(
        typeof(Node).GetProperty("Descriptions"),
        Expression.ElementInit(addMethod, Expression.Constant(
          "PVS-Studio is a static code analyzer for C, C++ and C#."))
    );

    Expression query1 = Expression.MemberInit(
        Expression.New(typeof(Node)),
        new List<MemberBinding>() {
            bindingMessages                    // One member
        }
    );

    Expression query2 = Expression.MemberInit(
        Expression.New(typeof(Node)),
        new List<MemberBinding>() {
            bindingMessages,                  // Two members
            bindingDescriptions
        }
    );

    var comparer = new ExpressionEqualityComparer();
    var key1Hash = comparer.GetHashCode(query1);
    var key2Hash = comparer.GetHashCode(query2);

    // The hash codes for both expressions 
    // were the same before my edit
    Assert.NotEqual(key1Hash, key2Hash);      // <=
}

Мои ожидания подтвердились. Это настоящая ошибка. Дело в том, что при сравнении 2 выражений (Expression's) всегда сравнивались только первые элементы 2 коллекций, что приводило к некорректному результату для разных выражений с одинаковыми первыми элементами. Учитывая тот факт, что Entity Framework очень тесно работает с выражениями, и практически основная его цель - это преобразование лямбд и Linq запросов в SQL запросы, думаю, несложно догадаться, к чему могла приводить столь серьезная ошибка в коде.

Согласно Common Weakness Enumeration найденную ошибку можно классифицировать как CWE-670 (Always-Incorrect Control Flow Implementation). Неизвестно, можно ли эксплуатировать данную слабость кода как уязвимость, но баг достаточно серьезный. Это хорошая демонстрация, что анализатор PVS-Studio можно использовать для поиска потенциальных уязвимостей. На самом деле он всегда это мог, просто мы не акцентировали на этом внимание. Подробнее эта тема раскрыта в статье "PVS-Studio: поиск дефектов безопасности".

#CSharp