Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
GBP
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Разнообразие ошибок в C# коде на пример…

Разнообразие ошибок в C# коде на примере CMS DotNetNuke: 40 вопросов к качеству

24 Ноя 2021

Сегодня мы вновь говорим о качестве C# кода и разнообразии возможных ошибок. На нашем операционном столе – CMS DotNetNuke, в исходный код которой мы и залезем. И лучше сразу заварите себе кофе...

0890_DNN_ru/image1.png

DotNetNuke

DotNetNuke – это система управления контентом (CMS) с открытым исходным кодом, преимущественно написанная на C#. Исходный код проекта доступен на GitHub. Также стоит отметить, что проект является частью .NET Foundation.

0890_DNN_ru/image2.png

У проекта есть свой сайт, Twitter, YouTube-канал.

Однако для меня остался открытым вопрос, насколько проект живой? Если судить по репозиторию GitHub – в коде делаются какие-то изменения, проходят релизы. Если посмотреть на Twitter или YouTube канал – они достаточно давно не обновлялись.

С другой стороны – есть даже отдельный сайт сообщества, там же есть новости про какие-то ивенты.

Но, так или иначе, нас в первую очередь интересует код. Код и его качество.

Кстати, на странице проекта (ниже скриншот с неё) указано, что качество кода контролируется с помощью статического анализатора NDepend.

0890_DNN_ru/image3.png

Я не могу сказать, как настроен этот анализатор на проекте, обрабатываются ли его предупреждения и так далее, но хочу напомнить, что статический анализатор должен использоваться регулярно. По-хорошему он должен быть тесно вплетён в ваш процесс разработки. Про это написано множество статей, найти которые можно, в том числе, в нашем блоге.

О проверке

Для проверки проекта я использовал исходники с GitHub от 22.10.2021. Прошу учитывать, что от момента написания до момента публикации / чтений этой статьи пройдёт определённое время, за которое код может несколько измениться.

Для проверки использовался статический анализатор PVS-Studio версии 7.15. Если хотите попробовать анализатор на своём проекте – переходите на эту страничку, которая проведёт вас по всем необходимым шагам. Появились вопросы или что-то непонятно? Смело пишите нам.

А начать мне сегодня хочется с одного из нововведений релиза PVS-Studio 7.15 – списка лучших предупреждений. Фича новая, и в будущем ещё будет улучшаться, но пользоваться можно (и нужно!) уже сейчас.

Best warnings

Допустим, вы решили попробовать какой-нибудь статический анализатор на своём проекте. Загрузили его, проанализировали проект и... получили кучу предупреждений. Десятки, сотни, тысячи, может, даже десятки тысяч. Ого, вот это номер... Хотелось бы каким-нибудь магическим образом отобрать из них, скажем, Топ-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. Как следствие, если settingsnull и isSuperTabtrue, получим исключение типа NullReferenceException.

Что интересно, в данном фрагменте кода есть и второй контракт, связывающий параметры isSuperTab и settings – тернарный оператор: isSuperTab ? Null.NullInteger : settings.PortalId. Но обратите внимание, что в этом случае, в отличие от if, settings.PortalId будет использоваться тогда, когда isSuperTab имеет значение false.

И тут можно заметить, что, если isSuperTabtrue, значение 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

Здесь мы опять сталкиваемся сразу с несколькими интересными моментами:

  • разработчик хотел удалить подстроку "Info" из tableName, но забыл про иммутабельность строк в C#. Никакой замены в tableName не произойдёт, а изменённая строка просто будет потеряна, так как результат вызова метода Replace никуда не сохраняется;
  • в коде объявлена переменная tableName, инициализированная пустой строкой. Сразу после этого идёт проверка, является ли tableName пустой строкой.

На первый кейс и указывает приведённое предупреждение анализатора. Второй кейс, кстати, также был обнаружен анализатором, но предупреждение не попало в список 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. Вот только если strExtensionsnull, уже при вызове экземплярного метода 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"). Или этот код просто избыточен, или здесь затерялась какая-то нужная проверка.

На самом деле, здесь есть и другие дубли, о которых анализатор почему-то не сообщил (взяли на карандаш). Например, также по два раза встречаются следующие выражения:

  • !info.Name.Equals("FileBrowserWindowWidth")
  • !info.Name.Equals("FileBrowserWindowHeight")

Три дублирующиеся проверки в рамках одного выражения – неплохо. На моей памяти это, вроде как, рекорд.

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 всегда будет неотрицательным, так как:

  • если this.Page лежит в диапазоне [1; int.MaxValue], pageIndex будет иметь значение в диапазоне [0; int.MaxValue - 1]
  • если this.Page лежит в диапазоне [int.MinValue; 0], pageIndex будет иметь значение 0.

Следовательно, проверка 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 == ActionType.Redirect302Now
  • result.Action == ActionType.Redirect302

Так как 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 интересная история получается. Мы видим, что, если urlnull, разработчики хотят выбросить исключение типа ArgumentNullException, недвусмысленно намекающее на то, что этот параметр должен быть ненулевой ссылкой. Вот только перед этим у url вызывается экземплярный метод Trim... Как следствие, если urlnull, будет выброшено исключение типа 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.

Простейший пример, демонстрирующий это:

0890_DNN_ru/image4.png

Кстати, здесь получается, что:

  • если obj == null, stringRepresentation != null (пустая строка);
  • если obj != null, stringRepresentation == null.

Оу, немного запутанно выходит...

Можно возразить, что это синтетический пример, и кто же вообще возвращает из метода ToString значение null? Ну, Microsoft порой этим грешил (см. Issue 14 из статьи по ссылке).

Внимание, вопрос! Знали ли авторы кода про эту особенность и учли её или нет? Знали ли про это вы и ожидаете ли, что метод ToString может вернуть null?

Кстати, хочу здесь похвалить nullable reference types и разметку методов .NET, где из сигнатуры метода сразу понятно, что может быть возвращено значение null:

public static string? ToString(object? value)

Здесь я предлагаю сделать небольшой перерыв, обновить кофеёк и запастись новой порцией печенек, после чего продолжим. Кофе-брейк!

0890_DNN_ru/image5.png

Надеюсь, вы прислушались к советам и обновили свои запасы. Продолжаем.

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 The 'e.FullPath' object was used before it was verified against null. Check lines: 160, 165. DotNetNuke.Web DotNetNukeShutdownOverload.cs 160
  • V3095 The 'e.FullPath' object was used before it was verified against null. Check lines: 173, 178. DotNetNuke.Web DotNetNukeShutdownOverload.cs 173
  • V3095 The 'e.FullPath' object was used before it was verified against null. Check lines: 186, 191. DotNetNuke.Web DotNetNukeShutdownOverload.cs 186

Если честно, мне уже надоело писать про 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. Сможете найти их? Далее привожу картинку для отвлечения внимания, чтобы ответ не бросился в глаза.

0890_DNN_ru/image6.png

Ох, прошу прощения! Кажется, я забыл дать вам ключик для разгадки... Вот он:

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);

Теперь должно стать немного легче. Нашли проблему? Если нет (или просто ленно это делать), предупреждения анализатора помогут:

  • V3066 Possible incorrect order of arguments passed to 'Journal_Save' method: 'journalItem.CommentsDisabled' and 'journalItem.CommentsHidden'. DotNetNuke.Library JournalControllerImpl.cs 125
  • V3066 Possible incorrect order of arguments passed to 'Journal_Update' method: 'journalItem.CommentsDisabled' and 'journalItem.CommentsHidden'. DotNetNuke.Library JournalControllerImpl.cs 253

Обратите внимание на последние параметры и аргументы – в обоих вызовах аргументы идут в таком порядке: 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? Вот только здесь опять что-то напутано...

Представим, что tabIdsnull, тогда:

  • вычисляется левый операнд оператора ?? (tabIds as int[]);
  • tabIds as int[] даёт в результате null;
  • вычисляется правый операнд оператора ?? (tabIds.ToArray());
  • исключение при попытке вызова метода ToArray, так как tabIdsnull.

Выходит, проверка не сработала.

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);
  ....
}

В таких случаях без картинки для отвлечения внимания не обойтись. За ней будет ответ.

0890_DNN_ru/image7.png

Что ж, время проверить себя!

Предупреждение 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 предупреждений из этой статьи? ;)

Популярные статьи по теме
Как и почему статические анализаторы борются с ложными срабатываниями

Дата: 20 Мар 2017

Автор: Андрей Карпов

В своей предыдущей статье я писал, что мне не нравится подход, при котором статические анализаторы кода оцениваются с помощью синтетических тестов. В статье приводился пример, воспринимаемый анализат…
Бесплатный PVS-Studio для тех, кто развивает открытые проекты

Дата: 22 Дек 2018

Автор: Андрей Карпов

В канун празднования нового 2019 года команда PVS-Studio решила сделать приятный подарок всем контрибьюторам open-source проектов, хостящихся на GitHub, GitLab или Bitbucket. Им предоставляется возмо…
Зло живёт в функциях сравнения

Дата: 19 Май 2017

Автор: Андрей Карпов

Возможно, читатели помнят мою статью под названием "Эффект последней строки". В ней идёт речь о замеченной мной закономерности: ошибка чаще всего допускается в последней строке однотипных блоков текс…
PVS-Studio ROI

Дата: 30 Янв 2019

Автор: Андрей Карпов

Время от времени нам задают вопрос, какую пользу в денежном эквиваленте получит компания от использования анализатора PVS-Studio. Мы решили оформить ответ в виде статьи и привести таблицы, которые по…
Статический анализ как часть процесса разработки Unreal Engine

Дата: 27 Июн 2017

Автор: Андрей Карпов

Проект Unreal Engine развивается - добавляется новый код и изменятся уже написанный. Неизбежное следствие развития проекта - появление в коде новых ошибок, которые желательно выявлять как можно раньш…
Любите статический анализ кода!

Дата: 16 Окт 2017

Автор: Андрей Карпов

Я в шоке от возможностей статического анализа кода, хотя сам участвую в разработке инструмента PVS-Studio. На днях я был искренне удивлён тому, что анализатор оказался умнее и внимательнее меня.
Главный вопрос программирования, рефакторинга и всего такого

Дата: 14 Апр 2016

Автор: Андрей Карпов

Вы угадали, ответ - "42". Здесь приводится 42 рекомендации по программированию, которые помогут избежать множества ошибок, сэкономить время и нервы. Автором рекомендаций выступает Андрей Карпов - тех…
PVS-Studio для Java

Дата: 17 Янв 2019

Автор: Андрей Карпов

В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальн…
Эффект последней строки

Дата: 31 Май 2014

Автор: Андрей Карпов

Я изучил множество ошибок, возникающих в результате копирования кода. И утверждаю, что чаще всего ошибки допускают в последнем фрагменте однотипного кода. Ранее я не встречал в книгах описания этого …
Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей

Дата: 21 Ноя 2018

Автор: Андрей Карпов

Краткое описание технологий, используемых в инструменте PVS-Studio, которые позволяют эффективно обнаруживать большое количество паттернов ошибок и потенциальных уязвимостей. Статья описывает реализа…

Комментарии (0)

Следующие комментарии
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять