trailofbits.go.missing-unlock-before-return.missing-unlock-before-return

profile photo of trailofbitstrailofbits
Author
unknown
Download Count*

Missing mutex unlock ($T variable) before returning from a function. This could result in panics resulting from double lock operations

Run Locally

Run in CI

Defintion

rules:
  - id: missing-unlock-before-return
    message: Missing mutex unlock (`$T` variable) before returning from a
      function.  This could result in panics resulting from double lock
      operations
    languages:
      - go
    severity: ERROR
    metadata:
      category: security
      cwe: "CWE-667: Improper Locking"
      subcategory:
        - vuln
      confidence: MEDIUM
      likelihood: HIGH
      impact: MEDIUM
      technology:
        - --no-technology--
      description: Missing `mutex` unlock before returning from a function
      references:
        - https://pkg.go.dev/sync#Mutex
        - https://blog.trailofbits.com/2020/06/09/how-to-check-if-a-mutex-is-locked-in-go/
      license: AGPL-3.0 license
      vulnerability_class:
        - Other
    patterns:
      - pattern-either:
          - pattern: panic(...)
          - pattern: return ...
      - metavariable-pattern:
          metavariable: $T
          patterns:
            - pattern: |
                ($T : sync.Mutex)
      - pattern-inside: |
          $T.Lock()
          ...
      - pattern-not-inside: |
          $T.Unlock()
          ...
      - pattern-not-inside: |
          defer $T.Unlock()
          ...
      - pattern-not-inside: |
          defer func(...) {
            ...
            $T.Unlock()
            ...
          }(...)
          ...
      - pattern-not-inside: |
          $FOO(..., ..., func(...) { 
              ... 
          })
      - pattern-not-inside: |
          return func(...) {
              ...
              $T.Unlock()
              ...
          }

Examples

missing-unlock-before-return.go

// Modified from https://gobyexample.com/mutexes

package main

import (
	"fmt"
	"sync"
)

type Unlocker func()

type Container struct {
	mu       sync.Mutex
	counters map[string]int
}

type Fridge struct {
	food int
}

func (f *Fridge) Lock() {
	fmt.Println(f.food)
}

func main() {
	c := Container{
		counters: map[string]int{"a": 0, "b": 0},
	}

	var wg sync.WaitGroup

	doIncrement := func(name string, n int) {
		for i := 0; i < n; i++ {
			err := c.inc(name)
			if err != nil {
				continue
			}
		}
		wg.Done()
	}

	wg.Add(3)
	go doIncrement("a", 10000)
	go doIncrement("a", 10000)
	go doIncrement("b", 10000)
	go doIncrement("c", 10000)
	go doIncrement("a", 10000)
	go doIncrement("c", 10000)
	go doIncrement("a", 10000)
	go doIncrement("b", 10000)

	wg.Wait()
	fmt.Println(c.counters)
}

func (c *Container) inc(name string) error {
	c.mu.Lock()
	c.counters[name]++
	if name == "c" {
		// ruleid: missing-unlock-before-return
		return fmt.Errorf("letter not allowed")
	}
	c.mu.Unlock()
	// ok: missing-unlock-before-return
	return nil
}

func (c *Container) inc_FP(name string) error {
	c.mu.Lock()
	c.counters[name]++
	if name == "c" {
		// ok: missing-unlock-before-return
		c.mu.Unlock()
		return fmt.Errorf("letter not allowed")
	}
	c.mu.Unlock()
	// ok: missing-unlock-before-return
	return nil
}

// This could still cause a deadlock in the case that the caller function
// implements a recover() to avoid the application from crashing
func (c *Container) inc2(name string) error {
	c.mu.Lock()
	c.counters[name]++
	if name == "c" {
		// ruleid: missing-unlock-before-return
		panic("letter not allowed")
	}
	c.mu.Unlock()
	// ok: missing-unlock-before-return
	return nil
}

func (c *Container) inc2_FP(name string) error {
	c.mu.Lock()
	c.counters[name]++
	if name == "c" {
		c.mu.Unlock()
		// ok: missing-unlock-before-return
		panic("letter not allowed")
	}
	c.mu.Unlock()
	// ok: missing-unlock-before-return
	return nil
}

func (c *Container) inc3(name string) error {
	c.mu.Lock()
	c.counters[name]++
	if name == "c" {
		c.mu.Unlock()
		// ok: missing-unlock-before-return
		return fmt.Errorf("letter not allowed")
	}
	// ruleid: missing-unlock-before-return
	return nil
}

func (c *Container) inc4FP(name string) Unlocker {
	c.mu.Lock()
	c.counters[name]++
	return func() {
		// ok: missing-unlock-before-return
		c.mu.Unlock()
	}
}

func (c *Container) inc5(name string) error {
	f := Fridge{food: 11}
	f.Lock()
	// ok: missing-unlock-before-return
	return nil
}

func (c *Container) inc6(name string) error {
	c.mu.Lock()
	c.counters[name]++
	defer func() {
		fmt.Println("before unlock")
		c.mu.Unlock()
		fmt.Println("after unlock")
	}()
	// ok: missing-unlock-before-return
	return nil
}

func (c *Container) inc6b(name string) error {
	c.mu.Lock()
	unlocker := c.mu.Unlock
	c.counters[name]++
	defer func() {
		fmt.Println("before unlock")
		unlocker()
		fmt.Println("after unlock")
	}()
	// todook: missing-unlock-before-return
	return nil
}

func (c *Container) inc7(name string) error {
	c.mu.Lock()
	c.counters[name]++
	defer func(earlyExit bool) {
		fmt.Println("before unlock")
		if (earlyExit) {
			// todoruleid: missing-unlock-before-return
			return
		}
		c.mu.Unlock()
		fmt.Println("after unlock")
	}(false)
	// ok: missing-unlock-before-return
	return nil
}

func (c *Container) inc8(name string) error {
	c.mu.Lock()
	c.counters[name]++
	_, err := fmt.Println("test if")
	if err != nil {
	    c.mu.Unlock()
	    // ok: missing-unlock-before-return
	    return nil, err
	}
	// ruleid: missing-unlock-before-return
	return nil
}

func (c *Container) inc8b(name string) error {
	c.mu.Lock()
	c.counters[name]++
	unlocker := c.mu.Unlock
	_, err := fmt.Println("test if")
	if err != nil {
	    unlocker()
	    // todook: missing-unlock-before-return
	    return nil, err
	}
	// ruleid: missing-unlock-before-return
	return nil
}