- PVSM.RU - https://www.pvsm.ru -
Около года назад Microsoft выложила в открытый доступ исходный код таких проектов, как CoreCLR и CoreFX. Последний проект до недавнего времени не был нам интересен, потому что написан на языке C#, а не C++. Но с выходом новой версии PVS-Studio 6.00, поддерживающей проекты и на языке программирования C#, я решил вернуться к CoreFX и написать статью.
.NET Core это модульная реализация библиотек и среды выполнения, которая включает подмножество .NET Framework. .NET Core состоит из набора библиотек, называемых «CoreFX» и небольшой оптимизированной рабочей среды «CoreCLR».
.NET Core распространяется с открытым исходным кодом, который доступен на GitHub:
Это крупные продукты от Microsoft, содержащие качественный исходный код, но подозрительные участки кода всё равно можно найти.
О проверке CoreCLR можно прочитать в статье "PVS-Studio: 25 подозрительных фрагментов кода из CoreCLR [3]".
Проект CoreFX, о котором подойдёт речь в статье, проверялся с помощью статического анализатора PVS-Studio [4] 6.00, который теперь поддерживает и C#!
Подготавливая статью о проверке открытого проекта [5], мы приводим в ней информацию далеко не о всех предупреждениях, которые выдаёт статический анализатор. Поэтому мы рекомендуем авторам проекта самостоятельно выполнить анализ и изучить все выдаваемые анализатором сообщения.
V3027 [6] The variable 'start.BaseMapping' was utilized in the logical expression before it was verified against null in the same logical expression. Mappings.cs 598
internal void SetSequence()
{
if (TypeDesc.IsRoot)
return;
StructMapping start = this;
// find first mapping that does not have the sequence set
while (!start.BaseMapping.IsSequence && //<==
start.BaseMapping != null && //<==???
!start.BaseMapping.TypeDesc.IsRoot)
start = start.BaseMapping;
....
}
В коде присутствует серьёзная логическая ошибка! В теле цикла объект с именем 'start' изменяется на каждой итерации, и цикл выполняется, пока объект находится в определённом состоянии. НО проверка условия «start.BaseMapping != null» выполняется после обращения к «start.BaseMapping.IsSequence», а это может привести к разыменованию нулевой ссылки.
V3019 [7] Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'comparand', 'comparedCredentialKey'. CredentialCache.cs 4007
public override bool Equals(object comparand)
{
CredentialHostKey comparedCredentialKey =
comparand as CredentialHostKey;
if (comparand == null)
{
// This covers also the compared == null case
return false;
}
bool equals = string.Equals(AuthenticationType,
comparedCredentialKey.AuthenticationType, ....
....
}
В функцию может прийти объект любого типа или null. Если придёт null, этот случай будет обработан корректно. Если это будет объект такого типа, который не удастся преобразовать к типу «CredentialHostKey», то произойдёт ошибка при обращении к «comparedCredentialKey.AuthenticationType», т.к. переменная «comparedCredentialKey» может быть равна null.
Скорее всего, хотели написать так:
CredentialHostKey comparedCredentialKey =
comparand as CredentialHostKey;
if (comparedCredentialKey == null)
{
return false;
}
Аналогичное место в коде:
V3008 [8] The 'HResult' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 169, 166. WebSocketException.cs 169
private void SetErrorCodeOnError(int nativeError)
{
if (!Succeeded(nativeError))
{
HResult = nativeError;
}
HResult = nativeError; //<==???
}
Почему-то независимо от условия, переменная «HResult» всегда принимает одно и то же значение. Скорее всего функция должна быть реализована другим образом.
V3008 [8] The 'ResPrec' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1735, 1731. SQLDecimal.cs 1735
public static SqlDecimal operator /(SqlDecimal x, SqlDecimal y)
{
int ResPrec;
....
ResPrec = ResScale + x.m_bPrec + y.m_bPrec + 1; //<==
MinScale = Math.Min(ResScale, s_cNumeDivScaleMin);
ResInteger = Math.Min(ResInteger, s_NUMERIC_MAX_PRECISION);
ResPrec = ResInteger + ResScale; //<==
if (ResPrec > s_NUMERIC_MAX_PRECISION)
ResPrec = s_NUMERIC_MAX_PRECISION;
....
}
Очень подозрительно, что значение переменной «ResPrec» вычисляется по некой формуле, но потом просто перетирается другим значением.
V3020 [9] An unconditional 'return' within a loop. Enumerable.cs 517
public override bool MoveNext()
{
switch (state)
{
case 1:
_enumerator = _source.GetEnumerator();
state = 2;
goto case 2;
case 2:
while (_enumerator.MoveNext())
{
current = _selector(_enumerator.Current);
return true;
}
Dispose();
break;
}
return false;
}
Странно, в теле цикла «while» выполняется выход из функции без какого-либо условия. Возможно, в коде присутствует ошибка.
Ещё один похожий цикл:
V3008 [8] The 'prefix' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 953, 952. XmlSerializationWriter.cs 953
protected void WriteAttribute(string localName, string ns, ....)
{
....
string prefix = localName.Substring(0, colon);
prefix = _w.LookupPrefix(ns);
_w.WriteStartAttribute(prefix,
localName.Substring(colon + 1), ns);
....
}
В переменную 'prefix' сохраняется подстрока из 'localName' длинной «colon», потом это значение перетирается другим. Дальше по коду видно, что используется оставшаяся подстрока из 'localName', а первая часть была потеряна. Очень сомнительный код.
V3030 [10] Recurring check. The 'baseTableRowCounts == null' condition was already verified in line 68. MetadataAggregator.cs 70
private MetadataAggregator(....)
{
....
if (baseTableRowCounts == null) //<==
{
if (baseReader == null)
{
throw new ArgumentNullException("deltaReaders");
}
if (baseReader.GetTableRowCount(TableIndex.EncMap) != 0)
{
throw new ArgumentException("....", "baseReader");
}
CalculateBaseCounts(baseReader, out baseTableRowCounts, //<==
out baseHeapSizes);
}
else
{
if (baseTableRowCounts == null) //<==???
{
throw new ArgumentNullException("baseTableRowCounts");
}
....
}
....
}
Анализатор обнаружил условие, которое уже проверялось. Если посмотреть на фрагмент кода, то последняя проверка в 'else' — «baseTableRowCounts == null» не имеет смысла. Зато выше по коду можно увидеть, что если переменная «baseTableRowCounts» равна null, то её значение пытаются изменить вызовом функции CalculateBaseCounts(). Вот после этой функции, скорее всего, и не хватает дополнительной проверки «baseTableRowCounts == null». Т.е. скорее всего код должен выглядеть так:
private MetadataAggregator(....)
{
....
if (baseTableRowCounts == null)
{
if (baseReader == null)
{
throw new ArgumentNullException("deltaReaders");
}
if (baseReader.GetTableRowCount(TableIndex.EncMap) != 0)
{
throw new ArgumentException("....", "baseReader");
}
CalculateBaseCounts(baseReader, out baseTableRowCounts,
out baseHeapSizes);
if (baseTableRowCounts == null)
{
throw new ArgumentNullException("baseTableRowCounts");
}
}
else
{
....
}
....
}
V3022 [11] Expression 'readercount >= 0' is always true. Unsigned type value is always >= 0. ReaderWriterLockSlim.cs 977
private void ExitAndWakeUpAppropriateWaitersPreferringWriters()
{
....
uint readercount = GetNumReaders();
....
if (readercount == 1 && _numWriteUpgradeWaiters > 0)
{
....
}
else if (readercount == 0 && _numWriteWaiters > 0)
{
ExitMyLock();
_writeEvent.Set();
}
else if (readercount >= 0)
{
....
}
else
ExitMyLock();
....
}
Переменная «readercount» имеет беззнаковый тип, поэтому условие «readercount >= 0» не имеет смысла. Скорее всего, раньше она была знакового типа, но тогда для функции ExitMyLOck() в последнем 'else' был хоть какой-то шанс выполниться. Теперь этот код никогда не получает управления. Необходимо переписать это место.
V3014 [12] It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. RegexCharClass.cs 1094
private void Canonicalize()
{
....
for (i = 1, j = 0; ; i++)
{
for (last = _rangelist[j]._last; ; i++)
{
if (i == _rangelist.Count || last == LastChar)
{
done = true;
break;
}
if ((CurrentRange = _rangelist[i])._first > last + 1)
break;
if (last < CurrentRange._last)
last = CurrentRange._last;
}
_rangelist[j] = new SingleRange(_rangelist[j]._first, last);
j++;
if (done)
break;
if (j < i)
_rangelist[j] = _rangelist[i];
}
_rangelist.RemoveRange(j, _rangelist.Count - j);
....
}
Анализатор обнаружил изменение счётчика одного цикла в другом. Сложно сказать, есть ли в этой функции ошибка, но написано не очень понятно. Вполне можно где-нибудь ошибиться в индексе при обращении к массиву, т.к. в таком коде сложно следить за изменением одного счётчика в нескольких циклах.
V3004 [13] The 'then' statement is equivalent to the 'else' statement. XmlSerializationWriterILGen.cs 1213
private void WriteMember(...., TypeDesc memberTypeDesc, ....)
{
....
if (memberTypeDesc.IsArray)
{
LocalBuilder localI = ilg.DeclareOrGetLocal(typeof(Int32), iVar);
ilg.For(localI, 0, ilg.GetLocal(aVar));
}
else
{
LocalBuilder localI = ilg.DeclareOrGetLocal(typeof(Int32), iVar);
ilg.For(localI, 0, ilg.GetLocal(aVar));
}
....
}
Условие, которое ни на что не влияет, т.к. всегда будет выполнятся один код. Классический copy-paste.
V3004 [13] The 'then' statement is equivalent to the 'else' statement. SqlUtil.cs 93
internal static void ContinueTask(....)
{
....
if (connectionToDoom != null || connectionToAbort != null)
{
try
{
onSuccess();
}
catch (Exception e)
{
completion.SetException(e);
}
}
else
{ // no connection to doom - reliability section not required
try
{
onSuccess();
}
catch (Exception e)
{
completion.SetException(e);
}
}
....
}
Тут тоже что-то много одинакового кода в условии, хотя в комментарии написано, что ситуации разные.
Вот проверен ещё один проект от Microsoft. Для такого объёма проект имеет довольно качественный код, но программисты всё равно могут допускать ошибки. Статья является обзорной и содержит далеко не все предупреждения анализатора, которые были получены в отчёте.
Качественному коду способствуют два очень важных обстоятельства:
Надеемся, вам понравилась эта статья. Обещаем и дальше радовать наших читателей проверками интересных открытых проектов на языках C/C++ и C#.
Спасибо за внимание. И безбажного вам кода в Новом Году!
Автор: PVS-Studio
Источник [14]
Сайт-источник PVSM.RU: https://www.pvsm.ru
Путь до страницы источника: https://www.pvsm.ru/c-2/107527
Ссылки в тексте:
[1] .NET Core Libraries (CoreFX): https://github.com/dotnet/corefx
[2] .NET Core Common Language Runtime (CoreCLR): https://github.com/dotnet/coreCLR
[3] PVS-Studio: 25 подозрительных фрагментов кода из CoreCLR: http://habrahabr.ru/company/pvs-studio/blog/253280/
[4] PVS-Studio: http://www.viva64.com/ru/pvs-studio/
[5] открытого проекта: http://www.viva64.com/ru/a/0084/
[6] V3027: http://www.viva64.com/ru/d/0414/
[7] V3019: http://www.viva64.com/ru/d/0388/
[8] V3008: http://www.viva64.com/ru/d/0395/
[9] V3020: http://www.viva64.com/ru/d/0410/
[10] V3030: http://www.viva64.com/ru/d/0415/
[11] V3022: http://www.viva64.com/ru/d/0391/
[12] V3014: http://www.viva64.com/ru/d/0384/
[13] V3004: http://www.viva64.com/ru/d/0402/
[14] Источник: http://habrahabr.ru/post/274191/
Нажмите здесь для печати.