Search code examples
cmemory-leaksstrdup

Seeking Help to Identify Memory Leaks in my C Function


I'm currently trying to implement the setenv function in C that sets the value of an environment variable, and I'm concerned about memory leaks in my code flagged by Valgrind due to strdup. I would greatly appreciate your insights in helping me identify any memory leak issues in the following code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

extern char **environ;

int _setenv(const char *name, const char *value)
{
    char **new_environ, *current_env, *env_name;
    int count = 0, modified = 0, i = 0;
    
    /* Count the number of environment strings */
    while (environ[i])
    {
        count++;
        i++;
    }

    /* Create a copy of the environment variable */
    new_environ = malloc(sizeof(char *) * (count + 2));
    if (!new_environ)
    {
        perror("Memory Allocation Error");
        return (-1);
    }

    for (i = 0; i < count; i++)
    {
        current_env = strdup(environ[i]);
        env_name = strtok(current_env, "=");
        if (env_name != NULL && strcmp(env_name, name) == 0)
        {
            new_environ[count] = malloc(strlen(name) + strlen(value) + 2);
            if (new_environ[count])
            {
                sprintf(new_environ[count], "%s=%s", name, value);
                new_environ[count + 1] = NULL; /* Terminate the new array */
                modified = 0;
                free(current_env);
                break;
            }
        }
        else
        {
            new_environ[i] = current_env;
            modified = 1;
            free(current_env);
        }
    }
    if (!modified)
    {
        new_environ[count] = malloc(strlen(name) + strlen(value) + 2);
            if (new_environ[count])
            {
                sprintf(new_environ[count], "%s=%s", name, value);
                new_environ[count + 1] = NULL; /* Terminate the new array */
            }
    }
    else
    {
        new_environ[count] = NULL; /* Terminate the new array */
    }
    environ = new_environ;
    return (0);
}

Solution

  • if (new_environ[count])
    { ... }
    else 
    {
      // this will be regarded as a leak of current_env 
    }
    

    A better design would be:

    current_env = strdup(environ[i]);
    env_name = strtok(current_env, "=");
    
    bool modified = alloc_name(current_env, env_name);
    if(modified)
    { /* ... */ }
    else 
    { /* ... */ }
    
    free(current_env); // always free it, unconditionally.
    

    Also I don't understand this part at all:

    new_environ[i] = current_env;
    modified = 1;
    free(current_env);
    

    The new_environ[i] = current_env is a soft copy only assigning a pointer to the pointer array. The actual data remains where it is. So if you free(current_env) then new_environ[i] ends up pointing at garbage since you just freed the data it pointed at.