0890_DNN/image1.png

Today, we discuss C# code quality and a variety of errors by the example of CMS DotNetNuke. We're going to dig into its source code. You're going to need a cup of coffee...

DotNetNuke

DotNetNuke is an open-source content management system (CMS) written mainly in C#. The source code is available on GitHub. The project is part of the .NET Foundation.

0890_DNN/image2.png

The project has its website, Twitter, YouTube channel.

However, I still don't understand the project status. The GitHub repository is updated from time to time. They have new releases. Though, it's been a while since they post something on Twitter or YouTube channel.

At the same time, they have a community website where you can find information about some events.

Anyway, we are interested in the code especially. The code and its quality.

By the way, the project web page (see a screenshot below) shows that the developers use the NDepend static analyzer to monitor code quality.

0890_DNN/image3.png

I don't know how the project developers configured the analyzer, whether warnings are handled, and so on. But I'd like to remind you that it's better to use static analysis tools regularly in your development process. You can find lots of articles on this topic – visit our blog to read some.

About the check

To check the project, I used the source code from GitHub from the 22nd of October 2021. Take into account that we published / you read this article after a while. The code may be different by now.

I use PVS-Studio 7.15 to perform the analysis. Want to try the analyzer on your project? Click here to open the page with all the necessary steps. Have any questions? Don't understand something? Feel free to contact us.

Today, I'd like to start with one of the new features of PVS-Studio 7.15 – the best warnings list. The feature is brand-new, and we will enhance it in the future. However, you can (and should) use it right now.

Best warnings

Let's say you decide to try a static analyzer on your project. You downloaded it, analyzed the project, and… got a bunch of warnings. Tens, hundreds, thousands, maybe even tens of thousands. Wow, "cool"… It would be great to magically select, for example, the Top 10 most interesting warnings. Enough to look at and think: "Yeah, that code is rubbish, definitely!". Well, now PVS-Studio has such a mechanism. It is called best warnings.

So far, you can use the feature only in the PVS-Studio plugin for Visual Studio. But we plan to add the best warnings to other IDE plugins later. With the best warnings mechanism, the analyzer selects the most interesting and plausible warnings from the log.

Ready to see the best warnings list for the DNN project?

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

The PVS-Studio warning: V3095 The 'settings' object was used before it was verified against null. Check lines: 190, 195. DotNetNuke.Library NavigationManager.cs 190

I wonder why at first we access the settings.PortalId instance property, and then we check settings for null inequality. Thus, if settings – null and isSuperTab – true, we get NullReferenceException.

Surprisingly, this code fragment has a second contract that links isSuperTab and settings parameters – the ternary operator: isSuperTab? Null.NullInteger: settings.PortalId. Note that in this case, unlike if, settings.PortalId is used when isSuperTab is false.

If isSuperTab is true, the settings.PortalId value is not processed. You may think that it's just an implicit contract, and everything is fine.

Nope.

The code must be easy to read and understandable – you don't have to think like Sherlock. If you intend to create this contract, write it explicitly in the code. Thus, the developers, the static analyzer, and you will not be confused. ;)

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

The PVS-Studio warning: V3010 The return value of function 'Replace' is required to be utilized. DotNetNuke.Library CBO.cs 1038

Here we have several curious cases:

the developers wanted to remove the "Info" substring from tableName but forgot that C# strings are immutable. tableName remains the same. The replaced string is lost, since the result of the Replace method call is not stored anywhere;
the tableName variable initialized with an empty string is declared in the code. Right after, the developers check whether tableName is an empty string.

The analyzer issues the warning for the first case. By the way, the analyzer also detects the second case. However, the best warnings list does not include this warning. Here it is: 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));
  }
  ....
}

The PVS-Studio warning: 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

In the strExtensions string, the developers try to find the strExtension substring. If the substring is not found, they check whether strExtensions is empty or null. But if strExtensions is null, the IndexOf call leads to NullReferenceException.

If strExtension is implied to be an empty string but never has a null value, we can more explicitly express the intentions: strExtensions.Length == 0.

In any case, it's better to fix this code fragment because it raises questions – as in 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);
  ....
}

The PVS-Studio warning: 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

Suspicious operations with formatted strings – the value of the seconds variable is substituted into the resulting string. But there was no place for Globals.ApplicationPath due to the absence of {0} in the format string.

Best warnings. Issue 5

private void ProcessRequest(....)
{
  ....
  if (!result.RewritePath.ToLowerInvariant().Contains("tabId="))
  ....
}

The PVS-Studio warning: V3122 The 'result.RewritePath.ToLowerInvariant()' lowercase string is compared with the '"tabId="' mixed case string. DotNetNuke.Library AdvancedUrlRewriter.cs 2252

Guess I've never seen warnings of this diagnostic in projects. Well, a first time for everything. :)

The developers lowercase the string from RewritePath and check whether it has the "tabId=" substring. But there's a problem – the source string is lowercased, but the string that they check contains uppercase characters.

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

The PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. DotNetNuke.Library DNNListEditControl.cs 380

Classic copy-paste: then and else branches of the if statement are identical.

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

The PVS-Studio warning: V3122 The 'path.ToLowerInvariant()' lowercase string is compared with the 'Localization.LocalResourceDirectory' mixed case string. Dnn.PersonaBar.Extensions LanguagesController.cs 644

Here we go again. But this time, the error is less obvious. The developers convert the path value to lowercase. Then, they check whether it ends in a string that contains uppercase characters – "App_LocalResources" (the literal returned from the LocalResourceDirectory property).

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

The PVS-Studio warning: 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

I've formatted this code to make it clearer. The analyzer detected a suspicious duplicate of checks: !info.Name.Equals("Templates_Files"). Perhaps this code is redundant. Or some necessary check got lost here.

In fact, we also have other duplicates here. For some reason, the analyzer did not report about them (we'll check it later). Also, the following expressions occur twice:

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

Three duplicate checks within the same expression – not bad. I guess that's a record!

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

The PVS-Studio warning: 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

Hmm, too much code… Let's reduce it.

   moduleEditRoles.Equals(this.PortalSettings.AdministratorRoleName, 
                          StringComparison.InvariantCultureIgnoreCase)
|| string.IsNullOrEmpty(moduleEditRoles)

So much better now! I guess we've already discussed something similar today… Again, at first, the developers check whether moduleEditRoles equals another string. Then they check whether moduleEditRoles is an empty string or a null value.

However, at this stage, the variable cannot store a null value because it contains the result of the ToLowerInvariant method. Therefore, it can be an empty string at most. We could lower the warning level of the analyzer here.

Though, I would fix the code by moving the IsNullOrEmpty check in the beginning.

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

The PVS-Studio warning: 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. Obviously, the programmer intended to write the code this way. So, we must fix this at the analyzer level.

Summary

Not bad, I guess! Yeah, we have 1 false positive. But other issues in the code must be fixed.

However, you are free to create your list of the best warnings. For that, I describe other warnings below. :)

Other warnings

As you see, that's not all we have today! The analyzer found lots of worthy cases to consider.

Issue 11

In the best warnings section, we've already discussed a copy-paste of then/else branches of the if statement. Unfortunately, this is not the only place:

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

The PVS-Studio warning: 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);
  }
  ....
}

The PVS-Studio warning: V3005 The 'comp.Value.Description' variable is assigned to itself. DotNetNuke.Library SitemapBuilder.cs 231

Sometimes you can encounter the code where a variable is assigned to itself. This code can be redundant or may contain a more serious error – perhaps the developers mixed something up. I guess the above code fragment is exactly the case.

Description is an auto-implemented property:

public string Description { get; set; }

Here's one more fragment that contains the variable assigned to itself:

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

The PVS-Studio warning: V3005 The 'this.SuppressTokenReplace' variable is assigned to itself. DotNetNuke.Library SendTokenizedBulkEmail.cs 109

This code is not as suspicious as the previous one but still looks strange. The SuppressTokenReplace property is assigned to itself. The corresponding parameter is absent. I don't know what value must be assigned. Maybe the default value described in the comments (that is, 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

In the best warnings section, we discussed that the developers forgot about strings' immutability. Well, they forgot about it more than once. :)

public static string BuildPermissions(IList Permissions, string PermissionKey)
{
  ....
  // get string
  string permissionsString = permissionsBuilder.ToString();

  // ensure leading delimiter
  if (!permissionsString.StartsWith(";"))
  {
    permissionsString.Insert(0, ";");
  }

  ....
}

The PVS-Studio warning: V3010 The return value of function 'Insert' is required to be utilized. DotNetNuke.Library PermissionController.cs 64

If permissionsString does not start with ';', the developers want to fix this by adding ';' in the beginning. However, Insert does not change the source string, it returns the modified one.

Another case:

public override void Install()
{
  ....
  skinFile.Replace(Globals.HostMapPath + "\\", "[G]");
  ....
}

The PVS-Studio warning: 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;
  ....
}

The PVS-Studio warning: V3022 Expression 'pageIndex < 0' is always false. DotNetNuke.Library ListModules.cs 61

When the pageIndex < 0 expression is evaluated, the pageIndex value will be always non-negative, since:

if this.Page is in the [1; int.MaxValue] range, pageIndex will be in the [0; int.MaxValue — 1] range
if this.Page is in the [int.MinValue; 0] range, pageIndex will have the 0 value.

Therefore, the pageIndex < 0 check will always be 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);
  }
  ....
}

The PVS-Studio warning: V3022 Expression 'portalKeys.Count > 0' is always false. DotNetNuke.Library CacheController.cs 968

The developers created an empty list and then checked that it is non-empty. Just in case :)

Issue 18

public JournalEntity(string entityXML)
{
  ....
  XmlDocument xDoc = new XmlDocument { XmlResolver = null };
  xDoc.LoadXml(entityXML);
  if (xDoc != null)
  ....
}

The PVS-Studio warning: V3022 Expression 'xDoc != null' is always true. DotNetNuke.Library JournalEntity.cs 30

Called the constructor, wrote the reference to a variable. After that, called the LoadXml instance method. Then, the developers check the same link for null inequality. Just in case. (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)
  ....
}

The PVS-Studio warning: V3022 Expression is always true. Probably the '&&' operator should be used here. DotNetNuke.Library AdvancedUrlRewriter.cs 1695

This expression will be false only if the result of both operands is false. In this case, the following conditions must be met:

result.Action == ActionType.Redirect302Now
result.Action == ActionType.Redirect302

Since result.Action cannot have two different values, the described condition is impossible. Therefore, the expression is always true.

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

The PVS-Studio warning: V3022 Expression 'url == null' is always false. DotNetNuke.Web.Mvc MvcRoutingManager.cs 66

What a curious case we have with the url parameter. If url is null, the developers want to throw ArgumentNullException. The exception unambiguously hints that this parameter should be non-null. But before this, for url, the developers call an instance method – Trim… As a result, if url is null, NullReferenceException is thrown.

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

The PVS-Studio warning: V3022 Expression 'Convert.ToString(this.Settings["uploadroles"]) != null' is always true. DotNetNuke.Website.Deprecated WebUpload.ascx.cs 151

Convert.ToString may return the result of a successful converting or String.Empty, but not null. Eventually, this check makes no sense.

Believed it? This is false positive.

Let's start with the Convert.ToString method overloading: Convert.ToString(String value). It returns value as is. Thus, if the input is null, the outputis also null.

The above code snippet contains another overloading – Convert.ToString(Object value). The return value of this method has the following comment:

// Returns:
//     The string representation of value, 
//     or System.String.Empty if value is null.

You may think that the method will always return some string. However, the string representation of the object may have a null value. As a result, the method will return null.

Here's the simplest example:

0890_DNN/image4.png

By the way, it turns out that:

if obj == null, stringRepresentation != null (an empty string);
if obj != null, stringRepresentation == null.

Hmm, that's a bit tangled...

You could say that this is a synthetic example. Who returns null from the ToString method? Well, I know that Microsoft had a few cases (follow the link and take a look at Issue 14).

And here's the question! Did the code authors know about this peculiarity? Did they take this into account or not? What about you? Did you know about this?

By the way, nullable reference types can help here. The method's signature indicates that the method may return the null value. As a result, possible misunderstanding is gone:

public static string? ToString(object? value)

Now it's time for a break. Pour some more coffee and take a few cookies. It's coffee break!

0890_DNN/image5.png

Grabbed a snack? We're proceeding to the next issue.

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,
};

The PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'module.DesktopModule' object Dnn.PersonaBar.Extensions Converters.cs 67

Take a look at FriendlyName and IsPortable initialization. The developers use module.DesktopModule.FriendlyName and module.DesktopModule?.IsPortable as values for initialization. You might ask – can module.DesktopModule be null? If it is null, ?. won't protect the code because module.DesktopModule.FriendlyName does not contain null checking. If it is not null, ?. is redundant and misleading.

Here's a strikingly similar code fragment.

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

The PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'PortalSettings.Current' object Dnn.PersonaBar.Extensions PagesMenuController.cs 47

The same happens here. When developers initialize the dictionary, they use PortalSettings.Current several times. In some cases, they check it for null, in other cases, they don't:

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

The PVS-Studio warning: V3095 The 'coloumnValue' object was used before it was verified against null. Check lines: 902, 903. DotNetNuke.Library CBO.cs 902

The GetType method is called for the coloumnValue variable. Then, coloumnValue != null is checked. This looks strange.

Unfortunately, we have another similar case. Here it is:

private void DeleteLanguage()
{
  ....
  // Attempt to get the Locale
  Locale language = LocaleController.Instance
                                    .GetLocale(tempLanguagePack.LanguageID);
  if (tempLanguagePack != null)
  {
    LanguagePackController.DeleteLanguagePack(tempLanguagePack);
  }
  ....
}

The PVS-Studio warning: V3095 The 'tempLanguagePack' object was used before it was verified against null. Check lines: 235, 236. DotNetNuke.Library LanguageInstaller.cs 235

The same story – at first, the LanguageId property (tempLanguagePack.LanguageID) is accessed. On the next line, the tempLanguagePack != null is checked.

And more...

private static void AddLanguageHttpAlias(int portalId, Locale locale)
{
  ....
  var portalAliasInfos =    portalAliasses as IList<PortalAliasInfo> 
                         ?? portalAliasses.ToList();

  if (portalAliasses != null && portalAliasInfos.Any())
  ....
}

The PVS-Studio warning: V3095 The 'portalAliasses' object was used before it was verified against null. Check lines: 1834, 1835. DotNetNuke.Library Localization.cs 1834

That's all for this pattern. Although, the analyzer issued similar warnings for other code fragments. Let's take a look at another way to refer to members before checking for 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();
  }
}

The PVS-Studio warning: V3095 The 'e.FullPath' object was used before it was verified against null. Check lines: 147, 152. DotNetNuke.Web DotNetNukeShutdownOverload.cs 147

Notice e.FullPath. At first, e.FullPath.EndsWith(".log.resources") is accessed. Then, the ?? operator is used: e.FullPath ?? string.Empty.

This code is successfully multiplied via 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

I think that's enough for V3095. And I guess you don't want to read about it anymore. So, let's move on.

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

The PVS-Studio warning: 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

The Constants.FOLDER_ValidFolderPath value is initially written in the physicalPath field. Then, string.Empty is assigned to the same field. Note that these values are different. That's why this code looks even more suspicious:

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

  ....
}

The PVS-Studio warning: V3022 Expression 'y < 0' is always false. Unsigned type value is always >= 0. CountryListBox CountryLookup.cs 210

byte type values are in the [0; 255] range. Hence, the y < 0 check will always give false, and then branch will never be executed.

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

The PVS-Studio warning: V3057 The 'Combine' function is expected to receive a valid path string. Inspect the second argument. DotNetNuke.Library PortalController.cs 3538

Hmm. An interesting error. Here we have two operations to construct a path (the Path.Combine call). The first one is clear, but the second one is not. Apparently, in the second case, the developers wanted to take the admin.template file not from the templatePath directory, but from the parent one. Unfortunately, after they added ..\, the path became invalid since an escape sequence was formed: ..\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();
  }
  ....
}

The PVS-Studio warning: V3057 The 'Remove' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. DotNetNuke.Log4Net StackTraceDetailPatternConverter.cs 67

Now, this code runs without any errors, but looking at it, I have a sneaking suspicion that something is wrong. In the then branch of the if statement, the value of sb.Length is >= 1. When the Remove method is called, we subtract 2 from this value. So, if sb.Length == 1, the call will be as follows: sb.Remove(-1, 2). This will cause an exception.

Right now, this code runs because, in StringBuilder, strings are added via the "{0}, " format. Therefore, these lines consist of 2 characters. A check like that is ambiguous and causes concerns.

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

Here we have 2 issues. Looks as if they are multiplied by copy-paste. Try to find them! The answer is behind this picture.

0890_DNN/image6.png

Whoops, my bad! I forgot to give you a clue… Here you are:

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

Hope it's clear now. Found a problem? If not (or you don't want to do that), take a look at the analyzer warnings:

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

Notice the last parameters and arguments. In both calls, journalItem.CommentsDisabled comes before journalItem.CommentsHidden. However, the commentsHidden parameter comes before commentsDisabled. Yeah, that's suspicious.

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

The PVS-Studio warning: V3077 The setter of 'LastPurge' property does not utilize its 'value' parameter. DotNetNuke.Library IPCount.cs 96

The fact that set-accessor does not use the value parameter is suspicious. So, it's possible to write something to this property, but the assigned value is… ignored. I found one place in the code, where the following property is assigned:

public static bool CheckIp(string ipAddress)
{
  ....
  LastPurge = DateTime.Now;
  ....
}

As a result, in this case, DateTime.Now will not be stored anywhere.

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

The PVS-Studio warning: 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

Again, the variable is assigned twice – the whole expression is duplicated. Perhaps it's redundant. But maybe developers copied the expression and forgot to change it. Hmm… The last line effect?

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

The PVS-Studio warning: V3139 Two or more case-branches perform the same actions. DotNetNuke.HttpModules RequestFilterModule.cs 81

I guess these case branches should not be identical. In the second case, AddressType.IPv6 should be used.

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

The PVS-Studio warning: V3139 Two or more case-branches perform the same actions. DotNetNuke.Tests.Core PropertyAccessTests.cs 118

Pay attention to "d" and "w" – the bodies of the case branches. They duplicate each other. Copy-paste… Copy-paste never changes. The DateTime type doesn't contain the AddWeeks method, however, the case branch "w" obviously must work with weeks.

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

The PVS-Studio warning: V3030 Recurring check. The 'customAliasUsedAndNotCurrent && settings.RedirectUnfriendly' condition was already verified in line 1095. DotNetNuke.Library TabIndexController.cs 1097

The analyzer detects the following pattern:

if (a && b)
  ....
else
{
  if (a && b)
    ....
}

In this code fragment, the second condition will be false – the variables have not changed between calls.

However, here we hit the big jackpot! Besides the conditions, the blocks of code are duplicated. if with its then branch was entirely copied.

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

The PVS-Studio warning: V3095 The 'tabIds' object was used before it was verified against null. Check lines: 356, 357. Dnn.PersonaBar.Library TabsController.cs 356

We've discussed a similar case before, but I decided to do this again and analyze it in more detail.

The tabIds parameter is expected to have a null value. Otherwise, why do we check tabIds == null? But something is messed up here again...

Suppose tabIds is null, then:

the left operand of the ?? operator is evaluated (tabIds as int[]);
tabIds as int[] results in null;
the right operand of the ?? operator is evaluated (tabIds.ToArray());
the ToArray method call leads to an exception because tabIds is null.

Turns out that the check failed.

Issue 43

And now take a chance to find an error yourself! I simplified the task for you. Below is a shortened method, I cut almost everything unnecessary. The original method contained 500 lines – doubt that you would find the error. Although, if you want, take a look at it – here's a link on GitHub.

If you figure out what's wrong, you'll definitely get endorphins rush. :)

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

Here's a picture to hide the answer. You'll find it right behind the unicorn.

0890_DNN/image7.png

Now, it's time to check yourself!

The PVS-Studio warning: 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

Wow, the analyzer is very attentive! Here's the shortened code.

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

Note that in the second case, we process 'height' variables, not 'width'. However, when we call the UpdateModuleSetting method, this.txtWidth.Text is passed instead of this.txtHeight.Text.

Issue N

Of course, these are not all warnings the analyzer found. I tried to select the most interesting and concise. The analyzer also issued interprocedural warnings and lots of others similar to those that we discussed. I guess the project developers are interested in warnings more than the readers.

Also, the analyzer issued false positives. We discussed some of them. I guess the analyzer developers are interested in other false positives more than the readers. So, I didn't write about them all.

Conclusion

In my opinion, the issues are diverse. You may say: "I would never make such errors!" But humans tend to make mistakes – this is totally normal! There are lots of reasons for this. That's why we regularly find new errors.

We also make mistakes. And sometimes false positives happen – we admit those issues and fix them. :)

As for code quality, is it enough to have a team of experts? I don't think so. You have to adopt a complex approach and use various tools/techniques to control code and product quality.

Let's sum it up:

be careful with copy-paste;
use static analysis;
follow me on Twitter.

P.S. By the way, what's your Top 10 warnings from this article? ;)

A variety of errors in C# code by the example of CMS DotNetNuke: 40 questions about the quality