Detect some TOCTTOU bugs

Time-of-check to time-of-use (aka “TOCTTOU”, “TOCTOU”, or “TOC/TOU”) is a class of race condition bugs that can be observed in almost any language. Some basic examples in Go would be:

// BAD:  Checks if the file is there before opening it instead of catching
// os.ErrNotExist with errors.Is.  File may be deleted between the call to
// os.Stat and the call to os.Open.
_, err = os.Stat(fileName)
if err == nil {
        f, err = os.Open(fileName)
        if err == nil {
                fatal()
        }
}

And:

// BAD:  lockedX could change between the first mu.Unlock() and the second
// mu.Lock().  Either lock the whole method or use the atomic.CompareAndSwap*
// family of functions.
var x int
mu.Lock()
x = lockedX
mu.Unlock()

if x == 0 {
        mu.Lock()
        lockedX = newX
        mu.Unlock()
}

Detecting such bugs in general is probably an impossible task (although there seems to have been some work put into it), but I think that these two patterns could be detected reliably at least within one function.

We can check for the first pattern, but

I sincerely hope Gophers don't actually do this
I'm not certain it'll be free of false positives. There might be scenarios where this IS the desired behavior? Will have to check on an actual corpus to find out.

I don't think we can do much for the second pattern, as we'll have to allow arbitrary code between the first and second access of the guarded variable, which makes it far too likely to introduce false positives.

I'll give the linked to paper a read once time permits.

I sincerely hope Gophers don't actually do this

How about the Go Team :-) ?

https://github.com/golang/tools/blob/6d45e3d999c13387b54fe2b35939efb993dda8e3/internal/lsp/lsprpc/lsprpc.go#L328

A quick grep shows that golang.org/x/tools has a lot of suspicious os.Stat calls, although most of them are in tests and gen.go scripts.

Another one, from modernc.org/ql, also found with grep, although this one is a bit trickier:

https://gitlab.com/cznic/ql/-/blob/08f6e2127ceae9291763c7db60c9c6ca683b3cef/helper/helper.go#L273

And, let's just say that I haven't filed this issue out of nowhere, heh. Although the code that motivated me to do so had the first part and the second part in neighbouring functions. Overall, it seems like at least half of all os.Stat calls that ignore the main result are at least a little bit suspicious.

Another one I found in real code several times, reduced:

type frobulator struct {
        // mu protects m.
        mu *sync.RWMutex
        m  map[int]int
}

func (f *frobulator) frobulate() (n int) {
        f.mu.RLock()
        if f.m == nil {
                f.mu.RUnlock()

                return 0
        }

        // A silly attempt at “upgrading” a read lock into a write one.
        // Clearly a TOCTTOU.
        f.mu.RUnlock()
        f.mu.Lock()
        defer f.mu.Unlock()

        f.m[123] = 42

        return f.m[456]
}

I'm pretty sure that about 95 % of all RUnlocks followed by a Lock are bugs, but perhaps there are weird cases where they aren't. The cases where the same data is manipulated are almost guaranteed to be bugs, imo.

Navigation Menu

Repository navigation

Detect some TOCTTOU bugs #946

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Detect some TOCTTOU bugs #946

Description

Activity

dominikh commented on Mar 16, 2021

ainar-g commented on Mar 16, 2021

ainar-g commented on Jun 21, 2021

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions