Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Go] support for select blocks and matching cases inside of those #2847

Closed
hex0punk opened this issue Mar 26, 2021 · 6 comments · Fixed by #2853
Closed

[Go] support for select blocks and matching cases inside of those #2847

hex0punk opened this issue Mar 26, 2021 · 6 comments · Fixed by #2853
Assignees
Labels
enhancement New feature or request lang:golang priority:low user:external requested by someone outside of r2c

Comments

@hex0punk
Copy link

Is your feature request related to a problem? Please describe.
Currently, there is no clean way to write a pattern for this:

func request() {
    c := make(chan int)
    for i := 0; i < 5; i++ {
        i := i
        go func() {
            select {
              case c <- i:
              default:
            }
        }()
    }
}

Describe the solution you'd like
I am interested in finding where the channel send occurs, but because it occurs as a case inside of a select block, there is no way to detect that in semgrep without getting a Pattern could not be parsed as a Golang semgrep pattern error

I would like to specify a pattern like this

  pattern: |
    go func(...) {
      ...
      select {
        case $CHANNEL <- $VAR
      }
      ...
    }(...)
    ...

or even this

    go func(...) {
      ...
      case $CHANNEL <- $VAR
      ...
    }(...)
    ...

to detect where data is sent to a go channel as a case inside select blocks

Describe alternatives you've considered
This won't work where channel sends occur as cases

rules:
- id: test
  pattern: |
    go func(...) {
      ...
      $CHANNEL <- $VAR
      ...
    }(...)
    ...
  message: |
    test
  severity: WARNING

Additional context
This is a very common concurrency pattern in go

@aryx
Copy link
Collaborator

aryx commented Mar 26, 2021

I think this is better (which you suggested on slack):

rules:
- id: test
  patterns:
  - pattern: |
        go func(...) {
          ...
          select {
             ... 
             case $CHANNEL <- $VAL
             ...
          }
          ...
        }(...)
        ...
  message: test
  languages: [go]
  severity: WARNING

@hex0punk
Copy link
Author

I think this is better (which you suggested on slack):

rules:
- id: test
  patterns:
  - pattern: |
        go func(...) {
          ...
          select {
             ... 
             case $CHANNEL <- $VAL
             ...
          }
          ...
        }(...)
        ...
  message: test
  languages: [go]
  severity: WARNING

yep, agreed

@aryx aryx self-assigned this Mar 26, 2021
@ievans
Copy link
Member

ievans commented Mar 26, 2021

hi @hex0punk, thanks so much for creating an issue! can you share the relative priority of this for you -- is it blocking your adoption of semgrep (p0), a significant problem to using semgrep for you (p1), or a minor annoyance (p2)?

playground link: https://semgrep.dev/s/Nr4L

@ievans ievans added enhancement New feature or request user:external requested by someone outside of r2c labels Mar 26, 2021
@hex0punk
Copy link
Author

hi @hex0punk, thanks so much for creating an issue! can you share the relative priority of this for you -- is it blocking your adoption of semgrep (p0), a significant problem to using semgrep for you (p1), or a minor annoyance (p2)?

playground link: https://semgrep.dev/s/Nr4L

for sure!

Priority: (p2)
Reason: not a significant problem at the moment, just working on new semgrep rules while I have time. It is a common pattern in go though.

@aryx
Copy link
Collaborator

aryx commented Mar 30, 2021

link https://semgrep.dev/s/nqqg/

aryx added a commit to semgrep/pfff that referenced this issue Mar 30, 2021
aryx added a commit that referenced this issue Mar 30, 2021
This closes #2847

test plan:
test files included
make test
aryx added a commit that referenced this issue Mar 30, 2021
This closes #2847

test plan:
test files included
make test
@aryx
Copy link
Collaborator

aryx commented Mar 30, 2021

This works now on develop: https://semgrep.dev/s/nqqg/?version=develop
Note that you need to use 'case ...' inside a select/switch, and not just '...' because
of ambiguities otherwise in the grammar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lang:golang priority:low user:external requested by someone outside of r2c
Development

Successfully merging a pull request may close this issue.

3 participants