{"id":399345,"date":"2024-06-29T14:37:39","date_gmt":"2024-06-29T14:37:39","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=399345"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=399345","title":{"rendered":"<span>PVS-Studio checks Umbraco code for the third time<\/span>"},"content":{"rendered":"<div><!--[--><!--]--><\/div>\n<div id=\"post-content-body\">\n<div>\n<div class=\"article-formatted-body article-formatted-body article-formatted-body_version-2\">\n<div xmlns=\"http:\/\/www.w3.org\/1999\/xhtml\">\n<p>Six years ago, we first checked Umbraco with the PVS-Studio static analyzer for C#. Today, we decided to go where it all started and analyze the Umbraco CMS source code.<\/p>\n<figure class=\"full-width\"><img loading=\"lazy\" decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/getpro\/habr\/upload_files\/e34\/f73\/6e1\/e34f736e167e147486ac8f8c3627f8f2.png\" width=\"580\" height=\"327\" data-src=\"https:\/\/habrastorage.org\/getpro\/habr\/upload_files\/e34\/f73\/6e1\/e34f736e167e147486ac8f8c3627f8f2.png\"\/><figcaption><\/figcaption><\/figure>\n<h3>Introduction<\/h3>\n<p>As you guessed from the title, we wrote two articles about the Umbraco project check.<\/p>\n<ul>\n<li>\n<p><a href=\"https:\/\/pvs-studio.com\/en\/blog\/posts\/csharp\/0357\/\">The First C# Project Analyzed<\/a>,<\/p>\n<\/li>\n<li>\n<p><a href=\"https:\/\/pvs-studio.com\/en\/blog\/posts\/csharp\/0461\/\">Re-analysis of Umbraco code<\/a>.<\/p>\n<\/li>\n<\/ul>\n<p>Take a look at how the error types changed with time.<\/p>\n<p>If you are interested in this article, you probably know about Umbraco. Still, let me remind you. Umbraco is an open-source content management system that provides great experience of editing website content. You can find the source code on <a href=\"https:\/\/github.com\/umbraco\/Umbraco-CMS\">GitHub<\/a>.<\/p>\n<p>Let me also remind you about PVS-Studio. \ud83d\ude09<\/p>\n<p><a href=\"https:\/\/pvs-studio.com\/en\/\">PVS-Studio<\/a> is a static analysis tool for improving code quality, safety (SAST), and security. It works with C, C++, C#, and Java languages and runs on Windows, Linux, and macOS.<\/p>\n<p>We chose the Umbraco project version of 12.11.2021 on <a href=\"https:\/\/github.com\/umbraco\/Umbraco-CMS\">GitHub<\/a>. The PVS-Studio version used \u2014 7.15.54288.<\/p>\n<p>As usual, we selected the most interesting warnings for this article. Some of them point to obvious errors. Some point to the suspicious code. But let&#8217;s get down to business and look at what we found.<\/p>\n<h3>How are the warnings doing?<\/h3>\n<p><strong>Issue 1<\/strong><\/p>\n<p>Can you find an error in this fragment?<\/p>\n<pre><code>protected virtual string VisitMethodCall(MethodCallExpression m) {   ....   case \"SqlText\":     if (m.Method.DeclaringType != typeof(SqlExtensionsStatics))       goto default;     if (m.Arguments.Count == 2)     {       var n1 = Visit(m.Arguments[0]);       var f = m.Arguments[2];       if (!(f is Expression&lt;Func&lt;string, string>> fl))         throw new NotSupportedException(\"Expression is not a proper                                           lambda.\");       var ff = fl.Compile();       return ff(n1);     }     else if (m.Arguments.Count == 3)     {       var n1 = Visit(m.Arguments[0]);       var n2 = Visit(m.Arguments[1]);       var f = m.Arguments[2];       if (!(f is Expression&lt;Func&lt;string, string, string>> fl))         throw new NotSupportedException(\"Expression is not a proper                                           lambda.\");       var ff = fl.Compile();       return ff(n1, n2);     }     else if (m.Arguments.Count == 4)     {       var n1 = Visit(m.Arguments[0]);       var n2 = Visit(m.Arguments[1]);       var n3 = Visit(m.Arguments[3]);       var f = m.Arguments[3];       if (!(f is Expression&lt;Func&lt;string, string, string, string>> fl))         throw new NotSupportedException(\"Expression is not a proper                                           lambda.\");       var ff = fl.Compile();       return ff(n1, n2, n3);     }     else       throw new NotSupportedException(\"Expression is not a proper lambda.\");      .... } <\/code><\/pre>\n<p>Okay-okay, now look at the shortened version of the code.<\/p>\n<pre><code>protected virtual string VisitMethodCall(MethodCallExpression m) {   ....   case \"SqlText\":     ....     if (m.Arguments.Count == 2)     {       var n1 = Visit(m.Arguments[0]);       var f = m.Arguments[2];       ....     } } <\/code><\/pre>\n<p>PVS-Studio warning: <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3106\/\">V3106<\/a> Possibly index is out of bound. The &#8216;2&#8217; index is pointing beyond &#8216;m.Arguments&#8217; bound. ExpressionVisitorBase.cs 632<\/p>\n<p>I think every developer made such mistakes at least once. The developers check that <em>m.Arguments.Count<\/em> equals 2, and immediately after that they try to access the third element. Obviously, this leads to <em>IndexOutOfRangeException<\/em>.<\/p>\n<p>We found similar errors in other projects. As you see, Umbraco is no exception.<\/p>\n<p><strong>Issue 2<\/strong><\/p>\n<p>Let&#8217;s test your attention-paying abilities. Try to find an error here yourself. The code fragment is followed by a picture. Only after that you can read the correct answer.<\/p>\n<pre><code>public static string ToXmlString(this object value, Type type) {   if (value == null) return string.Empty;   if (type == typeof(string))      return (value.ToString().IsNullOrWhiteSpace() ? \"\" : value.ToString());   if (type == typeof(bool)) return XmlConvert.ToString((bool)value);   if (type == typeof(byte)) return XmlConvert.ToString((byte)value);   if (type == typeof(char)) return XmlConvert.ToString((char)value);   if (type == typeof(DateTime)) return XmlConvert.ToString((DateTime)value,   XmlDateTimeSerializationMode.Unspecified);   if (type == typeof(DateTimeOffset))      return XmlConvert.ToString((DateTimeOffset)value);   if (type == typeof(decimal)) return XmlConvert.ToString((decimal)value);   if (type == typeof(double)) return XmlConvert.ToString((double)value);   if (type == typeof(float)) return XmlConvert.ToString((float)value);   if (type == typeof(Guid)) return XmlConvert.ToString((Guid)value);   if (type == typeof(int)) return XmlConvert.ToString((int)value);   if (type == typeof(long)) return XmlConvert.ToString((long)value);   if (type == typeof(sbyte)) return XmlConvert.ToString((sbyte)value);   if (type == typeof(short)) return XmlConvert.ToString((short)value);   if (type == typeof(TimeSpan)) return XmlConvert.ToString((TimeSpan)value);   if (type == typeof(bool)) return XmlConvert.ToString((bool)value);   if (type == typeof(uint)) return XmlConvert.ToString((uint)value);   if (type == typeof(ulong)) return XmlConvert.ToString((ulong)value);   if (type == typeof(ushort)) return XmlConvert.ToString((ushort)value);   .... } <\/code><\/pre>\n<figure class=\"\"><img loading=\"lazy\" decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/getpro\/habr\/upload_files\/d01\/ee0\/e0f\/d01ee0e0f9d3e74860a0eab7b19bee09.png\" width=\"450\" height=\"388\" data-src=\"https:\/\/habrastorage.org\/getpro\/habr\/upload_files\/d01\/ee0\/e0f\/d01ee0e0f9d3e74860a0eab7b19bee09.png\"\/><figcaption><\/figcaption><\/figure>\n<p>If you have quickly found an error, you have an eagle eye! Look at the shortened version of the method:<\/p>\n<pre><code>public static string ToXmlString(this object value, Type type) {   ....   if (type == typeof(bool)) return XmlConvert.ToString((bool)value);   ....   if (type == typeof(bool)) return XmlConvert.ToString((bool)value);   .... } <\/code><\/pre>\n<p>PVS-Studio issued warning <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3021\/\">V3021<\/a>: There are two &#8216;if&#8217; statements with identical conditional expressions. The first &#8216;if&#8217; statement contains method return. This means that the second &#8216;if&#8217; statement is senseless ObjectExtensions.cs 615<\/p>\n<p>Not a very attractive code fragment for code review, right?<\/p>\n<p>Looks like we got lucky and there&#8217;s just an extra <em>if<\/em> statement. You can deduce this when analyzing the used and available overloads of the <em>XmlConvert.ToString<\/em> method. But not everyone is so lucky \u2014 sometimes copy-paste <a href=\"https:\/\/pvs-studio.com\/en\/blog\/examples\/v3021\/\">hides inconspicuous errors<\/a>.<\/p>\n<p><strong>Issue 3<\/strong><\/p>\n<pre><code>public bool FlagOutOfDateModels {   get => _flagOutOfDateModels;    set   {     if (!ModelsMode.IsAuto())     {       _flagOutOfDateModels = false;     }      _flagOutOfDateModels = value;   } } <\/code><\/pre>\n<p>PVS-Studio issued warning <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3008\/\">V3008<\/a> The &#8216;_flagOutOfDateModels&#8217; variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 54, 51. ModelsBuilderSettings.cs 54<\/p>\n<p>As you see, set accessor has a check with the assignment of the <em>_flagOutOfDateModels<\/em> value. However, immediately after this check, another value is set to the same field. The <em>if<\/em> block has no practical use.<\/p>\n<p><strong>Issue 4<\/strong><\/p>\n<pre><code>private bool MatchesEndpoint(string absPath) {   IEnumerable&lt;RouteEndpoint> routeEndpoints = _endpointDataSource     ?.Endpoints     .OfType&lt;RouteEndpoint>()     .Where(x =>     {       ....     });    var routeValues = new RouteValueDictionary();    RouteEndpoint matchedEndpoint = routeEndpoints     .Where(e => new TemplateMatcher(         TemplateParser.Parse(e.RoutePattern.RawText),         new RouteValueDictionary())       .TryMatch(absPath, routeValues))     .OrderBy(c => c.Order)     .FirstOrDefault();    return matchedEndpoint != null; }<\/code><\/pre>\n<p>PVS-Studio issued warning <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3105\/\">V3105<\/a> The &#8216;routeEndpoints&#8217; variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RoutableDocumentFilter.cs 198<\/p>\n<p>Diagnostics <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3105\/\">V3105<\/a> warns about the possibility of a <em>NullReferenceException<\/em>. <em>_endpointDataSource<\/em> is checked for <em>null<\/em> with the &#8216;?.&#8217; operator. If the* _endpointDataSource* variable still contains the <em>null<\/em> value, then <em>routeEndpoints<\/em> is also <em>null.<\/em><\/p>\n<p>It&#8217;s weird that we access <em>routeEndpoints<\/em> without the &#8216;?.&#8217; operator. As a result, if <em>routeEndpoints<\/em> is <em>null<\/em>, <em>NullReferenceException<\/em> will be thrown when we access this reference.<\/p>\n<p><strong>Issue 5<\/strong><\/p>\n<pre><code>public void Handle(ContentCopiedNotification notification) {   ....   if (relationType == null)   {     relationType = new RelationType(       Constants.Conventions.RelationTypes.RelateDocumentOnCopyAlias,       Constants.Conventions.RelationTypes.RelateDocumentOnCopyName,       true,       Constants.ObjectTypes.Document,       Constants.ObjectTypes.Document);      _relationService.Save(relationType);   }   .... } <\/code><\/pre>\n<p>PVS-Studio warning: <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3066\/\">V3066<\/a> Possible incorrect order of arguments passed to &#8216;RelationType&#8217; constructor. RelateOnCopyNotificationHandler.cs 32<\/p>\n<p>In this case, the constructor is called, and arguments are passed to it. Let&#8217;s look at its signature:<\/p>\n<pre><code>public RelationType(string name,                     string alias,                     bool isBidrectional,                     Guid? parentObjectType,                     Guid? childObjectType) <\/code><\/pre>\n<p>Looks like the arguments are passed in the wrong order. The <em>RelateDocumentOnCopy<\/em><strong><em>Alias<\/em><\/strong> argument is passed to the <em>name parameter of the constructor. The RelateDocumentOnCopy**Name<\/em> is passed to the <em>alias<\/em> parameter.<\/p>\n<p><strong>Issue 6<\/strong><\/p>\n<pre><code>private static async Task&lt;Attempt&lt;UrlInfo>> DetectCollisionAsync(....) {   ....   if (pcr.IgnorePublishedContentCollisions)   {     logger.LogDebug(logMsg, url, uri, culture);   }   else   {     logger.LogDebug(logMsg, url, uri, culture);   } }<\/code><\/pre>\n<p>PVS-Studio warning:<a href=\"https:\/\/pvs-studio.com\/en\/w\/v3004\/\"> V3004<\/a> The &#8216;then&#8217; statement is equivalent to the &#8216;else&#8217; statement. UrlProviderExtensions.cs 274<\/p>\n<p>The analyzer has found a construction where branches <em>then<\/em> and <em>else<\/em> are identical. The same code is executed regardless of the property value checked. Most likely, the developer copied the code and forgot to fix the method parameters.<\/p>\n<figure class=\"\"><img loading=\"lazy\" decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/getpro\/habr\/upload_files\/9de\/276\/f22\/9de276f22dfe73bbc9e10fd20263148a.png\" width=\"400\" height=\"345\" data-src=\"https:\/\/habrastorage.org\/getpro\/habr\/upload_files\/9de\/276\/f22\/9de276f22dfe73bbc9e10fd20263148a.png\"\/><figcaption><\/figcaption><\/figure>\n<p><strong>Issue 7<\/strong><\/p>\n<pre><code>public async Task&lt;bool> IsMemberAuthorizedAsync(....) {   ....   if (IsLoggedIn() == false)   {     allowAction = false;   }   else   {      string username;     ....     username = currentMember.UserName;     IList&lt;string> allowTypesList = allowTypes as IList&lt;string> ??                                                allowTypes.ToList();     if (allowTypesList.Any(allowType => allowType != string.Empty))     {       allowAction = allowTypesList.Select(x => x.ToLowerInvariant())                                                 .Contains(currentMember                                                 .MemberTypeAlias                                                 .ToLowerInvariant());     }      if (allowAction &amp;&amp; allowMembers.Any())     {       allowAction = allowMembers.Contains(memberId);     }     ....   }   return allowAction; } <\/code><\/pre>\n<p>PVS-Studio warning: <a href=\"https:\/\/pvs-studio.com\/en\/docs\/warnings\/v3137\/\">V3137<\/a> The &#8216;username&#8217; variable is assigned but is not used by the end of the function. MemberManager.cs 87<\/p>\n<p>We noticed an interesting warning. The developer declares the <em>username<\/em> variable and assigns a value to it. After that <em>username<\/em> is never used. <\/p>\n<p>Most likely the developers didn&#8217;t delete it after refactoring. However, there&#8217;s a probability that some logic was not implemented, or a tricky error is hidden here.<\/p>\n<p><strong>Issue 8<\/strong><\/p>\n<pre><code>public async Task&lt;ActionResult&lt;UserDisplay>> PostInviteUser(UserInvite userSave) {   if (_securitySettings.UsernameIsEmail)   {     userSave.Username = userSave.Email;   }   else   {     var userResult = CheckUniqueUsername(userSave.Username, u =>                                            u.LastLoginDate != default                                         || u.EmailConfirmedDate.HasValue);                                               if (!(userResult.Result is null))     {       return userResult.Result;     }      user = userResult.Value;   }   user = CheckUniqueEmail(userSave.Email, u => u.LastLoginDate != default ||                                               u.EmailConfirmedDate.HasValue);   .... } <\/code><\/pre>\n<p>PVS-Studio warning <a href=\"https:\/\/pvs-studio.com\/en\/docs\/warnings\/v3008\/\">V3008<\/a> The &#8216;user&#8217; variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 446, 444. UsersController.cs 446<\/p>\n<p>In the <em>else<\/em> block of the conditional expression, the *user *value is assigned. Right after the conditional expression is completed, <em>user<\/em> is assigned again. Therefore, the previously assigned value is not used and is immediately overwritten. It&#8217;s not clear whether the <em>userResult.Value<\/em> value should have been used, and some logic is missing, or it&#8217;s just a redundant code. Anyway, we are a bit suspicious about this code fragment.<\/p>\n<p><strong>Issue 9<\/strong><\/p>\n<pre><code>public ActionResult&lt;PagedResult&lt;EntityBasic>> GetPagedChildren(....                                                                int pageNumber,                                                                ....) {   if (pageNumber &lt;= 0)   {     return NotFound();   }   ....   if (objectType.HasValue)   {     if (id == Constants.System.Root &amp;&amp;         startNodes.Length > 0 &amp;&amp;         startNodes.Contains(Constants.System.Root) == false &amp;&amp;         !ignoreUserStartNodes)     {       if (pageNumber > 0)  \/\/ &lt;=       {         return new PagedResult&lt;EntityBasic>(0, 0, 0);       }       IEntitySlim[] nodes = _entityService.GetAll(objectType.Value,                                                    startNodes).ToArray();       if (nodes.Length == 0)       {         return new PagedResult&lt;EntityBasic>(0, 0, 0);       }        if (pageSize &lt; nodes.Length)       {         pageSize = nodes.Length; \/\/ bah       }        var pr = new PagedResult&lt;EntityBasic>(nodes.Length, pageNumber, pageSize)       {         Items = nodes.Select(_umbracoMapper.Map&lt;EntityBasic>)       };       return pr;     }   } } <\/code><\/pre>\n<p>PVS-Studio warning:<a href=\"https:\/\/pvs-studio.com\/en\/docs\/warnings\/v3022\/\"> V3022<\/a> Expression &#8216;pageNumber > 0&#8217; is always true. EntityController.cs 625<\/p>\n<p>The developer checks that <em>pageNumber<\/em> is less than or equal to 0. If it&#8217;s true, they exit from the method. Further on, the code checks whether <em>pageNumber<\/em> is greater than 0<em>.<\/em> Of course, this condition is always true. Therefore, the method exits. The code written after the <em>if<\/em> statement (a lot of code*,* by the way) is never executed. <\/p>\n<p>Here the analyzer also issued a warning about unreachable code: <a href=\"https:\/\/pvs-studio.com\/en\/docs\/warnings\/v3142\/\">V3142<\/a> Unreachable code detected. It is possible that an error is present. EntityController.cs 630<\/p>\n<p><strong>Issue 10<\/strong><\/p>\n<p>Here an error hides in the test. You may think that it&#8217;s not so important, but tests ensure that your code works in a defined way. If tests have errors, can we be sure that the program works correctly? At such moments static analysis <a href=\"https:\/\/pvs-studio.com\/en\/blog\/posts\/cpp\/a0080\/\">comes to the rescue<\/a>.<\/p>\n<pre><code>Public void SimpleConverter3Test() {   ....   IpublishedContentType contentType1 =     contentTypeFactory.CreateContentType(Guid.NewGuid(),     1002, \"content1\", t => CreatePropertyTypes(t, 1));    IpublishedContentType contentType2 =     contentTypeFactory.CreateContentType(Guid.NewGuid(),     1003, \"content2\", t => CreatePropertyTypes(t, 2));   ....   var cnt1 = new InternalPublishedContent(contentType1) \/\/ &lt;=   {     Id = 1003,     Properties = new[]     {       new InternalPublishedProperty {Alias = \"prop1\",         SolidHasValue = true, SolidValue = \"val1\"}     }   };   var cnt2 = new InternalPublishedContent(contentType1) \/\/ &lt;=   {     Id = 1004,     Properties = new[]     {       new InternalPublishedProperty {Alias = \"prop2\",         SolidHasValue = true, SolidValue = \"1003\"}     }   }; } <\/code><\/pre>\n<p>PVS-Studio warning: <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3056\/\">V3056<\/a> Consider reviewing the correctness of &#8216;contentType1&#8217; item&#8217;s usage. ConvertersTests.cs 115<\/p>\n<p>Most likely, it&#8217;s a copy-paste error: <em>contentType1<\/em> is used instead of <em>contentType2<\/em> when we declare the <em>cnt2 variable.<\/em> Agree, it&#8217;s a bit weird. <\/p>\n<h3>Conclusion<\/h3>\n<p>It was a pleasure to check the Umbraco code again. By the way, judging by the code comments, the developers started using ReSharper. However, PVS-Studio still found interesting errors. Conclusion \u2014 you can profit more by using several tools simultaneously. \ud83d\ude09<\/p>\n<p>If you want to check your project, you can request a trial key on <a href=\"https:\/\/pvs-studio.com\/pvs-studio\/try-free\/?utm_source=habr&amp;utm_medium=articles&amp;utm_content=umbraco&amp;utm_term=link_try-free\">our website<\/a>.<\/p>\n<p>And do not forget that one-time checks are better than none. But the maximum benefit from static analysis is achieved with its regular use and implementation in processes.<\/p>\n<\/div>\n<\/div>\n<\/div>\n<p><!----><!----><\/div>\n<p><!----><!----><br \/> \u0441\u0441\u044b\u043b\u043a\u0430 \u043d\u0430 \u043e\u0440\u0438\u0433\u0438\u043d\u0430\u043b \u0441\u0442\u0430\u0442\u044c\u0438 <a href=\"https:\/\/habr.com\/ru\/articles\/595507\/\"> https:\/\/habr.com\/ru\/articles\/595507\/<\/a><\/p>\n","protected":false},"excerpt":{"rendered":"<div><!--[--><!--]--><\/div>\n<div id=\"post-content-body\">\n<div>\n<div class=\"article-formatted-body article-formatted-body article-formatted-body_version-2\">\n<div xmlns=\"http:\/\/www.w3.org\/1999\/xhtml\">\n<p>Six years ago, we first checked Umbraco with the PVS-Studio static analyzer for C#. Today, we decided to go where it all started and analyze the Umbraco CMS source code.<\/p>\n<figure class=\"full-width\"><figcaption><\/figcaption><\/figure>\n<h3>Introduction<\/h3>\n<p>As you guessed from the title, we wrote two articles about the Umbraco project check.<\/p>\n<ul>\n<li>\n<p><a href=\"https:\/\/pvs-studio.com\/en\/blog\/posts\/csharp\/0357\/\">The First C# Project Analyzed<\/a>,<\/p>\n<\/li>\n<li>\n<p><a href=\"https:\/\/pvs-studio.com\/en\/blog\/posts\/csharp\/0461\/\">Re-analysis of Umbraco code<\/a>.<\/p>\n<\/li>\n<\/ul>\n<p>Take a look at how the error types changed with time.<\/p>\n<p>If you are interested in this article, you probably know about Umbraco. Still, let me remind you. Umbraco is an open-source content management system that provides great experience of editing website content. You can find the source code on <a href=\"https:\/\/github.com\/umbraco\/Umbraco-CMS\">GitHub<\/a>.<\/p>\n<p>Let me also remind you about PVS-Studio. \ud83d\ude09<\/p>\n<p><a href=\"https:\/\/pvs-studio.com\/en\/\">PVS-Studio<\/a> is a static analysis tool for improving code quality, safety (SAST), and security. It works with C, C++, C#, and Java languages and runs on Windows, Linux, and macOS.<\/p>\n<p>We chose the Umbraco project version of 12.11.2021 on <a href=\"https:\/\/github.com\/umbraco\/Umbraco-CMS\">GitHub<\/a>. The PVS-Studio version used \u2014 7.15.54288.<\/p>\n<p>As usual, we selected the most interesting warnings for this article. Some of them point to obvious errors. Some point to the suspicious code. But let&#8217;s get down to business and look at what we found.<\/p>\n<h3>How are the warnings doing?<\/h3>\n<p><strong>Issue 1<\/strong><\/p>\n<p>Can you find an error in this fragment?<\/p>\n<pre><code>protected virtual string VisitMethodCall(MethodCallExpression m) {   ....   case \"SqlText\":     if (m.Method.DeclaringType != typeof(SqlExtensionsStatics))       goto default;     if (m.Arguments.Count == 2)     {       var n1 = Visit(m.Arguments[0]);       var f = m.Arguments[2];       if (!(f is Expression&lt;Func&lt;string, string>> fl))         throw new NotSupportedException(\"Expression is not a proper                                           lambda.\");       var ff = fl.Compile();       return ff(n1);     }     else if (m.Arguments.Count == 3)     {       var n1 = Visit(m.Arguments[0]);       var n2 = Visit(m.Arguments[1]);       var f = m.Arguments[2];       if (!(f is Expression&lt;Func&lt;string, string, string>> fl))         throw new NotSupportedException(\"Expression is not a proper                                           lambda.\");       var ff = fl.Compile();       return ff(n1, n2);     }     else if (m.Arguments.Count == 4)     {       var n1 = Visit(m.Arguments[0]);       var n2 = Visit(m.Arguments[1]);       var n3 = Visit(m.Arguments[3]);       var f = m.Arguments[3];       if (!(f is Expression&lt;Func&lt;string, string, string, string>> fl))         throw new NotSupportedException(\"Expression is not a proper                                           lambda.\");       var ff = fl.Compile();       return ff(n1, n2, n3);     }     else       throw new NotSupportedException(\"Expression is not a proper lambda.\");      .... } <\/code><\/pre>\n<p>Okay-okay, now look at the shortened version of the code.<\/p>\n<pre><code>protected virtual string VisitMethodCall(MethodCallExpression m) {   ....   case \"SqlText\":     ....     if (m.Arguments.Count == 2)     {       var n1 = Visit(m.Arguments[0]);       var f = m.Arguments[2];       ....     } } <\/code><\/pre>\n<p>PVS-Studio warning: <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3106\/\">V3106<\/a> Possibly index is out of bound. The &#8216;2&#8217; index is pointing beyond &#8216;m.Arguments&#8217; bound. ExpressionVisitorBase.cs 632<\/p>\n<p>I think every developer made such mistakes at least once. The developers check that <em>m.Arguments.Count<\/em> equals 2, and immediately after that they try to access the third element. Obviously, this leads to <em>IndexOutOfRangeException<\/em>.<\/p>\n<p>We found similar errors in other projects. As you see, Umbraco is no exception.<\/p>\n<p><strong>Issue 2<\/strong><\/p>\n<p>Let&#8217;s test your attention-paying abilities. Try to find an error here yourself. The code fragment is followed by a picture. Only after that you can read the correct answer.<\/p>\n<pre><code>public static string ToXmlString(this object value, Type type) {   if (value == null) return string.Empty;   if (type == typeof(string))      return (value.ToString().IsNullOrWhiteSpace() ? \"\" : value.ToString());   if (type == typeof(bool)) return XmlConvert.ToString((bool)value);   if (type == typeof(byte)) return XmlConvert.ToString((byte)value);   if (type == typeof(char)) return XmlConvert.ToString((char)value);   if (type == typeof(DateTime)) return XmlConvert.ToString((DateTime)value,   XmlDateTimeSerializationMode.Unspecified);   if (type == typeof(DateTimeOffset))      return XmlConvert.ToString((DateTimeOffset)value);   if (type == typeof(decimal)) return XmlConvert.ToString((decimal)value);   if (type == typeof(double)) return XmlConvert.ToString((double)value);   if (type == typeof(float)) return XmlConvert.ToString((float)value);   if (type == typeof(Guid)) return XmlConvert.ToString((Guid)value);   if (type == typeof(int)) return XmlConvert.ToString((int)value);   if (type == typeof(long)) return XmlConvert.ToString((long)value);   if (type == typeof(sbyte)) return XmlConvert.ToString((sbyte)value);   if (type == typeof(short)) return XmlConvert.ToString((short)value);   if (type == typeof(TimeSpan)) return XmlConvert.ToString((TimeSpan)value);   if (type == typeof(bool)) return XmlConvert.ToString((bool)value);   if (type == typeof(uint)) return XmlConvert.ToString((uint)value);   if (type == typeof(ulong)) return XmlConvert.ToString((ulong)value);   if (type == typeof(ushort)) return XmlConvert.ToString((ushort)value);   .... } <\/code><\/pre>\n<figure class=\"\"><figcaption><\/figcaption><\/figure>\n<p>If you have quickly found an error, you have an eagle eye! Look at the shortened version of the method:<\/p>\n<pre><code>public static string ToXmlString(this object value, Type type) {   ....   if (type == typeof(bool)) return XmlConvert.ToString((bool)value);   ....   if (type == typeof(bool)) return XmlConvert.ToString((bool)value);   .... } <\/code><\/pre>\n<p>PVS-Studio issued warning <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3021\/\">V3021<\/a>: There are two &#8216;if&#8217; statements with identical conditional expressions. The first &#8216;if&#8217; statement contains method return. This means that the second &#8216;if&#8217; statement is senseless ObjectExtensions.cs 615<\/p>\n<p>Not a very attractive code fragment for code review, right?<\/p>\n<p>Looks like we got lucky and there&#8217;s just an extra <em>if<\/em> statement. You can deduce this when analyzing the used and available overloads of the <em>XmlConvert.ToString<\/em> method. But not everyone is so lucky \u2014 sometimes copy-paste <a href=\"https:\/\/pvs-studio.com\/en\/blog\/examples\/v3021\/\">hides inconspicuous errors<\/a>.<\/p>\n<p><strong>Issue 3<\/strong><\/p>\n<pre><code>public bool FlagOutOfDateModels {   get => _flagOutOfDateModels;    set   {     if (!ModelsMode.IsAuto())     {       _flagOutOfDateModels = false;     }      _flagOutOfDateModels = value;   } } <\/code><\/pre>\n<p>PVS-Studio issued warning <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3008\/\">V3008<\/a> The &#8216;_flagOutOfDateModels&#8217; variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 54, 51. ModelsBuilderSettings.cs 54<\/p>\n<p>As you see, set accessor has a check with the assignment of the <em>_flagOutOfDateModels<\/em> value. However, immediately after this check, another value is set to the same field. The <em>if<\/em> block has no practical use.<\/p>\n<p><strong>Issue 4<\/strong><\/p>\n<pre><code>private bool MatchesEndpoint(string absPath) {   IEnumerable&lt;RouteEndpoint> routeEndpoints = _endpointDataSource     ?.Endpoints     .OfType&lt;RouteEndpoint>()     .Where(x =>     {       ....     });    var routeValues = new RouteValueDictionary();    RouteEndpoint matchedEndpoint = routeEndpoints     .Where(e => new TemplateMatcher(         TemplateParser.Parse(e.RoutePattern.RawText),         new RouteValueDictionary())       .TryMatch(absPath, routeValues))     .OrderBy(c => c.Order)     .FirstOrDefault();    return matchedEndpoint != null; }<\/code><\/pre>\n<p>PVS-Studio issued warning <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3105\/\">V3105<\/a> The &#8216;routeEndpoints&#8217; variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RoutableDocumentFilter.cs 198<\/p>\n<p>Diagnostics <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3105\/\">V3105<\/a> warns about the possibility of a <em>NullReferenceException<\/em>. <em>_endpointDataSource<\/em> is checked for <em>null<\/em> with the &#8216;?.&#8217; operator. If the* _endpointDataSource* variable still contains the <em>null<\/em> value, then <em>routeEndpoints<\/em> is also <em>null.<\/em><\/p>\n<p>It&#8217;s weird that we access <em>routeEndpoints<\/em> without the &#8216;?.&#8217; operator. As a result, if <em>routeEndpoints<\/em> is <em>null<\/em>, <em>NullReferenceException<\/em> will be thrown when we access this reference.<\/p>\n<p><strong>Issue 5<\/strong><\/p>\n<pre><code>public void Handle(ContentCopiedNotification notification) {   ....   if (relationType == null)   {     relationType = new RelationType(       Constants.Conventions.RelationTypes.RelateDocumentOnCopyAlias,       Constants.Conventions.RelationTypes.RelateDocumentOnCopyName,       true,       Constants.ObjectTypes.Document,       Constants.ObjectTypes.Document);      _relationService.Save(relationType);   }   .... } <\/code><\/pre>\n<p>PVS-Studio warning: <a href=\"https:\/\/pvs-studio.com\/en\/w\/v3066\/\">V3066<\/a> Possible incorrect order of arguments passed to &#8216;RelationType&#8217; constructor. RelateOnCopyNotificationHandler.cs 32<\/p>\n<p>In this case, the constructor is called, and arguments are passed to it. Let&#8217;s look at its signature:<\/p>\n<pre><code>public RelationType(string name,                     string alias,                     bool isBidrectional,                     Guid? parentObjectType,                     Guid? childObjectType) <\/code><\/pre>\n<p>Looks like the arguments are passed in the wrong order. The <em>RelateDocumentOnCopy<\/em><strong><em>Alias<\/em><\/strong> argument is passed to the <em>name parameter of the constructor. The RelateDocumentOnCopy**Name<\/em> is passed to the <em>alias<\/em> parameter.<\/p>\n<p><strong>Issue 6<\/strong><\/p>\n<pre><code>private static async Task&lt;Attempt&lt;UrlInfo>> DetectCollisionAsync(....) {   ....   if (pcr.IgnorePublishedContentCollisions)   {     logger.LogDebug(logMsg, url, uri, culture);   }   else   {     logger.LogDebug(logMsg, url, uri, culture);   } }<\/code><\/pre>\n<p>PVS-Studio warning:<a href=\"https:\/\/pvs-studio.com\/en\/w\/v3004\/\"> V3004<\/a> The &#8216;then&#8217; statement is equivalent to the &#8216;else&#8217; statement. UrlProviderExtensions.cs 274<\/p>\n<p>The analyzer has found a construction where branches <em>then<\/em> and <em>else<\/em> are identical. The same code is executed regardless of the property value checked. Most likely, the developer copied the code and forgot to fix the method parameters.<\/p>\n<figure class=\"\"><figcaption><\/figcaption><\/figure>\n<p><strong>Issue 7<\/strong><\/p>\n<pre><code>public async Task&lt;bool> IsMemberAuthorizedAsync(....) {   ....   if (IsLoggedIn() == false)   {     allowAction = false;   }   else   {      string username;     ....     username = currentMember.UserName;     IList&lt;string> allowTypesList = allowTypes as IList&lt;string> ??                                                allowTypes.ToList();     if (allowTypesList.Any(allowType => allowType != string.Empty))     {       allowAction = allowTypesList.Select(x => x.ToLowerInvariant())<\/code><\/pre>\n<\/div>\n<\/div>\n<\/div>\n<\/div>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[],"tags":[],"class_list":["post-399345","post","type-post","status-publish","format-standard","hentry"],"_links":{"self":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/399345","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=399345"}],"version-history":[{"count":0,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/399345\/revisions"}],"wp:attachment":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=399345"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=399345"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=399345"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}