Our website uses cookies to enhance your browsing experience.
Webinar: Let's make a programming language. Part 1. Intro - 20.02
Docker, Kubernetes, Gitea, and other projects—they all have one thing in common: they're written in Go. Perhaps you have now realized that this is what we are talking about today. We've never described Golang errors before, but now we're making up for it. PVS-Studio Go static analyzer is coming soon.
Static analyzers are rather common development tools, and Go has a built-in static analysis mechanism, go vet. However, standard linters don't always work as planned. For newcomers, we're PVS-Studio, a company that develops a static analyzer for C, C++, C#, and Java. Recently, we've been actively working on a Go analyzer and are planning to introduce its beta soon.
We've already written an article about how to create your own Go static analyzer, so now it's time to show how our analyzer shines in real-world projects.
The article may seem quite odd because it's our first try at writing about Go projects. In this article, we describe some diagnostic rules of the upcoming Go analyzer and also show some warnings issued for real-world projects.
Any PVS-Studio analyzer on any language—it doesn't matter C, C++, C#, Java, or Go—claims diagnostic rules because they determine the logic of certain analyzer warnings.
Before we begin reviewing the errors, let us tell you a little bit about how we develop diagnostic rules.
First, we look for ideas for diagnostic rules. We read forums, explore open-source codebases, process customer feedback, and study error patterns.
The developer then gets a task to implement specific diagnostic rules and does their own research: where the analyzer should issue warnings, and where it should stay silent? After that comes writing a large set of tests—both positive ones (where the analyzer should issue warnings) and negative ones (where it definitely shouldn't). And, of course, the diagnostic rule itself has to be implemented.
It sounds easy, but it's not that simple. The tests used during development are synthetic, and real-world projects often behave very differently. Therefore, it's necessary to test the diagnostic rule on real projects, which is why we analyze many open-source projects from GitHub. Depending on the results, developers either refine the diagnostic rule or send it for code review.
So what's the point of all this? All described warnings are taken from real projects.
With that out of the way, let's move on to the review.
The V8001 diagnostic rule warns developers when the binary expression contains identical subexpressions. This kind of issue usually shows up after a careless copy-paste—the built-in go vet linter can detect it too. For example:
func rgb1(r float32, g float32, b float32) {
if r > 1 || g > 1 || r > 1 {
....
}
}In this case, the standard tool outputs redundant or: r > 1 || r > 1. However, if we swap 1 and r and change the comparison operator, we get the following code:
func rgb(r float32, g float32, b float32) {
if r > 1 || g > 1 || 1 < r {
....
}
}Standard go vet no longer issues a warning about this code, although the problem remains: the r > 1 subexpression is equivalent to 1 < r.
A small spoiler: all potential errors in the article slip past the built-in go vet.
Let's look at an example of such an error in the Nuclei project, a vulnerability scanner that allows creating user-defined templates.
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)
},
}
}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
The packages.NeedTypes flag is repeated for no reason. Most likely, the error occurred due to code autocompletion, and in reality, 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
...
)It'd seem to be a trivial error, but it can't be detected using the standard go vet.
The analyzer detected the following code fragment in the Mediamtx media server and media proxy project using the analyzer:
func (p *Core) closeResources(....) {
....
closeRTSPServer := newConf == nil ||
newConf.RTSP != p.conf.RTSP ||
....
newConf.RTSPAddress != p.conf.RTSPAddress || // <=
....
newConf.RTSPUDPReadBufferSize != p.conf.RTSPUDPReadBufferSize ||
newConf.UDPReadBufferSize != p.conf.UDPReadBufferSize ||
newConf.ReadTimeout != p.conf.ReadTimeout ||
newConf.WriteTimeout != p.conf.WriteTimeout ||
newConf.WriteQueueSize != p.conf.WriteQueueSize ||
newConf.RTPAddress != p.conf.RTPAddress ||
newConf.RTCPAddress != p.conf.RTCPAddress ||
newConf.MulticastIPRange != p.conf.MulticastIPRange ||
newConf.MulticastRTPPort != p.conf.MulticastRTPPort ||
....
newConf.RTSPAddress != p.conf.RTSPAddress || // <=
....
newConf.RunOnConnect != p.conf.RunOnConnect ||
newConf.RunOnConnectRestart != p.conf.RunOnConnectRestart ||
newConf.RunOnDisconnect != p.conf.RunOnDisconnect ||
closeMetrics ||
closePathManager ||
closeLogger
}The PVS-Studio warning: V8001 There are identical sub-expressions 'newConf.RTSPAddress != p.conf.RTSPAddress' to the left and to the right of the '!=' operator. core.go 790
Would you spot such an error yourself, even in this shortened example? Yes, code review is undoubtedly a powerful way to find bugs, but it may not always be able to help. Besides, even a built-in tool can't handle everything!
A similar issue was also found in Tidb:
func ProcessModifyColumnOptions(....) error {
var sb strings.Builder
restoreFlags := format.RestoreStringSingleQuotes |
format.RestoreKeyWordLowercase |
format.RestoreNameBackQuotes |
format.RestoreSpacesAroundBinaryOperation |
format.RestoreWithoutSchemaName | // <=
format.RestoreWithoutSchemaName // <=
restoreCtx := format.NewRestoreCtx(restoreFlags, &sb)
....
}The PVS-Studio warning: V8001 There are identical sub-expressions 'format.RestoreWithoutSchemaName' to the left and to the right of the '|' operator. modify_column.go 2073
Although this project is popular enough (40,000 stars on GitHub), there are still trivial errors in the codebase.
In this project, the analyzer detected two instances of the same error:
func (s *Store) GetStorePluginManifests(....) []StorePluginManifest {
....
for _, manifest := range pluginManifest {
existingManifest, found := lo.Find(storePluginManifests,
func(manifest StorePluginManifest) bool {
return manifest.Id == manifest.Id
})
if found {
....
}
storePluginManifests = append(storePluginManifests, manifest)
}
}
}The PVS-Studio warning: V8001 There are identical sub-expressions 'manifest.Id' to the left and to the right of the '==' operator. store.go 100
Comparing manifest.Id with identical subexpression doesn't make sense. In our case, the manifest name is used both for iterating through pluginManifest:
for _, manifest := range pluginManifest { .... }The same applies to the parameters for the function:
func(manifest StorePluginManifest) bool { .... }The intent was to compare the Id field of elements from pluginManifest with the same field of the anonymous function parameter. Instead, it turned out that identical fields of the same parameter are being compared:
func(manifest StorePluginManifest) bool {
return manifest.Id == manifest.Id
}There is the same error in another place:
func (s *Store) GetStoreThemes(ctx context.Context) []common.Theme {
....
for _, manifest := range themeManifest {
_, found := lo.Find(storeThemeManifests,
func(manifest common.Theme) bool {
return manifest.ThemeId == manifest.ThemeId // <=
})
if found {
//skip duplicated theme
continue
}
storeThemeManifests = append(storeThemeManifests, manifest)
}
....
}The PVS-Studio warning: V8001 There are identical sub-expressions 'manifest.ThemeId' to the left and to the right of the '==' operator. store.go 72
When we posted this article, the errors had been fixed in this commit and in this one. Although judging by the changelog, the errors persisted for about three years.
These kinds of issues can be avoided entirely by using static analysis regularly, for example, by integrating it into a quality gate before each commit.
Most format string errors are caused by a mismatch between the number of arguments and the number of placeholders. However, we found this bug in the Gitea project:
func DropTableColumns(...) (err error) {
switch {
....
case setting.Database.Type.IsMSSQL():
....
for _, constraint := range constraints {
if _, err := sess.Exec(fmt.Sprintf("DROP INDEX `%[2]s` ON `%[1]s`",
tableName, constraint)); err != nil {
return fmt.Errorf("Drop index `%[2]s` on `%[1]s`: %v",
tableName, constraint, err)
}
}
....
}
....
}The PVS-Studio warning: V8013 Incorrect format. A different number of format items is expected while calling the 'fmt.Errorf' function. Expected: 2, present: 3. db.go 469
Let's look at this format string with piercing eyes:
"Drop index `%[2]s` on `%[1]s`: %v"Seems fine, yeah? And the number of arguments matches the number of placeholders in the string. It's assumed that %[2]s refers to the constraint variable, since there is a [2] index, %[1]s will correspond to tableName, and the last remaining %v will correspond to the err variable. But in reality, instead of err, the value constraint will be substituted for %v.
Why does this happen?
In Go, we can directly specify the argument that should be used in the actual placeholder. Note that next argument number for substitution is evaluated based on the number of the previous substitution argument, except for the first argument. Therefore, if we directly specify that the n argument should be substituted, the n + 1 will be substituted after it.
In this case, %[1]s points to the first argument, tableName, and implicitly shifts the argument number for the next placeholder by 1 from the current one—so the second argument, constraint, is used instead of the intended err.
The if A { } else if A { } pattern often indicates a copy-paste error because the second A condition is always false during the check, and so, the code within the then branch will never be executed.
We found this in the Glance project:
func parseCliOptions() (*cliOptions, error) {
var args []string
args = os.Args[1:]
if len(args) == 0 {
intent = cliIntentServe
} else if len(args) == 1 {
....
} else if len(args) == 2 {
if args[0] == "password:hash" {
intent = cliIntentPasswordHash
} else {
return nil, unknownCommandErr
}
} else if len(args) == 2 {
if args[0] == "mountpoint:info" {
intent = cliIntentMountpointInfo
} else {
return nil, unknownCommandErr
}
} else {
return nil, unknownCommandErr
}
}The PVS-Studio warning: V8004 The use of 'if A {...} else if A {...}' pattern was detected. Potential logical error is present. Check lines: 92, 86. cli.go 92
Here's another case of copy-paste gone wrong: the then branches of both if statements are very similar, but most likely, someone copied and pasted them and forgot to change the second condition.
A similar error was found in the Havoc framework codebase:
var (
Status = Parser.ParseInt32()
Message = make(map[string]string)
)
logger.Debug(fmt.Sprintf(....))
if Status == INJECT_ERROR_SUCCESS {
Message["Type"] = "Good"
Message["Message"] = "Successful injected shellcode"
} else if Status == INJECT_ERROR_FAILED { // <=
Message["Type"] = "Error"
Message["Message"] = "Failed to inject shellcode"
} else if Status == INJECT_ERROR_INVALID_PARAM {
Message["Type"] = "Error"
Message["Message"] = "Invalid parameter specified"
} else if Status == INJECT_ERROR_PROCESS_ARCH_MISMATCH {
Message["Type"] = "Error"
Message["Message"] = "Process architecture mismatch"
} else if Status == INJECT_ERROR_FAILED { // <=
Message["Type"] = "Error"
Message["Message"] = "Failed to inject shellcode"
}The PVS-Studio warning: V8004 The use of 'if A {...} else if A {...}' pattern was detected. Potential logical error is present. Check lines: 3637, 3628. demons.go 3637
In the code snippet, the condition and the then branch of the if statements are identical, so it's possible that this is just redundant code. However, it's also possible that the second INJECT_ERROR_FAILED should be something else, and now, part of the program's logic simply doesn't work.
Indeed, the standard linter doesn't permit you to write multiple case statements with duplicated expressions, but sometimes it can fail.
For example, the standard tool will issue a warning:
const (
MONDAY = 1
TUESDAY = 2
WEDNESDAY = 3
THURSDAY = 4
FRIDAY = 5
)
switch day {
case MONDAY:
....
case TUESDAY:
....
case WEDNESDAY:
....
case THURSDAY:
....
case MONDAY:
....
}MONDAY is used twice as an expression for case.
But here, go vet won't find any errors:
switch {
case day == MONDAY:
....
case day == TUESDAY:
....
case day == WEDNESDAY:
....
case day == THURSDAY:
....
case day == MONDAY:
....
}However, the V8010 diagnostic rule of PVS-Studio analyzer can detect various arrangements within binary expressions.
Let's take a look at this rule in real-world projects.
The following potential bug was found in the code of the popular ad blocker, AdGuardHome:
func validateVersion(current, target uint) (err error) {
switch {
case current > target:
return fmt.Errorf("unknown current schema version %d", current)
case target > LastSchemaVersion:
return fmt.Errorf("unknown target schema version %d", target)
case target < current:
return fmt.Errorf("target schema version %d lower than current %d",
target, current)
default:
return nil
}
}The PVS-Studio warning: V8010 Two or more case branches have equivalent expressions. migrator.go 95
It's very suspicious code: at first glance, it seems that assignment operator caused a fuss, and in the last case, we should get target > current. However, judging by the line "target schema version %d lower than current %d" inside fmt.Errorf in the body of this case, the condition is correct.
Most likely, the condition in the first case is written incorrectly, which is very odd. Ususally, It's the other way around: the first line is correct, and developers make mistakes later (the so-called last line effect).
V8011 indicates that the same variable surrounds a compound assignment operator. This may mean that developers use a compound assignment operator (e.g., +=), and then wrote the same variable to which the assignment is made on the right side, but as if using a regular assignment operator =. For example:
x -= x - 2Obviously, there is no point in subtracting x from itself, and we can write it like this:
x = 2However, this is most likely an error, and developers intended to subtract 2 from x.
A simple example like this can be found in the Gitea project codebase:
func (s *linkifyParser) Parse(....) ast.Node {
....
if m != nil {
lastChar := line[m[1]-1]
if lastChar == '.' {
m[1]--
} else if lastChar == ')' {
....
} else if lastChar == ';' {
i := m[1] - 2
for ; i >= m[0]; i-- {
if util.IsAlphaNumeric(line[i]) {
continue
}
break
}
if i != m[1]-2 {
if line[i] == '&' {
m[1] -= m[1] – i // <=
}
}
}
}
....
}The PVS-Studio warning: V8011 Identical expression 'm[1]' to the left and to the right of compound assignment. linkify.go 108
Obviously, subtracting its own value from m[1] makes no sense. Most likely, devs would like to subtract i from m[1], but there was a typo and it turned out to be something between m[1] = m[1] – i and m[1] -= i.
A similar error was found in the same project:
func (p *Paginator) Pages() []*Page {
....
previousNum := getMiddleIdx(p.numPages) - 1
if previousNum > p.current-1 {
previousNum -= previousNum - (p.current - 1)
}
....
}The PVS-Studio warning: V8011 Identical expression to the left and to the right of compound assignment. paginator.go 177
func (c *Client) InvoiceSearch(....) (....) {
uri := searchInvoice
if page != 0 && pageSize != 0 {
uri += uri + "?page=" + // <=
strconv.Itoa(page) +
"&page_size=" +
strconv.Itoa(pageSize)
}
if totalRequired {
if page != 0 && pageSize != 0 {
uri += uri + "&total_required=true" // <=
} else {
uri += uri + "?total_required=true" // <=
}
}
....
}The PVS-Studio warnings:
V8011 Identical expression 'uri' to the left and to the right of compound assignment. invoice.go 362
V8011 Identical expression 'uri' to the left and to the right of compound assignment. invoice.go 366
V8011 Identical expression 'uri' to the left and to the right of compound assignment. invoice.go 368
The += operator is used on a string type, which is very strange to add the uri variable to itself.
Parentheses can also be a source of confusion. For example, this happens in the OpenDiablo2 project:
func (wg *WidgetGroup) adjustSize(w Widget) {
x, y := w.GetPosition()
width, height := w.GetSize()
if x+width > wg.x+wg.width {
wg.width += (x + width) - (wg.x + wg.width) // <= (1)
}
if wg.x > x {
wg.width += wg.x - x
wg.x = x
}
if y+height > wg.y+wg.height {
wg.height += (y + height) - (wg.y + wg.height) // <= (2)
}
if wg.y > y {
wg.height += wg.y - y
wg.y = y
}
}The PVS-Studio warnings:
V8011 Identical expression 'wg.width' to the left and to the right of compound assignment. widget_group.go 55
V8011 Identical expression 'wg.height' to the left and to the right of compound assignment. widget_group.go 64
Here, the errors aren't so obvious, but if we remove the brackets, we get:
wg.width += -wg.width + ....And:
wg.height += -wg.height + ....In April 2026, we plan to release an open beta version of PVS-Studio for Go. To make sure you don't miss it, we highly recommend subscribing to our newsletter.
Looking ahead, we plan to actively develop our Go analyzer, so we'd be glad if you try PVS-Studio and give us your feedback.
That's it! We've looked at a few examples of how our analyzer can work with Go projects. As we can see, even large projects have no immunity to errors. Therefore, it's important to use tools to control the code quality.
And don't forget—you can use PVS-Studio for free! If you're contributing to an open-source project (like the ones we mentioned), you can get a free license. Check out our article on free licensing for the details.
Take care of yourself and your code!
0
0
0
521
0
0