Re: [PATCH 1/3] overhaul environment functions

[Alexander Monakov <amonakov@ispras.ru>]

On Sun, 20 Mar 2016, Alexander Monakov wrote:
> On Sun, 20 Mar 2016, Rich Felker wrote:
> > After debunking most of my concerns, are there any things left to
> > change in patch 1, or should I go ahead and apply it?
> 
> Let me send a v2 with two changes: reinstating setenv EINVAL on null arg,
> and tweaking unsetenv like I mentioned. Should be good with those changes,
> I think.

I've attempted another go at testing, and it turns out the new code still has
a memory leak, although a subtler and smaller one. It is caused by failure to
use existing null slots in __env_change when it is called with p != 0, but no
slot in env_alloced matches p (so the early-out by replacing p by r and
freeing p does not happen).

I'm quite surprised I didn't catch this sooner, because the testcase for the
existing leak (alternating setenv/putenv calls) also exposes this one, albeit
at a slower rate. I've probably made some mistake like misinterpreting the
exit status; not sure.

The following incremental diff shows how I have addressed this issue. I'm
leaving it for comments, and will send an updated patch with two changes
mentioned above, plus __env_change renamed to __env_rm_add, some time later.

Alexander

diff --git a/src/env/setenv.c b/src/env/setenv.c
index db9d72a..051c8d0 100644
--- a/src/env/setenv.c
+++ b/src/env/setenv.c
@@ -9,17 +9,23 @@ void __env_change(char *p, char *r)
 {
        static char **env_alloced;
        static size_t env_alloced_n;
+       char **nullslot = 0;
        for (size_t i=0; i < env_alloced_n; i++)
                if (env_alloced[i] == p) {
                        env_alloced[i] = r;
                        free(p);
                        return;
+               } else if (!env_alloced[i]) {
+                       nullslot = env_alloced + i;
                }
        if (!r) return;
-       char **new_ea = realloc(env_alloced, sizeof *new_ea * (env_alloced_n+1));
-       if (!new_ea) return;
-       new_ea[env_alloced_n++] = r;
-       env_alloced = new_ea;
+       if (!nullslot) {
+               char **t = realloc(env_alloced, sizeof *t * (env_alloced_n+1));
+               if (!t) return;
+               env_alloced = t;
+               nullslot = env_alloced + env_alloced_n++;
+       }
+       *nullslot = r;
 }
 
 int setenv(const char *var, const char *value, int overwrite)