Missing (r)unlock broken after semgrep update

[GrosQuildu]

Seems like Semgrep two things in new version that break our rules:

• const propagation works in different way: the unlocker := c.mu.RUnlock is not propagated to unlocker()
• implicit return is added at the end of defer func() { block

Something to investigate and fix in our rules.

[mschwager]

Hmm, I don't think const propagation has changed. I think it's just an implicit return has been added to functions. It looks like something changed in version 1.94.0. I tested the following Go script:

package main

import "fmt"

func main() {
	defer func() {
		fmt.Println("defered")
	}()

	fmt.Println("hello")
}

Semgrep 1.93.0 gives:

$ semgrep -l go -e 'return ...' test.go
Ran 1 rule on 1 file: 0 findings.

Semgrep 1.94.0 gives:

$ semgrep -l go -e 'return ...' test.go
               
    test.go
            7┆ fmt.Println("defered")

Ran 1 rule on 1 file: 1 finding.

If I remove the defer it gives the same results, so it's not a special case of defer functions. It's also interesting that it doesn't apply the implicit return to main here either 😕

[mschwager]

I'm wondering if this was unintentional and we should file a bug upstream.

[mschwager]

It was probably this commit: semgrep/semgrep@a1764e4, which was released in version 1.94.0.

Now that I think about it, having the implicit return detection is probably a good thing. Although it may cause surprising results in some cases like this one. Realistically these two rules should probably be taint rules, but I couldn't get it to work. It was relatively straightforward to convert them to taint mode, but we need this Pro-only at-exit feature 🤷 .

I think todook is fine for now since those tests are already doing that anyway.

[GrosQuildu]

Ok, so the unlocker() was never correctly tracked in the first place, and we just were lucky before.
Merging, will fix someday.

[mschwager]

Ok, so the unlocker() was never correctly tracked in the first place, and we just were lucky before. Merging, will fix someday.

Yeah, I thought it might too, but const propagation doesn't appear to work for function references: https://semgrep.dev/playground/s/6J7rW