devLap

PVS-Studio for Go: Quick overview of new diagnostic rules

go dev.to

PVS-Studio 8.0 is here! We added support for Go project analysis and much more. This article discusses some of the new diagnostic rules for Go that we developed for the latest version of our analyzer.

Introduction

PVS-Studio 8.0 officially introduced support for analyzing Go projects, as well as the option to work directly from GoLand and Visual Studio Code when using Go.

The Visual Studio Code plugin also received a major overhaul. It now supports Go project analysis and all the new analyzers introduced in PVS-Studio 8.0. You can learn about other new features in the press release.

The new release also added 40 new diagnostic rules for Go to the analyzer. In this article, we'll take a look at some of the most interesting ones, explore different use cases, and compare them with traditional tools. Enjoy the read!

Classic errors

We'll start with something that isn't specific to the Go language. Instead, let's focus on more universal and familiar territory: the mistakes that anyone can make when coding or communicating, regardless of the language used.

I should mention that our approach to the "typical developer mistakes" is somewhat different. In addition to studying Go projects, developer forums, and other resources, we used our "secret weapon" to develop diagnostic rules that detect common issues.

That's 18 years of experience searching for issues in code :)

Thanks to this experience in developing analyzers for C, C++, C#, and Java, we've built solid expertise in detecting common mistakes that developers can make regardless of the language they use. This enables us to consider more error cases and scenarios when designing a new analyzer.

Mistakes like typos, copy-paste errors, and other patterns inherent to human behavior may seem simple...

Let's look at a few examples.

Accessing by constant index within a loop

The example:

for i := 0; i < n; i++ {
    sum += arr[0]     // <= It should most likely be arr[i]
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning: V8008. Suspicious access to a collection element by a constant index inside a loop.

This diagnostic rule is interesting because the issue isn't a syntax error, but rather a logical one—a mistake that developers of any skill level can make.

The index is syntactically valid, so the compiler doesn't report the error, but it remains. Situations like "Oops, I forgot to change i to j" or "I forgot that i even existed" are far more common than you might think. At this point, it becomes a pattern rather than an isolated issue.

We cover this because we know firsthand that developers make such mistakes! Each of our "classic" analyzers has equivalent rules (V3102 is for C#, V767 is for C++, and V6016 is for Java) and includes examples of such mistakes found in popular open-source projects. So, these issues shouldn't be underestimated. You can find examples of errors here:

Recurring condition check

The example:

if A == B {
  if A == B {
    ....
  }
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning: V8020. Recurring check. This condition was already verified on a previous line.

As you can see, I wasn't lying when I said these mistakes look simple... almost too simple.

And that's the scary part! You get this strong feeling, "I'd never, ever make a mistake like that. I'm not that silly." Then a fly buzzes by, a bug slips through, a colleague walks over, and suddenly there's a couple of checks doing the same thing.

Here's a real-world example from the Incus project, which we covered in one of our articles:

err = p.Save(pidPath)
if err != nil {
    err2 := p.Stop()
    if err != nil {        // <= It looks like it should be err2
        return fmt.Errorf("...: %s: %s", err, err2)
    }
    ...
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning: V8020 Recurring check. The 'err != nil' condition was already verified on line 407 proxy.go 407

The second condition should check err2 instead of err, as err has already been checked and hasn't changed since.

However, there's more to it because err2 appears in the error handling:

return fmt.Errorf("....: %s: %s", err, err2)
Enter fullscreen mode Exit fullscreen mode

At first, you might not see anything unusual about this context. Although, the answer is obvious: if a problem occurs while handling errors, it becomes more difficult to detect and fix them.

Here's one more point worth mentioning. Go's error-handling philosophy follows a simple principle: "an error is a value." The caller must explicitly handle an ordinary value that a function explicitly returns. Such code is common and boilerplate, so it's easy to make mistakes with it!

We designed one of our new diagnostic rules with this particular aspect of Go in mind:

V8023. It is possible that a wrong variable of the 'error' type is checked for 'nil'.

The example:

func processOrder(orderID string) error {
    order, err := fetchOrder(orderID)
    if err != nil {
        return fmt.Errorf("fetch order: %w", err)
    }

    payment, errPay := chargePayment(order)
    if err != nil {          
        return fmt.Errorf("charge payment: %w", err)
    }

    return savePayment(payment)
}
Enter fullscreen mode Exit fullscreen mode

Now, let's return to the topic of common developer mistakes and discuss copy-pasting. You can notice that err is used instead of errPay in the second case. The result of chargePayment is assigned to a different variable, but developers forgot to change the variable name.

It's easy to spot most of the errors in the context of this article. However, during a code review or while writing code, when we're not looking at a single function but rather going through hundreds or thousands of code lines without a break, it's easy to miss such issues.

We discussed this and other issues related to error handling in Go in the "Error handling in Go: Common pitfalls" article.

The parameter is overwritten before it is used

The example:

func Translate(value *T, numericValue int) {
    numericValue = reader.ReadInt32()   // <= the initial parameter value 
                                        //    isn't used anywhere
    ...
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning: V8038. A parameter is always rewritten in the function body before being used.

This is a rather unfortunate error because the value passed to the function parameter is never read; it's immediately overwritten inside the function body.

Technically, this isn't a compilation error, so everything should work as expected. However, it usually indicates one of two things: either the input value wasn't used, or the variables got mixed up during refactoring.

Basic tools aren't always enough

Having a nice set of tools right out of the box—and, of course, various open-source solutions—is always great, but they might not be enough for more demanding tasks.

Let's take the very first diagnostic rule of our Go analyzer as an example:

V8001. Identical sub-expressions to the left and to the right of the 'foo' operator.

Look at the example:

func rgb1(r float32, g float32, b float32) {
  if r > 1 || g > 1 || r > 1 {
    ....
  }
}
Enter fullscreen mode Exit fullscreen mode

The error lies in the identical sub-expressions within the binary expression. This issue may arise from carelessly copying code. Such errors are common and nothing out of the ordinary; so, go vet can spot them.

However, things aren't always that simple... Let's take a look at this code:

func rgb(r float32, g float32, b float32) {
  if r > 1 || g > 1 || 1 < r {
    ....
  }
}
Enter fullscreen mode Exit fullscreen mode

The only change is that 1 and r have switched places. Here's the catch, though: go vet doesn't issue any warnings, but the problem remains, since the r > 1 sub-expression is equivalent to 1 < r.

Impossible type assertion

Here's another case involving the V8035 diagnostic rule: an impossible type assertion.

Check out this example:

var v interface {
    Read()
    Read2()
    Read3()
    Read4()
    Read5()
}
_ = v.(io.Reader)
Enter fullscreen mode Exit fullscreen mode

The idea is simple: v is declared as an anonymous interface with the Read() method, whose signature doesn't match Read([]byte) (int, error) from io.Reader/io.ReadCloser (in the example, Read() has no arguments).

So, no specific type can implement both the v anonymous interface and io.Reader/io.ReadCloser simultaneously. So, this implementation guarantees that assertion will throw a panic at runtime. The compiler doesn't catch this because it's technically valid Go code, and everything runs smoothly.

Now wait for it... Go vet can detect it! This is a well-known and well-documented issue, so it should come as no surprise.

By the way, PVS-Studio issues the following message for such code:

The PVS-Studio warning: V8035. Type assertion is always false. Check the 'interface{Read(); Read2(); Read3(); Read4(); Read5()}' and 'io.Reader' interfaces, as they contain methods with incompatible signatures.

Here's another example:

var v interface {
    Read()
}

switch v.(type) {
case io.ReadCloser:
    fmt.Println(1)
default:
    fmt.Println(2)
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning: V8035. Type assertion is always false. Check the 'interface{Read()}' and 'io.ReadCloser' interfaces, as they contain methods with incompatible signatures.

In fact, it's exactly the same impossible assertion. The only difference is that the assignment is now done using case in the switch statement and via v.(type), rather than via v.(io.ReadCloser).

This is where I'm supposed to say that go vet can't detect it... but no! It can, but that's not the point here. A popular open-source analyzer won't detect the issue here, but it will spot it in the first case. This is a false negative that occurs when a warning isn't issued when it should be.

This presents a dilemma: you can either look for a tool that covers all use cases and spend time trying out different options, or you can take a closer look at PVS-Studio. It's designed for in-depth analysis and comes with guaranteed support for many years to come.

If you have any doubts about our claims, feel free to try our tool and draw your own conclusions.

Well, enough with the synthetic code. Will we show some real code, though?

Some real code

Do you remember the very first Go analyzer diagnostic rule I covered at the beginning of this section? Here's a real example of the warning from the Nuclei project:

func NewEntityParser(dir string) (*EntityParser, error) {

  cfg := &packages.Config{
    Mode: packages.NeedName | packages.NeedFiles | packages.NeedImports |
      packages.NeedTypes | packages.NeedSyntax | packages.NeedTypes |
      packages.NeedModule | packages.NeedTypesInfo,
    Tests: false,
    Dir:   dir,
    ParseFile: func(....) (*ast.File, error) {
      return parser.ParseFile(fset, filename, src, parser.ParseComments)
    },
  }
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning: V8001 There are identical sub-expressions 'packages.NeedTypes' to the left and to the right of the '|' operator. parser.go 33

If we take a closer look, we can see that the packages.NeedTypes flag is pointlessly repeated here. This error may have occurred due to code autocompletion or a typo. Most likely, there should be the packages.NeedTypesSizes flag here:

const (
  ....
  // NeedTypes adds Types, Fset, and IllTyped.
  NeedTypes

  // NeedSyntax adds Syntax and Fset.
  NeedSyntax

  // NeedTypesInfo adds TypesInfo and Fset.
  NeedTypesInfo

  // NeedTypesSizes adds TypesSizes.
  NeedTypesSizes

  ...
)
Enter fullscreen mode Exit fullscreen mode

It might seem like a simple mistake, but the standard go vet won't catch it. Yet the open-source tool I mentioned in the previous case study does find it. So, I'd like to reiterate the issue: go vet sometimes finds problems that other tools don't, and vice versa.

We discussed this and other errors that the standard go vet can't detect in the "Go vet can't go: How PVS-Studio analyzes Go projects" article.

Go-specific issues

Of course, Go has its own pitfalls. Even seasoned Go developers may not be familiar with all of the language's unique features and tricky parts (not to mention those coming from other languages).

For such cases, our comprehensive documentation provides detailed explanations of why this or that error occurred. We also have examples of common mistakes and how to fix them :)

Well, let's look at a couple of diagnostic rules.

recover() inside an anonymous function that isn't wrapped in defer

In Go, recover resumes execution only if called inside a deferred (defer) function.

However, if an anonymous function with recover is called immediately as (func() { ... }()), by the time a potential panic occurs, that call will have long since completed—recover is physically unable to catch it. So, if a panic occurs, the goroutine can't resume execution.

Here's the example:

func foo() {
    func() {
        if r := recover(); r != nil {
            fmt.Println("Recovered", r)
        }
    }()                        // <= recover started and finished immediately
    q, r := bits.Div64(hi, lo, y) // panic, recover won't help here anymore
}
Enter fullscreen mode Exit fullscreen mode

After defining the anonymous function and calling it, the bits.Div64 function is called. This function can trigger a panic if y equals 0. If that panic reaches the top of the stack, the program will crash.

Of course, the documentation explains how to fix this: the anonymous function call needs to be deferred using the defer keyword:

func foo() {
  defer func() {
    if r := recover(); r != nil {
      fmt.Println("Recovered", r)
    }
  }()
  q, r := bits.Div64(hi, lo, y)
}
Enter fullscreen mode Exit fullscreen mode

For more details, refer to the documentation.

They couldn't have made such a mistake, right?

Here's an error so interesting that we wrote an article about it titled "One mistake no one has ever made. Right?"

Since the article describes it pretty thoroughly and includes examples from real projects, I'll just give a brief overview:

func SimpleExample(arr []int) {
  _ = arr[len(arr)]
}
Enter fullscreen mode Exit fullscreen mode

Collection indices always start at 0. So, for a slice of the n length, the valid indices range from 0 to n – 1. Then len(arr) returns n. As a result, accessing arr[len(arr)] always leads to a program panic.

Here's an interesting thing: when developing this diagnostic rule, we thought it was "far-fetched" and that "no one would make a mistake like that" because any test would catch it. But the results were fascinating! You can check them out in the article.

Confusing XOR (^) with exponentiation

Here's another interesting Go-specific mistake.

Unlike in Lua, VB.NET, and R, in Go, ^ has nothing to do with exponentiation, which can be a trap for developers switching between languages. This mistake is exciting because, if we recall the problem with universal mistakes from the beginning of the article, it results in an unfortunate combination.

Binary expressions meant for exponentiation become incorrect when a developer chooses ^. This operator actually performs a bitwise exclusive OR.

The example is painfully simple:

x := 2 ^ 16
Enter fullscreen mode Exit fullscreen mode

In reality, the value here is 18 instead of the expected 2 to the power of 16 (65,536). As the documentation suggests, we can use the left bitwise shift operator to get the power of 2.

x := 1 << 16
Enter fullscreen mode Exit fullscreen mode

What's next?

If you're familiar with our work, then you've probably heard that PVS-Studio detects code quality issues and serves as a static application security testing (SAST) solution. This means our analyzer can also spot potential vulnerabilities.

Our new Go analyzer currently includes only a basic set of diagnostic rules that cover common code quality issues. This is just the beginning, though!

Consider this an announcement: we've already started working on SAST coverage for our Go analyzer. Stay tuned to our blog for updates :)

To back that up, let me introduce one of the first diagnostic rules in the code security section.

Trojan Source

Take a look at this code fragment:

isAuthorized := false
/* verify before transfer */ if isAuthorized {
    TransferFunds(account, amount)
/* end verify */ }
Enter fullscreen mode Exit fullscreen mode

What do you think it would look like to the compiler (without all the conventions and low-level details)?

If you think it should look like this:

isAuthorized := false
if isAuthorized {
    TransferFunds(account, amount)
Enter fullscreen mode Exit fullscreen mode

Well, you're almost right...

It looks as though the funds transfer is indeed protected by the isAuthorized check.

However, the issue lurks where we can't see it, because the compiler will interpret this as follows:

isAuthorized := false
TransferFunds(account, amount)
Enter fullscreen mode Exit fullscreen mode

It doesn't look that great anymore, does it? The isAuthorized check never runs because the if statement and {...} ended up inside what the reviewer considered to be "commented-out" text, so the TransferFunds call executes unconditionally.

The code contains special characters that may not be displayed. They can change how the code appears in the development environment. Combinations of such characters can cause a human and a compiler to interpret the code differently.

Here's what the code actually looks like:

isAuthorized := false
/*[RLO] } [LRI] if isAuthorized[PDI] [LRI] verify before transfer */  
    TransferFunds(account, amount)
/* end verify [RLO]{ [LRI]*/
Enter fullscreen mode Exit fullscreen mode

Let's briefly go over what these characters do here:

  • [LRI] if isAuthorized[PDI]: the fragment between LRI and PDI is isolated and evaluated from left to right as a single block (if isAuthorized).
  • [LRI] verify before transfer */: to the end of the line; this is also an isolated block (verify before transfer */).
  • [RLO]: reverses from right to left everything that follows it, and each of the obtained isolated blocks is treated as a single, indivisible character.

The final order is as follows:

'verify before transfer */', 'if isAuthorized', {space}, '{', {space}
Enter fullscreen mode Exit fullscreen mode

And, lo and behold! With a couple of simple gestures, the closing brace visually turns into an opening one.

This is an issue that PVS-Studio will highlight:

The PVS-Studio warning: V5901. OWASP. Code contains invisible characters that may alter its logic. Consider enabling the display of invisible characters in the code editor.

This vulnerability is listed in the OWASP standard, and our other analyzers can detect similar issues:

  • V5801 — JS\TS;
  • V5340 — Java;
  • V5629 — C#;
  • V1076 — C++.

We've covered this diagnostic rule in more detail in the documentation.

Is this the end? No, it's just the beginning!

The new analyzer for Go projects is here, but this is only the beginning of its story! The 40 diagnostic rules introduced with PVS-Studio 8.0 mark the start of that journey, and your input can make a real difference. If you're a Go developer, we invite you to try our tool for free. If you have ideas for new diagnostic rules or suggestions for improving the existing ones, feel free to reach out to us through the feedback form!

Thanks for reading. Take care of yourself and your code!

Source: dev.to

arrow_back Back to Tutorials