Мы используем куки, чтобы пользоваться сайтом
было удобно.
Чуть больше полугода назад состоялся релиз .NET 10. Остались ли дефекты в исходном коде проекта, спустя месяцы после релиза? Постараемся сегодня найти ответ на этот вопрос. Приятного чтения!
Новая версия платформы принесла множество изменений: улучшения производительности, обновления библиотек, новые API и очередные доработки компилятора и рантайма. Но, как показывает практика, даже код проектов такого уровня не застрахован от ошибок.
Поэтому мы решили проверить исходный код .NET 10 (версии 10.0.8) с помощью статического анализатора PVS-Studio.
В процессе анализа удалось обнаружить несколько подозрительных участков кода, потенциальных ошибок и просто любопытных мест, заслуживающих внимания разработчиков. В статье мы рассмотрим наиболее интересные предупреждения анализатора.
Как было сказано ранее, с момента релиза .NET 10 прошло уже немало времени. Однако освежить в памяти нововведения не будет лишним:
Перед разбором кода .NET10, хочется сказать, что список языков, поддерживаемых PVS-Studio, расширяется. Теперь в режиме раннего доступа можно протестировать работу анализатора для Go, JavaScript и TypeScript. Здесь можно записаться на тестирование.
Приступим к разбору кода!
Фрагмент кода 1
public static void ReflectionWriteValue(....)
{
....
if (memberValue == null) // <=
{
context.WriteNull(xmlWriter,
memberType,
DataContract.IsTypeSerializable(memberType));
}
else
{
PrimitiveDataContract? primitiveContract = ....;
if ( primitiveContract != null
&& primitiveContract.UnderlyingType != Globals.TypeOfObject
&& !writeXsiType)
{
primitiveContract.WriteXmlValue(xmlWriter, memberValue, context);
}
else
{
if ( memberValue == null // <=
&& ( memberType == Globals.TypeOfObject
|| (originValueIsNullableOfT && memberType.IsValueType)))
{
context.WriteNull(xmlWriter,
memberType,
DataContract.IsTypeSerializable(memberType));
}
else
{
ReflectionInternalSerialize(xmlWriter,
context,
memberValue!,
memberValue!.GetType()
.TypeHandle
.Equals(memberType.TypeHandle),
writeXsiType,
memberType,
originValueIsNullableOfT);
}
}
}
....
}Предупреждение PVS-Studio: V3022 Expression 'memberValue == null && (memberType == Globals.TypeOfObject || (originValueIsNullableOfT && memberType.IsValueType))' is always false. ReflectionClassWriter.cs 94
Обратите внимание на if верхнего уровня. В нём на null проверяется memberValue. Следовательно, в else блоке этого if переменная memberValue не null. Однако в этом блоке есть проверка:
if ( memberValue == null
&& ( memberType == Globals.TypeOfObject
|| (originValueIsNullableOfT && memberType.IsValueType)))Данное условие всегда ложно, так как проверка memberValue на null является одним из операндов оператора &&.
Фрагмент кода 2
protected override string GetName(NodeFactory factory)
{
return "Reflectable delegate type: "
+ _delegateType?.ToString() ?? "All delegates";
}Предупреждение PVS-Studio: 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. ReflectedDelegateNode.cs 38
Достаточно часто при написании кода, разработчики не учитывают, что оператор ?? имеет один из самых низких приоритетов. Это может негативно повлиять на логику работы программы. К счастью, в данном примере логика работы не пострадала, однако проблема всё равно есть. Если при выполнении метода GetName поле delegateType будет null, то вместо ожидаемого сообщения: "Reflectable delegate type: All delegates", мы получим: "Reflectable delegate type: ". Это обусловлено тем, что приоритет + выше, чем ??. Для исправления нужно взять часть выражения в скобки:
"Reflectable delegate type: " + (_delegateType?.ToString() ?? "All delegates");Фрагмент кода 3
public ISymbolNode GenericLookupHelper(
CORINFO_RUNTIME_LOOKUP_KIND runtimeLookupKind,
ReadyToRunHelperId helperId,
object helperArgument,
GenericContext methodContext)
{
switch (helperId)
{
case ReadyToRunHelperId.TypeHandle:
return GenericLookupTypeHelper(
runtimeLookupKind,
ReadyToRunFixupKind.TypeHandle,
helperArgument,
methodContext);
case ReadyToRunHelperId.MethodHandle:
return GenericLookupMethodHelper(
runtimeLookupKind,
ReadyToRunFixupKind.MethodHandle,
(MethodWithToken)helperArgument,
methodContext);
case ReadyToRunHelperId.MethodEntry:
return GenericLookupMethodHelper(
runtimeLookupKind,
ReadyToRunFixupKind.MethodEntry,
(MethodWithToken)helperArgument,
methodContext);
case ReadyToRunHelperId.MethodDictionary: // <=
return GenericLookupMethodHelper(
runtimeLookupKind,
ReadyToRunFixupKind.MethodHandle, // <=
(MethodWithToken)helperArgument,
methodContext);
case ReadyToRunHelperId.TypeDictionary:
return GenericLookupTypeHelper(
runtimeLookupKind,
ReadyToRunFixupKind.TypeDictionary,
(TypeDesc)helperArgument,
methodContext);
case ReadyToRunHelperId.VirtualDispatchCell:
return GenericLookupMethodHelper(
runtimeLookupKind,
ReadyToRunFixupKind.VirtualEntry,
(MethodWithToken)helperArgument,
methodContext);
case ReadyToRunHelperId.FieldHandle:
return GenericLookupFieldHelper(
runtimeLookupKind,
ReadyToRunFixupKind.FieldHandle,
(FieldWithToken)helperArgument,
methodContext);
default:
throw new NotImplementedException(helperId.ToString());
}
}Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. ReadyToRunSymbolNodeFactory.cs 583
Код содержит множество однотипных операций, записанных в одном формате. Неудивительно, что здесь присутствует крайне подозрительный момент. Если helperId соответствует ReadyToRunHelperId.MethodDictionary, то в метод GenericLookupMethodHelper передаётся ReadyToRunFixupKind.MethodHandle. Выглядит не совсем корректно, при условии, что в перечислении ReadyToRunFixupKind есть элемент MethodDictionary. Кажется, что именно он должен передаваться в GenericLookupMethodHelper, если helperId соответствует ReadyToRunHelperId.MethodDictionary.
Фрагмент кода 4
partial class MutableModule
{
....
public int CompareTo(MutableModule other)
{
return _index.CompareTo(_index);
}
}Предупреждение PVS-Studio: V3062 An object '_index' is used as an argument to its own method. Consider checking the first actual argument of the 'CompareTo' method. MutableModule.Sorting.cs 19
В методе CompareTo поле _index сравнивается само с собой. Скорее всего, в качестве аргумента должно передаваться other._index.
Фрагмент кода 5
public void ComputeReturnValueTreatment(....)
{
....
if (descriptor.passedInRegisters)
{
if (descriptor.eightByteCount == 1)
{
if (descriptor.eightByteClassifications0
== SystemVClassificationType.SystemVClassificationTypeSSE)
{
// Structs occupying just one eightbyte are treated as int / double
fpReturnSize = sizeof(double);
}
}
else
{
// Size of the struct is 16 bytes
fpReturnSize = 16;
// The lowest two bits of the size encode
// the order of the int and SSE fields
if (descriptor.eightByteClassifications0 // <=
== SystemVClassificationType.SystemVClassificationTypeSSE)
{
fpReturnSize += 1;
}
if (descriptor.eightByteClassifications0 // <=
== SystemVClassificationType.SystemVClassificationTypeSSE)
{
fpReturnSize += 2;
}
}
break;
}
....
}Предупреждение PVS-Studio: V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 364, 369. TransitionBlock.cs 364
Анализатор сообщает о последовательно идущих if с одинаковыми условиями. Это действительно так, descriptor.eightByteClassifications0 дважды сравнивается с SystemVClassificationType.SystemVClassificationTypeSSE. На мысль о том, что в одном из случаев проверка ошибочна, наталкивает наличие поля eightByteClassifications1 у descriptor. Скорее всего, именно оно должно использоваться в одной из проверок.
Фрагмент кода 6
public static bool SubstEqualTypeArrays(TypeArray taDst,
TypeArray taSrc,
TypeArray typeArgsCls,
TypeArray typeArgsMeth)
{
// Handle the simple common cases first.
if (taDst == taSrc || (taDst != null && taDst.Equals(taSrc))) // <=
{
// The following assertion is not always true and indicates a problem where
// the signature of override method does not match the one inherited from
// the base class. The method match we have found does not take the type
// arguments of the base class into account.
// So actually we are not overriding the method that we "intend" to.
// Debug.Assert(taDst == SubstTypeArray(taSrc,
typeArgsCls,
typeArgsMeth,
grfst));
return true;
}
if (taDst.Count != taSrc.Count) // <=
return false;
if (taDst.Count == 0)
return true;
var ctx = new SubstContext(typeArgsCls, typeArgsMeth, true);
if (ctx.IsNop)
{
return false;
}
for (int i = 0; i < taDst.Count; i++)
{
if (!SubstEqualTypesCore(taDst[i], taSrc[i], ctx))
return false;
}
return true;
}Предупреждение PVS-Studio: V3125 The 'taDst' object was used after it was verified against null. Check lines: 354, 344. TypeManager.cs 354
В условии первого if параметр taDst проверяется на null. Следовательно, предполагается, что этот параметр может иметь значение null. Однако в условии следующего if параметр taDst разыменовывается без проверки: taDst.Count. Если taDst будет null, а taSrc не null, то будет выброшено исключение типа NullReferenceException.
Фрагмент кода 7
internal void WriteXmlnsAttribute(XmlDictionaryString ns)
{
if (dictionaryWriter != null)
{
if (ns != null)
dictionaryWriter.WriteXmlnsAttribute(null, ns);
}
else
WriteXmlnsAttribute(ns.Value);
}Предупреждение PVS-Studio: V3125 The 'ns' object was used and was verified against null in different execution branches. Check lines: 71, 67. XmlWriterDelegator.cs 71
Проблема схожа с предыдущей. В блоке then первого if параметр ns перед использованием проверяется на null. В блоке else этой проверки уже нет. Возможно, параметр ns как-то связан с полем dictionaryWriter, что позволяет не проверять ns на null, если dictionaryWriter — null. Однако подтверждения этому найти не удалось.
На данный момент выглядит так, что если dictionaryWriter и ns будут иметь значение null, то при обращении к ns.Value выбросится исключение типа NullReferenceException.
Фрагмент кода 8
private struct ModuleAndIntValueKey : IEquatable<ModuleAndIntValueKey>
{
public bool Equals(ModuleAndIntValueKey other) =>
IntValue == other.IntValue
&& ( (Module == null && other.Module == null)
|| Module.Equals(other.Module));
}Предупреждение PVS-Studio: V3125 The 'Module' object was used after it was verified against null. Check lines: 180, 180. ReadyToRunCodegenNodeFactory.cs 180
При сравнении объектов типа ModuleAndIntValueKey проверяются их поля Module:
(Module == null && other.Module == null) || Module.Equals(other.Module)Если возникнет ситуация, в которой Module — null, а other.Module не null, то будет выброшено исключение типа NullReferenceException. Так как левый операнд оператора || будет false условие будет вычисляться дальше, что приведёт к разыменованию Module при вызове Equals.
Для исправления ситуации, нужно добавить проверку на null, перед обращением к Module.
Фрагмент кода 9
public bool IsInheritanceChainLayoutFixedInCurrentVersionBubble(TypeDesc type)
{
// This method is not expected to be called for value types
Debug.Assert(!type.IsValueType);
if (type.IsObject)
return true;
if (!IsLayoutFixedInCurrentVersionBubble(type))
{
return false;
}
type = type.BaseType;
if (type != null)
{
return false;
while (!type.IsObject && type != null)
{
if (!IsLayoutFixedInCurrentVersionBubble(type))
{
return false;
}
type = type.BaseType;
}
}
return true;
}Предупреждение PVS-Studio: V3027 The variable 'type' was utilized in the logical expression before it was verified against null in the same logical expression. ReadyToRunCodegenCompilation.cs 596
В условии while сначала обращаются к свойству IsObject переменной type, после чего проверяют type на null. Кажется, что такая проверка малоэффективна. Если type всё же окажется null, то будет выброшено исключение типа NullReferenceException.
Для исправления нужно либо поменять местами проверку на null и обращение, либо, если type никогда не null, убрать эту самую проверку.
Фрагмент кода 10
public class PropertyPseudoDesc : TypeSystemEntity
{
....
public static bool operator ==(PropertyPseudoDesc a, PropertyPseudoDesc b)
=> a._type == b._type && a._handle == b._handle;
....
}Предупреждение PVS-Studio: V3115 Passing 'null' to '==' operator should not result in 'NullReferenceException'. PropertyPseudoDesc.cs 89
Если при сравнении объектов типа PropertyPseudoDesc один из операндов будет null, выбросится исключение типа 'NullReferenceException'. Чтобы избежать подобной ситуации, перед обращением к полям операндов, нужно проверить, что они не null.
В ходе проверки исходного кода .NET 10 мы убедились, что даже спустя полгода после релиза в проекте сохраняются дефекты и потенциально проблемные участки. Часть из них относится к классическим ошибкам: неочевидные условия, избыточные проверки, возможные NRE и неоднозначные ветвления логики. Такие проблемы сложно заметить при беглом код-ревью, особенно в крупной кодовой базе уровня платформы.
Каждый год мы проверяем актуальную версию .NET:
Также регулярно публикуем проверки других популярных проектов. Если хотите всегда быть в курсе новых проверок, подпишитесь на нашу рассылку.
Если вы хотите самостоятельно проверить проект с помощью PVS-Studio, то попробовать анализатор можете по ссылке!
0
0
0
0