code review 148370043: os, syscall: add Unsetenv

src/syscall/env_unix.go

 // Copyright 2010 The Go Authors.  All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
 // +build darwin dragonfly freebsd linux nacl netbsd openbsd solaris
 
 // Unix environment variables.
 
 package syscall
 
 import "sync"
 
 var (
         // envOnce guards initialization by copyenv, which populates env.
         envOnce sync.Once
 
         // envLock guards env and envs.
         envLock sync.RWMutex
 
         // env maps from an environment variable to its first occurrence in envs.
         env map[string]int
 
-        // envs is provided by the runtime. elements are expected to be
-        // of the form "key=value".
+        // envs is provided by the runtime. elements are expected to
+        // be of the form "key=value". An empty string means deleted
+        // (or a duplicate to be ignored).
         envs []string = runtime_envs()
 )
 
 func runtime_envs() []string // in package runtime
 
 // setenv_c and unsetenv_c are provided by the runtime but are no-ops
 // if cgo isn't loaded.
 func setenv_c(k, v string)
 func unsetenv_c(k string)
 
 func copyenv() {
         env = make(map[string]int)
         for i, s := range envs {
                 for j := 0; j < len(s); j++ {
                         if s[j] == '=' {
                                 key := s[:j]
                                 if _, ok := env[key]; !ok {
-                                        env[key] = i
+                                        env[key] = i // first mention of key
+                                } else {
+                                        // Clear duplicate keys. This permits Unsetenv to
+                                        // safely delete only the first item without
+                                        // worrying about unshadowing a later one,
+                                        // which might be a security problem.
+                                        envs[i] = ""
                                 }
                                 break
                         }
                 }
         }
 }
 
 func Unsetenv(key string) error {
         envOnce.Do(copyenv)
 
         envLock.Lock()
         defer envLock.Unlock()
 
-        // Clear all envs[i] where envs[i] starts with key+"=".
-        // The empty values will be filtered out in Environ.
-        for i, pair := range envs {

[rsc, 2014/10/01 13:40:06]

This is definitely better, but I'm still worried about someone looping over a
large environment calling Unsetenv, and that being quadratic because Unsetenv is
linear.

We could avoid this loop here if above in copyenv we do

if _, ok := env[key]; ok {
    env[key] = i // first mention of key
} else {
    envs[i] = "" // clear second mention of key
}

-                if len(pair) >= len(key)+1 &&
-                        pair[:len(key)] == key &&
-                        pair[len(key)] == '=' {
-                        envs[i] = ""
-                }
+        if i, ok := env[key]; ok {
+                envs[i] = ""
+                delete(env, key)
         }
-        delete(env, key)
         unsetenv_c(key)
         return nil
 }
 
 func Getenv(key string) (value string, found bool) {
         envOnce.Do(copyenv)
         if len(key) == 0 {
                 return "", false
         }
 
@@ skipping 63 matching lines @@
         envLock.RLock()
         defer envLock.RUnlock()
         a := make([]string, 0, len(envs))
         for _, env := range envs {
                 if env != "" {
                         a = append(a, env)
                 }
         }
         return a
 }