Proposal: Add semgrep-go checks into validation step

[benjaminch]

Discovered recently this amazing semgrep-go rules collection put together by @dgryski here https://github.com/dgryski/semgrep-go allowing to avoid common issue patterns in Go.

The check shouldn't by blocking for the time being, but once all errors / warnings will be solved, maybe it should become blocking in case something is detected.

I will be pushing a PR to introduce this one in the workflow so we can discuss and let me know what you think about it.

Cheers ! :)

[dlukeomalley]

Hey @benjaminch! Semgrep maintainer here. @dgryski's rules are also published to the Semgrep registry if you'd like to skip the curl:

semgrep --config http://semgrep.dev/r/dgryski.semgrep-go

This is the equivalent and lets you skip the .gitignore. There is also a Semgrep GitHub Action that only displays and fails on new results, so it's easy to introduce new checks without having to fix all previous findings: https://github.com/returntocorp/semgrep-action/#readme

Would love to hear your feedback!

[benjaminch]

Hello @dlukeomalley !

Thanks a lot for your help here, really appreciate :)
I will try to use action directly, but first wanted to make this check part of the validation script so it's also available locally to contributors.

Let's see if I can make it happen !

Thanks !

[dlukeomalley]

Let me know if you run into any issues, the maintainer group loves supporting use cases like yours! 🙂

[SyntaxNode]

@benjaminch This looks very interesting. Thank you for taking the initiative.

I discovered this recently as well thanks to Grayson Hardaway's GoLang NYC talk. We're currently in the middle of revitalizing our CI/CD pipeline using GitHub Actions. This looks like a nice addition.

@mansinahar is working on adding a linter. I'm curious if you recommend this as a complimentary static analysis or a replacement to a traditional linter?

@dlukeomalley Thank you very much for the support. I'm excited to increase our code safety checks and also at the prospect of codifying our project specific rules to make PR reviews quicker. One use case I have in mind is catching writes to shared memory. Go lacks any sort of way to express read / write access, which is perhaps the thing I miss the most from C#. I imagine a rule of this kind would require flow analysis, right? Or is there a way to write a semgrep rule which catches a write to a variable which hasn't been copied first. Ex:

type Request struct {
    Site  *Site
    .. other stuff ...
}

type Site struct {
    Publisher string
}

BAD:
bidRequest.Site.Publisher = "foo"

GOOD:
siteCopy := *bidRequest.Site
siteCopy.Publisher = "foo" 
bidRequest.Site = &siteCopy

GOOD:
bidRequest.Site = &Site{Publisher: "foo"}

[benjaminch]

Hey @SyntaxNode !

@mansinahar is working on adding a linter. I'm curious if you recommend this as a complimentary static analysis or a replacement to a traditional linter?

I think it's a complimentary static analysis tool along the linter, it won't replace a linter IMO as it won't serve the same purpose.

[mansinahar]

@SyntaxNode I agree with @benjaminch that this won't replace the linter as the linter checks more for style related rules vs this seems to check for potential bugs and such.

Also, I did find a few other static analysis tools for Go like goreporter, go-critic (This has it's own set of rules and plus also supports a ruleguard integration) and staticcheck which look interesting and useful too and so wanted to throw them in the mix here as well so that we can consider them all and pick the one suited best for Prebid-Server.

I personally found goreporter the most interesting as it supports a bunch of different linters and normalizes their output but I am not sure if that project is maintained anymore :(

Looks like golangci-lint is one of the popular open-source linters for Go that supports a bunch of linters (one of them being go-critic which has a ruleguard integration as well)

My question would be, are there any semgrep-go rules that aren't covered by the myriad of linters supported by golangci-lint? And if there are, how important they are to this project?

If they are quite important then we can consider adding semgrep-go but as a starting point golangci-lint should be the most helpful IMHO.

[mansinahar]

@dlukeomalley out of curiosity, do you consider integrating semgrep-go with golangci-lint in the future?

[dgryski]

My suggestion is to start by integrating tools with little to no false positives: go vet, staticcheck, ineffassign, and nilness. Most of the other liners catch nits that are either more style issues and just generate code churn.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dmitris]

for reference, here's the output of running semgrep with the https://github.com/dgryski/semgrep-go ruleset:
https://gist.github.com/dmitris/908a85d205249f2018f4688308fb5053

I would be interested in fixing them and also adding this to the CI checks, if possible and reasonable.