SonarQube scan doesn't report all uninitialized variables

Must-share information (formatted with Markdown):

  • which versions are you using (SonarQube, Scanner, Plugin, and any relevant extension)
    SonarQube Community EditionVersion 7.7 (build 23042)
    The rule “Variables should be initialized before use” is enabled for the quality profile that’s being used.

  • what are you trying to achieve
    The scan doesn’t report all initialized variables in PHP, for example this piece of code, the rule is triggered for the first method, but not for the second ($blabla is uninitialized).

      <?php
      declare(strict_types=1);
    
      namespace Matchmaker;
    
      class ClassShouldFailSonarQubeScan
      {
          public function fun($condition)
          {
              $res = 1;
              if ($condition) {
                  $res++;
              }
              return $result; // Noncompliant, "$result" instead of "$res"
          }
    
          /**
           * @param DocumentGroup $documentGroup
           *
           * @return string
           */
          public function getDocumentGroupName(DocumentGroup $documentGroup): string
          {
              if (!$documentGroup->parentId) {
                  return $documentGroup->name;
              }
    
              return $documentGroup->getDocumentGroupPath($blabla) . ' / ' . $documentGroup->name;
          }
      }
    
  • what have you tried so far to achieve this
    I’ve tried using different names, changing private method to public. Neither of those fix the issue.

Am I overlooking something or is the scanner not doing what it should be doing?

This is probably a false negative and it’s not an easy one.
To determine whether an issue should be raised or not in the last return statement, we would need to

  • Determine what’s the method which is called in $documentGroup->getDocumentGroupPath($blabla)
  • Check whether the first parameter of the method is passed by reference: if the method was declared with something like public function getDocumentGroupPath(&$p) then we shouldn’t raise an issue.

That would require a cross-file analysis which we didn’t implement and which is far from trivial.
To avoid false positives, we prefer not to report any issue in such cases.

Jumping in here because I noticed this issue on 7.7 myself.

A bit confused by the response.

In the first case I see no complications, $result is used locally and is not declared. It should throw an error.

In the second case, could you explain why it matters if $documentGroup::getDocumentGroupPath() is declared with a byref parameter or not please?

As I see it, regardless of whether $blabla is being passed byval or byref it is still use of a local scope undeclared variable, and so should throw an error? Am I just missing something obvious?

In the first case, the rule raises an issue, as @pieter12build wrote:

About the second case, the parameter definition is important because we don’t want to raise a false positive on the following code:

function foo($pattern, $value)
{
    // $matches is not initialized at this point, and it's not a problem
    if (!preg_match($pattern, $value, $matches)) { 
        return null;
    }
    return $matches[0];
}

$matches is a local variable and is not initialized when it is passed to preg_match but we shouldn’t raise an issue on that case because the 3rd parameter of preg_match is passed by reference (check the documentation of preg_match), so $matches is not evaluated at that point.

I see what you’re saying… but sort of not…

It’s perfectly possible in php to use an uninitalized variable, byref or byval. See https://www.php.net/manual/en/language.variables.basics.php

But the point is that this is bad practice and likely a bug, certainly a code smell. This is the case whether the variable is passed byval or byref.

From my perspective the fact that you can do it, doesn’t mean you should do it. Regardless of whether the variable is being passed byval or byref.

To me, the final example you gave would not be a false positive. $matches should be initialized. Sure it works,… but so does use of an uninitialized var passed byval… it doesn’t mean you should actually do it.

function foo($pattern, $value)
{
    // At this point $subject is not byref so it IS a problem?
    if (!preg_replace('#.#', '', $subject)) { 
        return null;
    }
    // $matches is byref... but why isn't it a problem? It's still a code smell. It should be initialized.
    if (!preg_match($pattern, $value, $matches)) { 
        return null;
    }
    return $matches[0];
}

Perhaps this comes down to personal opinion. But it seems like if there’s one rule for byval and another rule for byref, then there should be proper justification for the different rules. Why is one acceptable and the other not?

The rule we are talking about is categorized as a “bug”, and I believe that its current implementation only raises issues which are uncontroversial. I think it has a high value.

Changing its behavior so that it raises issues on calls to any function would significantly decrease that value. We would have to consider it as a code smell and we would have to remove it from the default quality profile because, as you put it, it would come down to personal opinion.

We could consider having a separate code smell rule to raise the issues you would like to see. However, it would not be in the default quality profile, and very few people would find it and use it.

The real improvement would be the cross-file analysis which I described in my first message: it would remove the false negatives on uninitialized arguments without raising false positives. Such analysis is far from trivial but we already developed something similar to detect injection vulnerabilities. It would definitely have a big value, but it’s not in our short-term priorities.

1 Like