Вебинар: Использование статических анализаторов кода при разработке безопасного ПО - 19.12
В последнее время в качестве хобби и заодно с целью популяризации нашего статического анализатора PVS-Studio мы выполняем проверки open-source проектов и по возможности выпускаем патчи с исправлениями. Сегодня я хотел бы поговорить об одной интересной ошибке, которую я нашёл в проекте Entity Framework.
Патч, исправляющий данную ошибку, я уже отправил разработчикам, и с ним вы можете ознакомиться по ссылке.
А теперь ближе к делу. Анализатор выдал 2 предупреждения на одной строке:
Это, кстати, не редкая ситуация, когда анализатор выдаёт 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: поиск дефектов безопасности".
0