Мы используем куки, чтобы пользоваться сайтом
было удобно.
Сегодня мы вновь говорим о качестве C# кода и разнообразии возможных ошибок. На нашем операционном столе – CMS DotNetNuke, в исходный код которой мы и залезем. И лучше сразу заварите себе кофе...
DotNetNuke – это система управления контентом (CMS) с открытым исходным кодом, преимущественно написанная на C#. Исходный код проекта доступен на GitHub. Также стоит отметить, что проект является частью .NET Foundation.
У проекта есть свой сайт, Twitter, YouTube-канал.
Однако для меня остался открытым вопрос, насколько проект живой? Если судить по репозиторию GitHub – в коде делаются какие-то изменения, проходят релизы. Если посмотреть на Twitter или YouTube канал – они достаточно давно не обновлялись.
С другой стороны – есть даже отдельный сайт сообщества, там же есть новости про какие-то ивенты.
Но, так или иначе, нас в первую очередь интересует код. Код и его качество.
Кстати, на странице проекта (ниже скриншот с неё) указано, что качество кода контролируется с помощью статического анализатора NDepend.
Я не могу сказать, как настроен этот анализатор на проекте, обрабатываются ли его предупреждения и так далее, но хочу напомнить, что статический анализатор должен использоваться регулярно. По-хорошему он должен быть тесно вплетён в ваш процесс разработки. Про это написано множество статей, найти которые можно, в том числе, в нашем блоге.
Для проверки проекта я использовал исходники с GitHub от 22.10.2021. Прошу учитывать, что от момента написания до момента публикации / чтений этой статьи пройдёт определённое время, за которое код может несколько измениться.
Для проверки использовался статический анализатор PVS-Studio версии 7.15. Если хотите попробовать анализатор на своём проекте – переходите на эту страничку, которая проведёт вас по всем необходимым шагам. Появились вопросы или что-то непонятно? Смело пишите нам.
А начать мне сегодня хочется с одного из нововведений релиза PVS-Studio 7.15 – списка лучших предупреждений. Фича новая, и в будущем ещё будет улучшаться, но пользоваться можно (и нужно!) уже сейчас.
Допустим, вы решили попробовать какой-нибудь статический анализатор на своём проекте. Загрузили его, проанализировали проект и... получили кучу предупреждений. Десятки, сотни, тысячи, может, даже десятки тысяч. Ого, вот это номер... Хотелось бы каким-нибудь магическим образом отобрать из них, скажем, Топ-10 наиболее интересных. Таких, чтобы смотришь и понимаешь: "Да, это какая-то ерунда в коде, определённо!". Что ж, теперь в PVS-Studio есть такой механизм, и имя ему – best warnings.
Пока он реализован только в плагине для Visual Studio, но позже мы планируем добавить его и в плагины для других IDE. Суть проста – анализатор отбирает из всего лога наиболее интересные и правдоподобные предупреждения.
Ну что, посмотрим, что у нас попало в список best warnings на этом проекте?
Best warnings. Issue 1
public string NavigateURL(int tabID,
bool isSuperTab,
IPortalSettings settings,
....)
{
....
if (isSuperTab)
{
url += "&portalid=" + settings.PortalId;
}
TabInfo tab = null;
if (settings != null)
{
tab = TabController.Instance.GetTab(tabID,
isSuperTab ? Null.NullInteger : settings.PortalId, false);
}
....
}Предупреждение PVS-Studio: V3095 The 'settings' object was used before it was verified against null. Check lines: 190, 195. DotNetNuke.Library NavigationManager.cs 190
Интересный момент здесь состоит в том, что сначала идёт обращение к экземплярному свойству settings.PortalId, а затем идёт проверка settings на неравенство null. Как следствие, если settings – null и isSuperTab – true, получим исключение типа NullReferenceException.
Что интересно, в данном фрагменте кода есть и второй контракт, связывающий параметры isSuperTab и settings – тернарный оператор: isSuperTab ? Null.NullInteger : settings.PortalId. Но обратите внимание, что в этом случае, в отличие от if, settings.PortalId будет использоваться тогда, когда isSuperTab имеет значение false.
И тут можно заметить, что, если isSuperTab – true, значение settings.PortalId не обрабатывается. Может показаться, что вот такой неявный контракт заложен, и в принципе всё ОК.
Нет.
Код должен быть легкочитаем и понятен, чтобы не приходилось играть в Шерлока. Если этот контракт подразумевается – пропишите его явно в коде, чтобы никого не сбивать с толку: ни разработчиков, ни статический анализатор, ни себя спустя какое-то время. ;)
Best warnings. Issue 2
private static string GetTableName(Type objType)
{
string tableName = string.Empty;
// If no attrubute then use Type Name
if (string.IsNullOrEmpty(tableName))
{
tableName = objType.Name;
if (tableName.EndsWith("Info"))
{
// Remove Info ending
tableName.Replace("Info", string.Empty);
}
}
....
}Предупреждение PVS-Studio: V3010 The return value of function 'Replace' is required to be utilized. DotNetNuke.Library CBO.cs 1038
Здесь мы опять сталкиваемся сразу с несколькими интересными моментами:
На первый кейс и указывает приведённое предупреждение анализатора. Второй кейс, кстати, также был обнаружен анализатором, но предупреждение не попало в список best warnings. Вот оно: V3022 Expression 'string.IsNullOrEmpty(tableName)' is always true. DotNetNuke.Library CBO.cs 1032
Best warnings. Issue 3
public static ArrayList GetFileList(...., string strExtensions, ....)
{
....
if ( strExtensions.IndexOf(
strExtension,
StringComparison.InvariantCultureIgnoreCase) != -1
|| string.IsNullOrEmpty(strExtensions))
{
arrFileList.Add(new FileItem(fileName, fileName));
}
....
}Предупреждение PVS-Studio: V3027 The variable 'strExtensions' was utilized in the logical expression before it was verified against null in the same logical expression. DotNetNuke.Library Globals.cs 3783
В строке strExtensions пытаются найти подстроку strExtension. Если подстроку найти не удалось, тогда проверяют, а не является ли strExtensions пустой или равной null. Вот только если strExtensions – null, уже при вызове экземплярного метода IndexOf будет выброшено исключение типа NullReferenceException.
Если вдруг подразумевается, что strExtension может быть пустой строкой, но никогда не имеет значения null, можно более явно выразить свои намерения: strExtensions.Length == 0.
Но данный фрагмент кода в любом случае стоит поправить – как и в случае с Issue 1, к нему сразу возникают вопросы.
Best warnings. Issue 4
public static void KeepAlive(Page page)
{
....
var scriptBlock = string.Format(
"(function($){{setInterval(
function(){{$.get(location.href)}}, {1});}}(jQuery));",
Globals.ApplicationPath,
seconds);
....
}Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: Globals.ApplicationPath. DotNetNuke.Library jQuery.cs 402
Подозрительные операции с форматированными строками – значение переменной seconds в результирующую строку подставлено будет, а вот для Globals.ApplicationPath в ней места не нашлось из-за отсутствия {0} в строке формата.
Best warnings. Issue 5
private void ProcessRequest(....)
{
....
if (!result.RewritePath.ToLowerInvariant().Contains("tabId="))
....
}Предупреждение PVS-Studio: V3122 The 'result.RewritePath.ToLowerInvariant()' lowercase string is compared with the '"tabId="' mixed case string. DotNetNuke.Library AdvancedUrlRewriter.cs 2252
По-моему, срабатываний этой диагностики лично я на реальных проектах ещё не встречал. Что ж, всё бывает в первый раз. :)
Из RewritePath берут строку, приводят её к нижнему регистру и проверяют, не содержит ли она подстроку "tabId=". Вот только проблема в том, что исходная строка у нас в нижнем регистре, а проверяемая содержит, в том числе, символы в верхнем.
Best warnings. Issue 6
protected override void RenderEditMode(HtmlTextWriter writer)
{
....
// Add the Not Specified Option
if (this.ValueField == ListBoundField.Text)
{
writer.AddAttribute(HtmlTextWriterAttribute.Value, Null.NullString);
}
else
{
writer.AddAttribute(HtmlTextWriterAttribute.Value, Null.NullString);
}
....
}Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. DotNetNuke.Library DNNListEditControl.cs 380
Классика copy-paste: одинаковые тела then и else-веток оператора if.
Best warnings. Issue 7
public static string LocalResourceDirectory
{
get
{
return "App_LocalResources";
}
}
private static bool HasLocalResources(string path)
{
var folderInfo = new DirectoryInfo(path);
if (path.ToLowerInvariant().EndsWith(Localization.LocalResourceDirectory))
{
return true;
}
....
}Предупреждение PVS-Studio: V3122 The 'path.ToLowerInvariant()' lowercase string is compared with the 'Localization.LocalResourceDirectory' mixed case string. Dnn.PersonaBar.Extensions LanguagesController.cs 644
Вновь знакомые грабли, только в этот раз чуть менее очевидные. Значение path приводится к нижнему регистру и проверяется, что оно оканчивается на строку, заведомо содержащую символы в верхнем регистре – "App_LocalResources" (литерал, возвращаемый из свойства LocalResourceDirectory).
Best warnings. Issue 8
internal static IEnumerable<PropertyInfo> GetEditorConfigProperties()
{
return
typeof(EditorConfig).GetProperties()
.Where(
info => !info.Name.Equals("Magicline_KeystrokeNext")
&& !info.Name.Equals("Magicline_KeystrokePrevious")
&& !info.Name.Equals("Plugins")
&& !info.Name.Equals("Codemirror_Theme")
&& !info.Name.Equals("Width")
&& !info.Name.Equals("Height")
&& !info.Name.Equals("ContentsCss")
&& !info.Name.Equals("Templates_Files")
&& !info.Name.Equals("CustomConfig")
&& !info.Name.Equals("Skin")
&& !info.Name.Equals("Templates_Files")
&& !info.Name.Equals("Toolbar")
&& !info.Name.Equals("Language")
&& !info.Name.Equals("FileBrowserWindowWidth")
&& !info.Name.Equals("FileBrowserWindowHeight")
&& !info.Name.Equals("FileBrowserWindowWidth")
&& !info.Name.Equals("FileBrowserWindowHeight")
&& !info.Name.Equals("FileBrowserUploadUrl")
&& !info.Name.Equals("FileBrowserImageUploadUrl")
&& !info.Name.Equals("FilebrowserImageBrowseLinkUrl")
&& !info.Name.Equals("FileBrowserImageBrowseUrl")
&& !info.Name.Equals("FileBrowserFlashUploadUrl")
&& !info.Name.Equals("FileBrowserFlashBrowseUrl")
&& !info.Name.Equals("FileBrowserBrowseUrl")
&& !info.Name.Equals("DefaultLinkProtocol"));
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions '!info.Name.Equals("Templates_Files")' to the left and to the right of the '&&' operator. DNNConnect.CKEditorProvider SettingsUtil.cs 1451
Я немного отформатировал этот код для лучшего восприятия. Анализатор обнаружил подозрительный дубль проверок: !info.Name.Equals("Templates_Files"). Или этот код просто избыточен, или здесь затерялась какая-то нужная проверка.
На самом деле, здесь есть и другие дубли, о которых анализатор почему-то не сообщил (взяли на карандаш). Например, также по два раза встречаются следующие выражения:
Три дублирующиеся проверки в рамках одного выражения – неплохо. На моей памяти это, вроде как, рекорд.
Best warnings. Issue 9
private void ProcessContentPane()
{
....
string moduleEditRoles
= this.ModuleConfiguration.ModulePermissions.ToString("EDIT");
....
moduleEditRoles
= moduleEditRoles.Replace(";", string.Empty).Trim().ToLowerInvariant();
....
if ( viewRoles.Equals(this.PortalSettings.AdministratorRoleName,
StringComparison.InvariantCultureIgnoreCase)
&& (moduleEditRoles.Equals(this.PortalSettings.AdministratorRoleName,
StringComparison.InvariantCultureIgnoreCase)
|| string.IsNullOrEmpty(moduleEditRoles))
&& pageEditRoles.Equals(this.PortalSettings.AdministratorRoleName,
StringComparison.InvariantCultureIgnoreCase))
{
adminMessage = Localization.GetString("ModuleVisibleAdministrator.Text");
showMessage = !this.ModuleConfiguration.HideAdminBorder
&& !Globals.IsAdminControl();
}
....
}Предупреждение PVS-Studio: V3027 The variable 'moduleEditRoles' was utilized in the logical expression before it was verified against null in the same logical expression. DotNetNuke.Library Container.cs 273
Мда, многовато кода получилось... Давайте ещё немного сократим.
moduleEditRoles.Equals(this.PortalSettings.AdministratorRoleName,
StringComparison.InvariantCultureIgnoreCase)
|| string.IsNullOrEmpty(moduleEditRoles)Вот, теперь другое дело! Кажется, что-то подобное мы сегодня уже видели... Опять сначала проверяют moduleEditRoles на равенство другой строке, и лишь затем идёт проверка, а не является ли moduleEditRoles пустой строкой или null-значением.
Правда, конкретно в текущий момент времени null-значение переменная хранить не может, так как содержит результат вызова метода ToLowerInvariant. Следовательно – максимум она может быть пустой строкой. Уровень предупреждения анализатора здесь можно и понизить.
Тем не менее, я бы поправил код, перенеся проверку IsNullOrEmpty вперёд.
Best warnings. Issue 10
private static void Handle404OrException(....)
{
....
string errRV;
....
if (result != null && result.Action != ActionType.Output404)
{
....
// line 552
errRV = "500 Rewritten to {0} : {1}";
}
else // output 404 error
{
....
// line 593
errRV = "404 Rewritten to {0} : {1} : Reason {2}";
....
}
....
// line 623
response.AppendHeader(errRH,
string.Format(
errRV,
"DNN Tab",
errTab.TabName
+ "(Tabid:" + errTabId.ToString() + ")",
reason));
....
}Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: reason. DotNetNuke.Library AdvancedUrlRewriter.cs 623
False positive. Очевидно, что код написан таким образом специально – нужно править на уровне анализатора.
Summary
Неплохой результат вышел, на мой взгляд! Да, имеем 1 явный false positive, но во всех остальных случаях к коду возникают вопросы.
Однако вы можете попробовать составить свой список лучших предупреждений – ниже я дам материал для этого. :)
Как вы поняли, с предупреждениями мы не закончили. Нет-нет-нет, в проекте нашлось ещё немало мест, заслуживающих внимания и разбора.
Issue 11
В best warnings мы уже видели копипаст then/else-веток оператора if. То место оказалось не единственным:
protected void ExecuteSearch(string searchText, string searchType)
{
....
if (Host.UseFriendlyUrls)
{
this.Response.Redirect(this._navigationManager.NavigateURL(searchTabId));
}
else
{
this.Response.Redirect(this._navigationManager.NavigateURL(searchTabId));
}
....
}Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. DotNetNuke.Website Search.ascx.cs 432
Issues 12, 13
private static void LoadProviders()
{
....
foreach (KeyValuePair<string, SitemapProvider> comp in
ComponentFactory.GetComponents<SitemapProvider>())
{
comp.Value.Name = comp.Key;
comp.Value.Description = comp.Value.Description;
_providers.Add(comp.Value);
}
....
}Предупреждение PVS-Studio: V3005 The 'comp.Value.Description' variable is assigned to itself. DotNetNuke.Library SitemapBuilder.cs 231
Иногда встречаются присваивания, в которых переменная записывается в себя же. Это может быть как избыточный код, так и более серьёзная ошибка, когда что-то перепутали. Что-то мне подсказывает, что приведённый выше код не просто избыточен...
Description – автореализованное свойство:
public string Description { get; set; }Ещё один фрагмент с записью в себя:
public SendTokenizedBulkEmail(List<string> addressedRoles,
List<UserInfo> addressedUsers,
bool removeDuplicates,
string subject,
string body)
{
this.ReportRecipients = true;
this.AddressMethod = AddressMethods.Send_TO;
this.BodyFormat = MailFormat.Text;
this.Priority = MailPriority.Normal;
this._addressedRoles = addressedRoles;
this._addressedUsers = addressedUsers;
this.RemoveDuplicates = removeDuplicates;
this.Subject = subject;
this.Body = body;
this.SuppressTokenReplace = this.SuppressTokenReplace;
this.Initialize();
}Предупреждение PVS-Studio: V3005 The 'this.SuppressTokenReplace' variable is assigned to itself. DotNetNuke.Library SendTokenizedBulkEmail.cs 109
Не такой подозрительный код, как в предыдущем случае, но выглядит всё равно странно – опять свойство записывают само в себя. Соответствующего параметра нет, и какое значение должно быть записано – вопрос. Предположу, что дефолтное, описанное в комментариях (false, то бишь):
/// <summary>Gets or sets a value indicating whether
shall automatic TokenReplace be prohibited?.</summary>
/// <remarks>default value: false.</remarks>
public bool SuppressTokenReplace { get; set; }Issues 14, 15
Помните, мы видели в best warnings, как кто-то забыл про иммутабельность строк? В общем, забыли про неё не один раз. :)
public static string BuildPermissions(IList Permissions, string PermissionKey)
{
....
// get string
string permissionsString = permissionsBuilder.ToString();
// ensure leading delimiter
if (!permissionsString.StartsWith(";"))
{
permissionsString.Insert(0, ";");
}
....
}Предупреждение PVS-Studio: V3010 The return value of function 'Insert' is required to be utilized. DotNetNuke.Library PermissionController.cs 64
Если строка permissionsString не начиналась с ';', то и не будет – Insert не меняет исходную строку, а возвращает изменённую. С учётом вышесказанного отдельного внимания заслуживает комментарий. :)
Ещё одно место:
public override void Install()
{
....
skinFile.Replace(Globals.HostMapPath + "\\", "[G]");
....
}Предупреждение PVS-Studio: V3010 The return value of function 'Replace' is required to be utilized. DotNetNuke.Library SkinInstaller.cs 230
Issue 16
public int Page { get; set; } = 1;
public override IConsoleResultModel Run()
{
....
var pageIndex = (this.Page > 0 ? this.Page - 1 : 0);
pageIndex = pageIndex < 0 ? 0 : pageIndex;
....
}Предупреждение PVS-Studio: V3022 Expression 'pageIndex < 0' is always false. DotNetNuke.Library ListModules.cs 61
На момент вычисления выражения pageIndex < 0 значение pageIndex всегда будет неотрицательным, так как:
Следовательно, проверка pageIndex < 0 будет всегда давать false.
Issue 17
private CacheDependency GetTabsCacheDependency(IEnumerable<int> portalIds)
{
....
// get the portals list dependency
var portalKeys = new List<string>();
if (portalKeys.Count > 0)
{
keys.AddRange(portalKeys);
}
....
}Предупреждение PVS-Studio: V3022 Expression 'portalKeys.Count > 0' is always false. DotNetNuke.Library CacheController.cs 968
Создали пустой список, после чего проверяют, что он непустой. Нет, ну а вдруг? :)
Issue 18
public JournalEntity(string entityXML)
{
....
XmlDocument xDoc = new XmlDocument { XmlResolver = null };
xDoc.LoadXml(entityXML);
if (xDoc != null)
....
}Предупреждение PVS-Studio: V3022 Expression 'xDoc != null' is always true. DotNetNuke.Library JournalEntity.cs 30
Вызвали конструктор, записали ссылку в переменную, после чего вызывают экземплярный метод LoadXml. Далее всё ту же ссылку проверяют на неравенство null. Нет, ну а вдруг? (2)
Issue 19
public enum ActionType
{
....
Redirect302Now = 2,
....
Redirect302 = 5,
....
}
public ActionType Action { get; set; }
private static bool CheckForRedirects(....)
{
....
if ( result.Action != ActionType.Redirect302Now
|| result.Action != ActionType.Redirect302)
....
}Предупреждение PVS-Studio: V3022 Expression is always true. Probably the '&&' operator should be used here. DotNetNuke.Library AdvancedUrlRewriter.cs 1695
Данное выражение будет ложно, только если результат обоих операндов будет false. В таком случае должны выполняться оба следующих условия:
Так как result.Action не может иметь двух разных значений, описанное условие невыполнимо, следовательно выражение всегда истинно.
Issue 20
public Route MapRoute(string moduleFolderName,
string routeName,
string url,
object defaults,
object constraints,
string[] namespaces)
{
if ( namespaces == null
|| namespaces.Length == 0
|| string.IsNullOrEmpty(namespaces[0]))
{
throw new ArgumentException(Localization.GetExceptionMessage(
"ArgumentCannotBeNullOrEmpty",
"The argument '{0}' cannot be null or empty.",
"namespaces"));
}
Requires.NotNullOrEmpty("moduleFolderName", moduleFolderName);
url = url.Trim('/', '\\');
var prefixCounts = this.portalAliasMvcRouteManager.GetRoutePrefixCounts();
Route route = null;
if (url == null)
{
throw new ArgumentNullException(nameof(url));
}
....
}Предупреждение PVS-Studio: V3022 Expression 'url == null' is always false. DotNetNuke.Web.Mvc MvcRoutingManager.cs 66
С параметром url интересная история получается. Мы видим, что, если url – null, разработчики хотят выбросить исключение типа ArgumentNullException, недвусмысленно намекающее на то, что этот параметр должен быть ненулевой ссылкой. Вот только перед этим у url вызывается экземплярный метод Trim... Как следствие, если url – null, будет выброшено исключение типа NullReferenceException. Поздновато проверку сделали, выходит.
Issue 21
public Hashtable Settings
{
get
{
return this.ModuleContext.Settings;
}
}
public string UploadRoles
{
get
{
....
if (Convert.ToString(this.Settings["uploadroles"]) != null)
{
this._UploadRoles = Convert.ToString(this.Settings["uploadroles"]);
}
....
}
}Предупреждение PVS-Studio: V3022 Expression 'Convert.ToString(this.Settings["uploadroles"]) != null' is always true. DotNetNuke.Website.Deprecated WebUpload.ascx.cs 151
Convert.ToString либо возвращает результат успешного преобразования, либо String.Empty, но не null. Как результат, толку от этой проверки нет.
Поверили? Это false positive.
Для начала стоит сказать про перегрузки метода Convert.ToString. Есть такая перегрузка: Convert.ToString(String value). Она просто возвращает value as is. Как следствие, если на вход пришёл null, то и на выходе будет null.
Есть и другая перегрузка – Convert.ToString(Object value). В приведённом выше фрагменте кода как раз используется она. Возвращаемое значение этого метода прокомментировано следующим образом:
// Returns:
// The string representation of value,
// or System.String.Empty if value is null.И здесь можно ошибиться в том, что метод всегда будет возвращать какую-то строку, но! Строковое представление объекта вполне может иметь значение null, и, как следствие, метод всё же вернёт null.
Простейший пример, демонстрирующий это:
Кстати, здесь получается, что:
Оу, немного запутанно выходит...
Можно возразить, что это синтетический пример, и кто же вообще возвращает из метода ToString значение null? Ну, Microsoft порой этим грешил (см. Issue 14 из статьи по ссылке).
Внимание, вопрос! Знали ли авторы кода про эту особенность и учли её или нет? Знали ли про это вы и ожидаете ли, что метод ToString может вернуть null?
Кстати, хочу здесь похвалить nullable reference types и разметку методов .NET, где из сигнатуры метода сразу понятно, что может быть возвращено значение null:
public static string? ToString(object? value)Здесь я предлагаю сделать небольшой перерыв, обновить кофеёк и запастись новой порцией печенек, после чего продолжим. Кофе-брейк!
Надеюсь, вы прислушались к советам и обновили свои запасы. Продолжаем.
Issues 22, 23
public static ModuleItem ConvertToModuleItem(ModuleInfo module)
=> new ModuleItem
{
Id = module.ModuleID,
Title = module.ModuleTitle,
FriendlyName = module.DesktopModule.FriendlyName,
EditContentUrl = GetModuleEditContentUrl(module),
EditSettingUrl = GetModuleEditSettingUrl(module),
IsPortable = module.DesktopModule?.IsPortable,
AllTabs = module.AllTabs,
};Предупреждение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'module.DesktopModule' object Dnn.PersonaBar.Extensions Converters.cs 67
Обратите внимание на инициализацию свойств FriendlyName и IsPortable. В качестве значений для инициализации используются module.DesktopModule.FriendlyName и module.DesktopModule?.IsPortable соответственно. Возникает логичный вопрос – а может ли в данном случае module.DesktopModule иметь значение null? Если да, защита с помощью ?. не сработает, так как при первом обращении будет сгенерировано исключение типа NullReferenceException. Если нет, обращение через ?. избыточно и только сбивает всех с толку.
Очень похожий фрагмент кода встретился ещё раз.
public IDictionary<string, object> GetSettings(MenuItem menuItem)
{
var settings = new Dictionary<string, object>
{
{ "canSeePagesList",
this.securityService.CanViewPageList(menuItem.MenuId) },
{ "portalName",
PortalSettings.Current.PortalName },
{ "currentPagePermissions",
this.securityService.GetCurrentPagePermissions() },
{ "currentPageName",
PortalSettings.Current?.ActiveTab?.TabName },
{ "productSKU",
DotNetNukeContext.Current.Application.SKU },
{ "isAdmin",
this.securityService.IsPageAdminUser() },
{ "currentParentHasChildren",
PortalSettings.Current?.ActiveTab?.HasChildren },
{ "isAdminHostSystemPage",
this.securityService.IsAdminHostSystemPage() },
};
return settings;
}Предупреждение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'PortalSettings.Current' object Dnn.PersonaBar.Extensions PagesMenuController.cs 47
Когда разработчик инициализировал словарь, он несколько раз использовал PortalSettings.Current. Вот только в некоторых случаях при обращении использовалась проверка на null, а в некоторых – нет:
var settings = new Dictionary<string, object>
{
....
{ "portalName",
PortalSettings.Current.PortalName },
....
{ "currentPageName",
PortalSettings.Current?.ActiveTab?.TabName },
....
{ "currentParentHasChildren",
PortalSettings.Current?.ActiveTab?.HasChildren },
....
};Issues 24, 25, 26
private static void HydrateObject(object hydratedObject, IDataReader dr)
{
....
// Get the Data Value's type
objDataType = coloumnValue.GetType();
if (coloumnValue == null || coloumnValue == DBNull.Value)
{
// set property value to Null
objPropertyInfo.SetValue(hydratedObject,
Null.SetNull(objPropertyInfo),
null);
}
....
}Предупреждение PVS-Studio: V3095 The 'coloumnValue' object was used before it was verified against null. Check lines: 902, 903. DotNetNuke.Library CBO.cs 902
Для переменной coloumnValue вызывается экземплярный метод GetType, а дальше следует проверка, что coloumnValue != null. Ребята, вы чего? :( Это же соседние строчки.
Приведённый выше случай, к сожалению, не единичный. Вот ещё один.
private void DeleteLanguage()
{
....
// Attempt to get the Locale
Locale language = LocaleController.Instance
.GetLocale(tempLanguagePack.LanguageID);
if (tempLanguagePack != null)
{
LanguagePackController.DeleteLanguagePack(tempLanguagePack);
}
....
}Предупреждение PVS-Studio: V3095 The 'tempLanguagePack' object was used before it was verified against null. Check lines: 235, 236. DotNetNuke.Library LanguageInstaller.cs 235
Та же самая история – сначала идёт обращение к свойству LanguageId (tempLanguagePack.LanguageID), а на следующей строчке идёт проверка tempLanguagePack != null.
И ещё...
private static void AddLanguageHttpAlias(int portalId, Locale locale)
{
....
var portalAliasInfos = portalAliasses as IList<PortalAliasInfo>
?? portalAliasses.ToList();
if (portalAliasses != null && portalAliasInfos.Any())
....
}Предупреждение PVS-Studio: V3095 The 'portalAliasses' object was used before it was verified against null. Check lines: 1834, 1835. DotNetNuke.Library Localization.cs 1834
Конкретно с этим паттерном закончим, хотя подобные срабатывания ещё есть. Взглянем на то, как ещё может выглядеть обращение к членам перед проверкой на null.
Issues 27, 28, 29, 30
private static void WatcherOnChanged(object sender, FileSystemEventArgs e)
{
if (Logger.IsInfoEnabled && !e.FullPath.EndsWith(".log.resources"))
{
Logger.Info($"Watcher Activity: {e.ChangeType}. Path: {e.FullPath}");
}
if ( _handleShutdowns
&& !_shutdownInprogress
&& (e.FullPath ?? string.Empty)
.StartsWith(_binFolder,
StringComparison.InvariantCultureIgnoreCase))
{
ShceduleShutdown();
}
}Предупреждение PVS-Studio: V3095 The 'e.FullPath' object was used before it was verified against null. Check lines: 147, 152. DotNetNuke.Web DotNetNukeShutdownOverload.cs 147
Обратите внимание на e.FullPath. Сначала идёт обращение e.FullPath.EndsWith(".log.resources"), а позже – использование оператора ??: e.FullPath ?? string.Empty.
Этот код благополучно размножился через copy-paste:
Если честно, мне уже надоело писать про V3095, а вам, думаю, читать. Так что пойдём дальше.
Issue 31
internal FolderInfoBuilder()
{
this.portalId = Constants.CONTENT_ValidPortalId;
this.folderPath = Constants.FOLDER_ValidFolderRelativePath;
this.physicalPath = Constants.FOLDER_ValidFolderPath;
this.folderMappingID = Constants.FOLDER_ValidFolderMappingID;
this.folderId = Constants.FOLDER_ValidFolderId;
this.physicalPath = string.Empty;
}Предупреждение PVS-Studio: V3008 The 'this.physicalPath' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 29, 26. DotNetNuke.Tests.Core FolderInfoBuilder.cs 29
В поле physicalPath сначала записывается значение Constants.FOLDER_ValidFolderPath, однако затем всё в то же поле записывается string.Empty. Дополнительно обращаю внимание, что эти значения отличаются, из-за чего данный код выглядит ещё более подозрительно:
public const string FOLDER_ValidFolderPath = "C:\\folder";Issue 32
public int SeekCountry(int offset, long ipNum, short depth)
{
....
var buffer = new byte[6];
byte y;
....
if (y < 0)
{
y = Convert.ToByte(y + 256);
}
....
}Предупреждение PVS-Studio: V3022 Expression 'y < 0' is always false. Unsigned type value is always >= 0. CountryListBox CountryLookup.cs 210
Значения типа byte лежат в диапазоне [0; 255]. Следовательно, проверка y < 0 всегда будет давать false и then-ветвь никогда не будет выполнена.
Issue 33
private void ParseTemplateInternal(...., string templatePath, ....)
{
....
string path = Path.Combine(templatePath, "admin.template");
if (!File.Exists(path))
{
// if the template is a merged copy of a localized templte the
// admin.template may be one director up
path = Path.Combine(templatePath, "..\admin.template");
}
....
}Предупреждение PVS-Studio: V3057 The 'Combine' function is expected to receive a valid path string. Inspect the second argument. DotNetNuke.Library PortalController.cs 3538
Интересная ошибка. Здесь мы имеем две операции конструирования пути (вызов Path.Combine). С первой всё нормально, а вот вторая интереснее. Видно, что во втором случае хотели брать файл admin.template не из директории templatePath, а из родительской. Но вот незадача! После того как добавили ..\, путь перестал быть валидным, так как образовалась escape sequence: ..\admin.template.
Issue 34
internal override string GetMethodInformation(MethodItem method)
{
....
string param = string.Empty;
string[] names = method.Parameters;
StringBuilder sb = new StringBuilder();
if (names != null && names.GetUpperBound(0) > 0)
{
for (int i = 0; i <= names.GetUpperBound(0); i++)
{
sb.AppendFormat("{0}, ", names[i]);
}
}
if (sb.Length > 0)
{
sb.Remove(sb.Length - 2, 2);
param = sb.ToString();
}
....
}Предупреждение PVS-Studio: V3057 The 'Remove' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. DotNetNuke.Log4Net StackTraceDetailPatternConverter.cs 67
Сейчас этот код работает, но всё равно сразу закрадываются подозрения. В then-ветви оператора if значение sb.Length >= 1, но при вызове метода Remove мы вычитаем из этого значения 2. Соответственно, если sb.Length == 1, то вызов будет выглядеть следующим образом: sb.Remove(-1, 2), что приведёт к возникновению исключения.
Конкретно сейчас этот код спасает то, что в StringBuilder добавляются строки через формат "{0}, ". Следовательно, длина этих строк – как минимум 2 символа. Тем не менее, всё это выглядит достаточно зыбко, а проверка в таком виде только дополнительно сбивает с толку и настораживает.
Issues 35, 36
public void SaveJournalItem(JournalItem journalItem, int tabId, int moduleId)
{
....
journalItem.JournalId = this._dataService.Journal_Save(
journalItem.PortalId,
journalItem.UserId,
journalItem.ProfileId,
journalItem.SocialGroupId,
journalItem.JournalId,
journalItem.JournalTypeId,
journalItem.Title,
journalItem.Summary,
journalItem.Body,
journalData,
xml,
journalItem.ObjectKey,
journalItem.AccessKey,
journalItem.SecuritySet,
journalItem.CommentsDisabled,
journalItem.CommentsHidden);
....
}
public void UpdateJournalItem(JournalItem journalItem, int tabId, int moduleId)
{
....
journalItem.JournalId = this._dataService.Journal_Update(
journalItem.PortalId,
journalItem.UserId,
journalItem.ProfileId,
journalItem.SocialGroupId,
journalItem.JournalId,
journalItem.JournalTypeId,
journalItem.Title,
journalItem.Summary,
journalItem.Body,
journalData,
xml,
journalItem.ObjectKey,
journalItem.AccessKey,
journalItem.SecuritySet,
journalItem.CommentsDisabled,
journalItem.CommentsHidden);
....
}Тут сразу 2 проблемы, которые, похоже, размножились методом copy-paste. Сможете найти их? Далее привожу картинку для отвлечения внимания, чтобы ответ не бросился в глаза.
Ох, прошу прощения! Кажется, я забыл дать вам ключик для разгадки... Вот он:
int Journal_Update(int portalId,
int currentUserId,
int profileId,
int groupId,
int journalId,
int journalTypeId,
string title,
string summary,
string body,
string itemData,
string xml,
string objectKey,
Guid accessKey,
string securitySet,
bool commentsHidden,
bool commentsDisabled);Теперь должно стать немного легче. Нашли проблему? Если нет (или просто ленно это делать), предупреждения анализатора помогут:
Обратите внимание на последние параметры и аргументы – в обоих вызовах аргументы идут в таком порядке: journalItem.CommentsDisabled, journalItem.CommentsHidden; в то время как параметры перечислены так: commentsHidden, commentsDisabled. Выглядит достаточно подозрительно, не правда ли?
Issue 37
private static DateTime LastPurge
{
get
{
var lastPurge = DateTime.Now;
if (File.Exists(CachePath + "_lastpurge"))
{
var fi = new FileInfo(CachePath + "_lastpurge");
lastPurge = fi.LastWriteTime;
}
else
{
File.WriteAllText(CachePath + "_lastpurge", string.Empty);
}
return lastPurge;
}
set
{
File.WriteAllText(CachePath + "_lastpurge", string.Empty);
}
}Предупреждение PVS-Studio: V3077 The setter of 'LastPurge' property does not utilize its 'value' parameter. DotNetNuke.Library IPCount.cs 96
Здесь подозрительным выглядит то, что set accessor никак не использует value-параметр. То есть в это свойство могут что-то записывать, и записываемое значение просто... игнорируется. В коде я нашёл одно место, где идёт запись в это свойство:
public static bool CheckIp(string ipAddress)
{
....
LastPurge = DateTime.Now;
....
}Соответственно, текущее время нигде зафиксировано не будет. Да, можно сказать, что косвенно оно будет зафиксировано в файле, который будет записан в этот момент, но... Представьте, что вместо DateTime.Now в свойство записывается какая-то другая дата (ведь ограничений в set accessor на это нет).
Issue 38
private void DisplayNewRows()
{
this.divTabName.Visible = this.optMode.SelectedIndex == 0;
this.divParentTab.Visible = this.optMode.SelectedIndex == 0;
this.divInsertPositionRow.Visible = this.optMode.SelectedIndex == 0;
this.divInsertPositionRow.Visible = this.optMode.SelectedIndex == 0;
}Предупреждение PVS-Studio: V3008 The 'this.divInsertPositionRow.Visible' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 349, 348. DotNetNuke.Website Import.ascx.cs 349
Здесь не просто дважды идёт запись в одну переменную – тут целое выражение дублируется. Возможно, оно просто избыточно. Но есть вероятность, что его скопировали, а поменять забыли. Эффект последней строки?
Issue 39
public enum AddressType
{
IPv4 = 0,
IPv6 = 1,
}
private static void FilterRequest(object sender, EventArgs e)
{
....
switch (varArray[1])
{
case "IPv4":
varVal = NetworkUtils.GetAddress(varVal, AddressType.IPv4);
break;
case "IPv6":
varVal = NetworkUtils.GetAddress(varVal, AddressType.IPv4);
break;
}
....
}Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. DotNetNuke.HttpModules RequestFilterModule.cs 81
Что-то мне подсказывает, что не должны эти case-ветки быть одинаковыми и что во втором случае стоит использовать AddressType.IPv6.
Issue 40
private static DateTime CalculateTime(int lapse, string measurement)
{
var nextTime = new DateTime();
switch (measurement)
{
case "s":
nextTime = DateTime.Now.AddSeconds(lapse);
break;
case "m":
nextTime = DateTime.Now.AddMinutes(lapse);
break;
case "h":
nextTime = DateTime.Now.AddHours(lapse);
break;
case "d":
nextTime = DateTime.Now.AddDays(lapse);
break;
case "w":
nextTime = DateTime.Now.AddDays(lapse);
break;
case "mo":
nextTime = DateTime.Now.AddMonths(lapse);
break;
case "y":
nextTime = DateTime.Now.AddYears(lapse);
break;
}
return nextTime;
}Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. DotNetNuke.Tests.Core PropertyAccessTests.cs 118
Обратите внимание на тела case-ветвей "d" и "w" – они дублируют друг друга. Copy-paste... Copy-paste никогда не меняется. Стандартного метода AddWeeks в типе DateTime нет, но очевидно, что в case-ветке "w" работа должна идти с неделями.
Issue 41
private static int AddTabToTabDict(....)
{
....
if (customAliasUsedAndNotCurrent && settings.RedirectUnfriendly)
{
// add in the standard page, but it's a redirect to the customAlias
rewritePath = RedirectTokens.AddRedirectReasonToRewritePath(
rewritePath,
ActionType.Redirect301,
RedirectReason.Custom_Tab_Alias);
AddToTabDict(tabIndex,
dupCheck,
httpAlias,
tabPath,
rewritePath,
tab.TabID,
UrlEnums.TabKeyPreference.TabRedirected,
ref tabPathDepth,
settings.CheckForDuplicateUrls,
isDeleted);
}
else
{
if (customAliasUsedAndNotCurrent && settings.RedirectUnfriendly)
{
// add in the standard page, but it's a redirect to the customAlias
rewritePath = RedirectTokens.AddRedirectReasonToRewritePath(
rewritePath,
ActionType.Redirect301,
RedirectReason.Custom_Tab_Alias);
AddToTabDict(tabIndex,
dupCheck,
httpAlias,
tabPath,
rewritePath,
tab.TabID,
UrlEnums.TabKeyPreference.TabRedirected,
ref tabPathDepth,
settings.CheckForDuplicateUrls,
isDeleted);
}
else
....
}
....
}Предупреждение PVS-Studio: V3030 Recurring check. The 'customAliasUsedAndNotCurrent && settings.RedirectUnfriendly' condition was already verified in line 1095. DotNetNuke.Library TabIndexController.cs 1097
Анализатор предупреждает о том, что обнаружен паттерн следующего вида:
if (a && b)
....
else
{
if (a && b)
....
}Соответственно, в этом фрагменте кода второе условие будет ложным – переменные не изменялись между вызовами.
Но здесь мы наткнулись даже на бОльший куш – дублируются не только условия, но и целые блоки кода. Посмотрите, if с его then-ветвью был скопирован целиком.
Issue 42
private IEnumerable<TabDto> GetDescendantsForTabs(
IEnumerable<int> tabIds,
IEnumerable<TabDto> tabs,
int selectedTabId,
int portalId,
string cultureCode,
bool isMultiLanguage)
{
var enumerable = tabIds as int[] ?? tabIds.ToArray();
if (tabs == null || tabIds == null || !enumerable.Any())
{
return tabs;
}
....
}Предупреждение PVS-Studio: V3095 The 'tabIds' object was used before it was verified against null. Check lines: 356, 357. Dnn.PersonaBar.Library TabsController.cs 356
Подобный кейс я приводил раньше, но решил повторить и разобрать более подробно.
tabIds – параметр. Скорее всего, ожидается, что он может иметь значение null – не зря же есть проверка tabIds == null? Вот только здесь опять что-то напутано...
Представим, что tabIds – null, тогда:
Выходит, проверка не сработала.
Issue 43
Предлагаю снова поразмяться и найти ошибку самостоятельно. Задача очень сильно упрощена – ниже приведён сокращённый метод, отрезано почти всё лишнее! Оригинальный же метод был на 500 строк – вот в нём поискать проблему было бы настоящим хардкором. Хотя если есть желание – вот ссылка на него на GitHub.
Уверен, если сами поймёте, что не так, получите выброс эндорфинов. :)
private void SaveModuleSettings()
{
....
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.SKIN}",
this.ddlSkin.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.CODEMIRRORTHEME}",
this.CodeMirrorTheme.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.BROWSER}",
this.ddlBrowser.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.IMAGEBUTTON}",
this.ddlImageButton.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.FILELISTVIEWMODE}",
this.FileListViewMode.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.DEFAULTLINKMODE}",
this.DefaultLinkMode.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.USEANCHORSELECTOR}",
this.UseAnchorSelector.Checked.ToString());
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.SHOWPAGELINKSTABFIRST}",
this.ShowPageLinksTabFirst.Checked.ToString());
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.OVERRIDEFILEONUPLOAD}",
this.OverrideFileOnUpload.Checked.ToString());
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.SUBDIRS}",
this.cbBrowserDirs.Checked.ToString());
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.BROWSERROOTDIRID}",
this.BrowserRootDir.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.UPLOADDIRID}",
this.UploadDir.SelectedValue);
if (Utility.IsNumeric(this.FileListPageSize.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.FILELISTPAGESIZE}",
this.FileListPageSize.Text);
}
if (Utility.IsNumeric(this.txtResizeWidth.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.RESIZEWIDTH}",
this.txtResizeWidth.Text);
}
if (Utility.IsNumeric(this.txtResizeHeight.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.RESIZEHEIGHT}",
this.txtResizeHeight.Text);
}
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.INJECTJS}",
this.InjectSyntaxJs.Checked.ToString());
if (Utility.IsUnit(this.txtWidth.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.WIDTH}",
this.txtWidth.Text);
}
if (Utility.IsUnit(this.txtHeight.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.HEIGHT}",
this.txtWidth.Text);
}
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.BLANKTEXT}",
this.txtBlanktext.Text);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.CSS}",
this.CssUrl.Url);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.TEMPLATEFILES}",
this.TemplUrl.Url);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.CUSTOMJSFILE}",
this.CustomJsFile.Url);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.CONFIG}",
this.ConfigUrl.Url);
....
}В таких случаях без картинки для отвлечения внимания не обойтись. За ней будет ответ.
Что ж, время проверить себя!
Предупреждение PVS-Studio: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'txtHeight' variable should be used instead of 'txtWidth' DNNConnect.CKEditorProvider CKEditorOptions.ascx.cs 2477
Внимательности анализатору не занимать, конечно. Давайте ещё немного сократим код, чтобы ошибка совсем уж оказалась на поверхности.
private void SaveModuleSettings()
{
....
if (Utility.IsUnit(this.txtWidth.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.WIDTH}",
this.txtWidth.Text); // <=
}
if (Utility.IsUnit(this.txtHeight.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.HEIGHT}",
this.txtWidth.Text); // <=
}
....
}Обратите внимание, что во втором случае мы уже обрабатываем переменные с 'height' в названии, а не 'width'. Тем не менее, при вызове метода UpdateModuleSetting последним аргументом передаётся this.txtWidth.Text вместо this.txtHeight.Text.
Issue N
Нет, конечно, это не все предупреждения, которые нашёл анализатор в проекте. Я постарался отобрать наиболее интересные и ёмкие. Были и межпроцедурные, были похожие на те, которые приводил. Но я исхожу из того, что, скорее, они будут интересны разработчикам проекта, нежели читателям.
Без false positives тоже не обошлось, но они уже интересны скорее разработчикам анализатора, нежели читателям этой статьи, так что их я также оставил за рамками.
По-моему, сегодняшняя подборка выдалась достаточно разнообразной. По поводу некоторых ошибок у вас мог возникнуть вопрос: "Как вообще можно было допустить такой косяк?". Но можно, можно! Как? Вопрос открытый, причины разные бывают. Ошибаются ли? Конечно. И мы из раза в раз находим тому подтверждения.
Да мы и сами ошибаемся иногда, чего уж тут. И false positives бывают. Важно признавать проблемы и работать над их исправлением. :)
Говоря про качество кода, достаточно ли одной экспертизы разработчиков? Скорее нет. Нужно подходить к проблеме комплексно и использовать разные инструменты/методики как контроля качества кода в частности, так и качества продукта в целом.
Выводы сегодня такие:
P.S. Кстати, а какой ваш Топ-10 предупреждений из этой статьи? ;)
0
0
0
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0