TOCTOU race in refreshServer (serversInfo.go)

[hklcf]

Bug Description

There is a TOCTOU (Time-of-Check-Time-of-Use) race condition in refreshServer in serversInfo.go. The initial check for whether a server exists in serversInfo.inner is done under RLock, but the actual insertion into the slice is done later under a separate Lock. Between these two lock acquisitions, another goroutine could add the same server, leading to duplicate entries.

Affected Code

serversInfo.go:207-244:

func (serversInfo *ServersInfo) refreshServer(proxy *Proxy, name string, stamp stamps.ServerStamp) error {
    serversInfo.RLock()
    isNew := true
    for _, oldServer := range serversInfo.inner {
        if oldServer.Name == name {
            isNew = false
            break
        }
    }
    serversInfo.RUnlock()
    // ... fetchServerInfo (no lock held, takes time) ...
    isNew = true
    serversInfo.Lock()
    // check again under write lock
    for i, oldServer := range serversInfo.inner {
        if oldServer.Name == name {
            serversInfo.inner[i] = &newServer
            isNew = false
            break
        }
    }
    serversInfo.Unlock()
    if isNew {
        serversInfo.Lock()
        serversInfo.inner = append(serversInfo.inner, &newServer)
        serversInfo.Unlock()
    }
}

Impact

The race can be triggered when refreshServer is called concurrently from two paths:

1. The periodic cert refresh goroutine (refresh() → refreshServer())
2. The ODoH key update path in query processing (processODoHQuery() → refreshServer())

Both goroutines can pass the initial RLock check before either acquires the Lock, resulting in duplicate ServerInfo pointers in serversInfo.inner.

Fix

Remove the first RLock check entirely and use only the second Lock-based check. The isNew variable can be determined solely from the write-locked section:

func (serversInfo *ServersInfo) refreshServer(proxy *Proxy, name string, stamp stamps.ServerStamp) error {
    isNew := true
    serversInfo.RLock()
    for _, oldServer := range serversInfo.inner {
        if oldServer.Name == name {
            isNew = false
            break
        }
    }
    serversInfo.RUnlock()
    newServer, err := fetchServerInfo(proxy, name, stamp, isNew)
    if err != nil {
        return err
    }
    newServer.rtt = ewma.NewMovingAverage(RTTEwmaDecay)
    newServer.rtt.Set(float64(newServer.initialRtt))
    serversInfo.Lock()
    found := false
    for i, oldServer := range serversInfo.inner {
        if oldServer.Name == name {
            serversInfo.inner[i] = &newServer
            found = true
            break
        }
    }
    if !found {
        serversInfo.inner = append(serversInfo.inner, &newServer)
        proxy.serversInfo.registerServer(name, stamp)
    }
    serversInfo.Unlock()
    return nil
}