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);
}
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.