Писать одно и то же — невероятно муторное занятие! Поэтому программисты регулярно используют copy-paste, но, помимо экономии времени, также растет и количество ошибок. Чтобы в этом убедиться, мы разберём ошибки и странные места проекта Radarr.
Radarr — менеджер фильмов для пользователей Usenet и BitTorrent с открытым исходным кодом. Проект поддерживает мониторинг RSS-фидов для того, чтобы скачивать фильмы и обновлять их по мере появления нужных версий и разрешений.
Проект достаточно хорошо написан и содержит не так много ошибок. Но тем не менее есть существенное количество слабых мест, которые появились из-за комбинации невнимательности и использовании copy-paste.
Для проведения анализа использовался исходный код, доступный на GitHub по ссылке.
Перейдём к рассмотрению обнаруженных анализатором ошибок!
Фрагмент кода 1
public bool Equals(OsPath other, bool ignoreTrailingSlash)
{
if (ReferenceEquals(other, null))
{
return false;
}
....
}Предупреждение PVS-Studio: V3161 Comparing value type variables with 'ReferenceEquals' is incorrect because 'other' will be boxed. OsPath.cs 379
В этом случае выполняется сравнение объекта значимого типа по ссылке. Метод ReferenceEquals принимает параметр типа Object, и при передаче значимого типа будет произведена упаковка. Созданная в куче ссылка не будет равна никакой другой, и из-за этого метод всегда будет возвращать false.
Приведу пример, который иллюстрирует, почему важно помнить nullable value types:
static void NullableTest()
{
int? a = null;
object aObj = a;
int? b = new int?();
object bObj = b;
Console.WriteLine(Object.ReferenceEquals(aObj, bObj)); // True or False?
}Предлагаю вам внимательно изучить фрагмент кода выше и ответить, что будет выведено в консоль. Для тех, кто хочет себя проверить или более подробно изучить этот вопрос, оставляю ссылку на статью.
Фрагмент кода 2
switch (format.ToUpperInvariant())
{
case "L": // Long format
var stringBuilder = new StringBuilder();
stringBuilder.AppendLine("Guid: " + Guid ?? "Empty");
....
default:
return ToString();
}Предупреждение 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. ReleaseInfo.cs 87
На первый взгляд этот фрагмент кода тоже не вызывает подозрений, но автор забыл о том, что операторы ?? и ?: имеют более низкий приоритет по сравнению с оператором +. Таким образом, выражение "Guid: " + Guid будет отрабатывать первым, и даже если переменная Guid равна null, то ?? "Empty" всё равно не будет выполнена.
Ситуация, когда программист один раз ошибся и потом преумножил ошибку копипастом, достаточно распространена даже в больших проектах. Так, например, в проекте Azure SDK для .NET можно было встретить следующие фрагменты кода:
public struct BlobSasBuilder : IEquatable<BlobSasBuilder>
{
....
public bool Equals(BlobSasBuilder other) =>
BlobName == other.BlobName &&
CacheControl == other.CacheControl &&
BlobContainerName == other.BlobContainerName &&
ContentDisposition == other.ContentDisposition &&
ContentEncoding == other.ContentEncoding && // <=
ContentLanguage == other.ContentEncoding && // <=
....
}Предупреждение PVS-Studio: V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'ContentLanguage == other.ContentEncoding'. BlobSasBuilder.cs 410
И точно такая же ошибка встречается в другом месте:
public struct FileSasBuilder : IEquatable<FileSasBuilder>
{
....
public bool Equals(FileSasBuilder other)
=> CacheControl == other.CacheControl
&& ContentDisposition == other.ContentDisposition
&& ContentEncoding == other.ContentEncoding // <=
&& ContentLanguage == other.ContentEncoding // <=
....
}V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'ContentLanguage == other.ContentEncoding'. FileSasBuilder.cs 265
Можете более подробно почитать про проверку Azure SDK для .NET в этой статье.
Такую базовую вещь можно случайно забыть и написать ошибочный код, подобный приведенному выше. Встроенные в IDE анализаторы не способны выявить подобные паттерны, и проблема останется незамеченной. Но в этой ситуации выручают инструменты статического анализа.
Фрагмент кода 3
var qualityComparer = new QualityModelComparer(qualityProfile);
var qualityCompare = qualityComparer.Compare(newQuality?.Quality, // <=
currentQuality.Quality);
var downloadPropersAndRepacks = _configService.DownloadPropersAndRepacks;
if (qualityCompare > 0 &&
QualityCutoffNotMet(qualityProfile,
currentQuality,
newQuality))
{
_logger.Debug("New item has a better quality.
Existing: {0}. New: {1}",
currentQuality,
newQuality);
return UpgradeableRejectReason.None;
}Предупреждение PVS-Studio: V3105 The result of null-conditional operator is dereferenced inside the 'Compare' method. NullReferenceException is possible. Inspect the first argument 'newQuality?.Quality'. UpgradableSpecification.cs 34
Анализатор сообщает, что возможно возникновение NullReferenceException при выполнении программы. В коде мы передали поле переменной без проверки её на null. Ключевым моментом является то, что значение переменной вычисляется с помощью выражения, где используется оператор null-conditional. Также в методе Compare не производится проверка аргумента на null, значит, при попытке обратиться к полю Quality, может возникнуть NullReferenceException.
Рассмотрим пример:
void UsersProcessing(Users users)
{
....
LogUserNames(users?.GetUsersCollection());
}
void LogUserNames(IEnumerable<User> usersList)
{
foreach (var user in usersList)
{
....
}
}Переменная 'usersList' передаётся в качестве аргумента методу 'LogUserNames'. Она может иметь значение 'null', так как оно получено с помощью оператора null-conditional. Внутри 'LogUserNames' производится обход переданной коллекции. Для этого используется 'foreach' и, следовательно, у коллекции будет вызван метод 'GetEnumerator'. Если 'userList' имеет значение 'null', то будет выброшено исключение типа 'NullReferenceException'.
Вариант с исправлениями может выглядеть следующим образом:
void UsersProcessing(Users users)
{
....
LogUserNames( users?.GetUsersCollection()
?? Enumerable.Empty<User>());
}
void LogUserNames(IEnumerable<User> usersList)
{
foreach (var user in usersList)
{
....
}
}Результат выполнения 'users?.GetUsersCollection()' присваивается переменной 'usersList'. Если значение этой переменной 'null', то в метод 'LogUserNames' будет передаваться пустая коллекция. Это поможет избежать исключения типа 'NullReferenceException' при обходе 'usersList' в 'foreach'.
Подобный паттерн ошибок часто встречается при использовании foreach. Об этом у нас есть статья.
Фрагмент кода 4
if (resource.Languages != null)
{
// Don't allow user to set movieFile with 'Any' or 'Original' language
movieFile.Languages = resource.Languages
.Where(l => l != Language.Any
|| l != Language.Original
|| l != null)
.ToList();
}Предупреждение PVS-Studio: V3063 A part of conditional expression is always true if it is evaluated: l != null. MovieFileController.cs 122
Здесь явно намудрили с условием. Из комментария понятно, что нельзя позволить пользователю установить язык на Any или Original, но этому условию будут удовлетворять вообще все элементы. Допустим, на проверку поступает элемент, равный Language.Any. Первую проверку он завалит, но вторую пройдёт. Аналогично дело обстоит с элементами, равными Language.Original. А элемент, не равный Language.Any и Language.Original, удовлетворит любое из условий.
Делаем вывод, что программист ошибся и должен был написать && вместо ||.
Фрагмент кода 5
var rootFolder = Path.GetDirectoryName(path);
var movie = _movieService.GetMovie(movieId);
var languageParse = LanguageParser.ParseLanguages(path);
if ( languageParse.Count <= 1
&& languageParse.First() == Language.Unknown // <=
&& movie != null)
{
}Предупреждение PVS-Studio: V3179 Calling the 'First' method for potentially empty collection 'languageParse' may result in exception. ManualImportService.cs 107
Анализатор выдал предупреждение с низким уровнем важности, но тем не менее мне эта ситуация кажется интересной. В условии if проверяется, что коллекция languageParse имеет один элемент или меньше. Если получится, что у languageParse не будет элементов вовсе, то при попытке получить первый элемент с помощью функции First программа получит исключение.
Подозрительное место в этой ситуации не является существенным, поскольку очень маловероятно, что у languageParse не будет элементов из-за предшествующего кода.
Фрагмент кода 6
public virtual bool IsServiceRunning(string name)
{
_logger.Debug("Checking if '{0}' service is running", name);
....
return service != null && (
service.Status != ServiceControllerStatus.Stopped
|| service.Status == ServiceControllerStatus.StopPending
|| service.Status == ServiceControllerStatus.Paused
|| service.Status == ServiceControllerStatus.PausePending);
}Предупреждение PVS-Studio: V3063 A part of conditional expression is always false if it is evaluated: service.Status == ServiceControllerStatus.StopPending. ServiceProvider.cs 55
Анализатор сообщает, что второе условие будет всегда ложным, если до него дойдёт дело. Так и есть: если первая проверка service.Status окажется ложной, а это произойдёт только если service.Status == ServiceControllerStatus.Stopped, то и все остальные проверки будут ложными. Возможно, программист опечатался и хотел использовать == вместо != в условии service.Status != ServiceControllerStatus.Stopped
Фрагмент кода 7
switch (torrent.Status)
{
....
case "error":
status = DownloadItemStatus.Failed;
break;
case "complete":
status = DownloadItemStatus.Completed;
break;
case "removed":
status = DownloadItemStatus.Failed;
break;
}Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. Aria2.cs 118
Здесь анализатор обращает наше внимание на то, что две или более ветви case выполняют одно и то же присваивание. Без знания того, какой функционал закладывался в код, сложно понять, есть ли здесь ошибка.
В пользу того, что перед нами может быть ошибка, скажу, что у перечисления DownloadItemStatus есть неиспользованный элемент Warning, который, возможно, подходит по смыслу.
Фрагмент кода 8
Разработчик оставил простенькую ошибку для внимательных, поэтому предлагаю вам попробовать самостоятельно найти опечатку:
switch (Settings.TraktListType)
{
case (int)TraktPopularListType.Trending:
link += "movies/trending";
break;
case (int)TraktPopularListType.Popular:
link += "movies/popular";
break;
case (int)TraktPopularListType.Anticipated:
link += "movies/anticipated";
break;
case (int)TraktPopularListType.BoxOffice:
link += "movies/boxoffice";
break;
case (int)TraktPopularListType.TopWatchedByWeek:
link += "movies/watched/weekly";
break;
case (int)TraktPopularListType.TopWatchedByMonth:
link += "movies/watched/monthly";
break;
case (int)TraktPopularListType.TopWatchedByYear:
link += "movies/watched/yearly";
break;
case (int)TraktPopularListType.TopWatchedByAllTime:
link += "movies/watched/all";
break;
case (int)TraktPopularListType.RecommendedByWeek:
link += "movies/recommended/weekly";
break;
case (int)TraktPopularListType.RecommendedByMonth:
link += "movies/recommended/monthly";
break;
case (int)TraktPopularListType.RecommendedByYear:
link += "movies/recommended/yearly";
break;
case (int)TraktPopularListType.RecommendedByAllTime:
link += "movies/recommended/yearly";
break;
}Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. TraktPopularRequestGenerator.cs 65
Здесь проблема схожа с той, что была в предыдущем фрагменте кода. Эта ошибка, на мой взгляд, более очевидная и, скорее всего, допущена из-за невнимательности при использовании copy-paste. По смыслу понятно, что переменная link в последнем case должна принимать значение movies/recommended/all вместо movies/recommended/yearly. На это нам намекает код выше.
Справиться с такой ошибкой иногда и код ревью не поможет: у всех глаза раньше отвалятся, а у статического анализатора ничего не отвалится, и сделает он это практически моментально :)
Фрагмент кода 9
public override bool Enable
=> OnGrab
|| OnDownload
||(OnDownload && OnUpgrade)
|| OnRename
|| OnMovieAdded
|| OnMovieDelete
|| OnMovieFileDelete
||(OnMovieFileDelete && OnMovieFileDeleteForUpgrade)
|| OnHealthIssue
|| OnHealthRestored
|| OnApplicationUpdate
|| OnManualInteractionRequired;Предупреждение PVS-Studio: V3017 A pattern was detected: (OnDownload) || ((OnDownload) && ...). The expression is excessive or contains a logical error. NotificationDefinition.cs 62
Тот самый случай, когда в одной строке допущены сразу две ошибки. Надеюсь, всем понятно, что не надо дублировать условия и нужно стараться писать код так, чтобы его можно было легко прочитать. А если неочевидно, то поясню: условие OnDownload ||(OnDownload && OnUpgrade) является излишним, поскольку ему достаточно, чтобы OnDownload имело значение true. Аналогичным образом дела обстоят с условиями OnMovieFileDelete || (OnMovieFileDelete && OnMovieFileDeleteForUpgrade).
Фрагмент кода 10
if (importList == null || !definition.Enable)
{
_logger.Debug("Import List {0} ({1}) is not enabled, skipping.",
importList.Name, // <=
importList.Definition.Name); // <=
return result;
}Предупреждение PVS-Studio: V3125 The 'importList' object was used after it was verified against null. Check lines: 147, 145. FetchAndParseImportListService.cs 147
Здесь произошла ошибка по невнимательности. Очевидно, что разработчик хотел сделать проверку на null. Но что-то пошло не так, и, если importList равен null, будет обращение по нулевой ссылке в местах, отмеченных // <=, что вызовет ошибку.
Фрагмент кода 11
if (mediaInfo.VideoHdrFormatCompatibility
.IsNotNullOrWhiteSpace())
{
if (mediaInfo.VideoHdrFormatCompatibility
.ContainsIgnoreCase("HLG"))
{
return HdrFormat.Hlg10;
}
if (mediaInfo.VideoHdrFormatCompatibility
.ContainsIgnoreCase("dolby"))
{
return HdrFormat.DolbyVision;
}
if (mediaInfo.VideoHdrFormatCompatibility // <=
.ContainsIgnoreCase("dolby"))
{
return HdrFormat.DolbyVision; // <=
}
if (mediaInfo.VideoHdrFormatCompatibility
.ContainsIgnoreCase("hdr10+"))
{
return HdrFormat.Hdr10Plus;
}
if (mediaInfo.VideoHdrFormatCompatibility
.ContainsIgnoreCase("hdr10"))
{
return HdrFormat.Hdr10;
}
}Предупреждение PVS-Studio: V3022 Expression 'mediaInfo.VideoHdrFormatCompatibility.ContainsIgnoreCase("dolby")' is always false. 199_mediainfo_to_ffmpeg.cs 351
Здесь также явно прослеживается ошибка, допущенная из-за невнимательного копипаста. Оператор if с идентичным условием можно смело убрать, поскольку он никогда не выполнит свою роль. Я предполагаю, что программист забыл изменить параметр внутри метода mediaInfo.VideoHdrFormatCompability.ContainsIgnoreCase и возвращаемый элемент HdrFormat. На эту мысль меня натолкнул тот факт, что в перечислении HdrFormat ещё есть DolbyVisionHdr10, DolbyVisionSdr, DolbyVisionHlg и DolbyVisionHdr10Plus.
Фрагмент кода 12
if (videoFormat.ContainsIgnoreCase("MPEG-4 Visual"))
{
m.VideoFormat = "mpeg4";
....
return;
}
if ( videoFormat.ContainsIgnoreCase("MPEG-4 Visual") // <=
|| videoFormat.ContainsIgnoreCase("mp4v"))
{
m.VideoFormat = "mpeg4";
....
return;
}Предупреждение PVS-Studio: V3063 A part of conditional expression is always false if it is evaluated: videoFormat.ContainsIgnoreCase("MPEG-4 Visual"). 199_mediainfo_to_ffmpeg.cs 205
Первая часть условия, отмеченного оператором if, является всегда ложной, поскольку, если условие верное, программа остановится ещё на первом if. Исходя из значения, которое присваивается переменной m.VideoFormat, я бы предположил, что это ошибка копирования. И условие должно проверять, соответствует ли формат видео mp4v, и, соответственно, в переменную m.VideoFormat нужно записывать mp4v.
Опять же, без знания, как должна работать программа, нельзя однозначно говорить, что перед нами ошибка. Скорее это просто странность программы, о которой нужно сообщить разработчикам.
Фрагмент кода 13
if (Type == TokenType.UnknownSymbol) // <=
{
Value = Buffer[Index].ToString();
Index++;
return Type;
}
....
if ( end >= Buffer.Length
|| Buffer[end] == ','
|| Buffer[end] == ')'
|| Buffer[end] == '('
|| char.IsWhiteSpace(Buffer[end]))
{
Index = end;
}
else if (Type == TokenType.UnknownSymbol) // <=
{
Value = Buffer[Index].ToString();
Index++;
return Type;
}Предупреждение PVS-Studio: V3022 Expression 'Type == TokenType.UnknownSymbol' is always false. SqliteSyntaxReader.cs 166
Здесь нижнее выражение Type == TokenType.UnknownSymbol будет всегда ложным, потому что сверху есть точно такое же условие, внутри которого находится return. Если условие будет true, то выполнение программы в контексте этого метода закончится в теле первого if.
Подводя итог, можно заявить, что проект содержит не так уж и много ошибок, хоть и рассмотрели мы далеко не все. Зачастую это были простые опечатки, которые допустили из-за невнимательности. Но на примере этого приложения можно сделать выводы о том, что даже такие мелкие вещи, как пренебрежение приоритетом операций, порой доставляют большие проблемы даже в таких крупных проектах, как Azure.
Также следует помнить о том, что чем больше проект, тем больше ответственность, а также возрастает и плотность ошибок. Поэтому лучше всего отлавливать их на ранних стадиях разработки с помощью статического анализатора.
Если вы хотите писать безопасный код и не боятся опечаток, то можете попробовать анализатор PVS-Studio по ссылке.
Удачи в ваших проектах, и не забывайте проверять свой код!