Skip to content
Merged
Prev Previous commit
Next Next commit
Remove the memory leak
  • Loading branch information
filipnavara committed Aug 26, 2021
commit d9f527ba1959fb5be53ef1b18af73a07898460cf
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,8 @@ internal unsafe partial class Sys
{
[DllImport(Interop.Libraries.SystemNative, EntryPoint = "SystemNative_GetEnviron")]
internal static extern unsafe IntPtr GetEnviron();

[DllImport(Interop.Libraries.SystemNative, EntryPoint = "SystemNative_FreeEnviron")]
internal static extern unsafe void FreeEnviron(IntPtr environ);
}
}
1 change: 1 addition & 0 deletions src/libraries/Native/Unix/System.Native/entrypoints.c
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ static const Entry s_sysNative[] =
DllImportEntry(SystemNative_GetGroups)
DllImportEntry(SystemNative_GetEnv)
DllImportEntry(SystemNative_GetEnviron)
DllImportEntry(SystemNative_FreeEnviron)
};

EXTERN_C const void* SystemResolveDllImport(const char* name);
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/Native/Unix/System.Native/pal_environment.c
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,8 @@ char** SystemNative_GetEnviron()
return environ;
#endif
}

void SystemNative_FreeEnviron(char** environ)
{
// no op
}
4 changes: 3 additions & 1 deletion src/libraries/Native/Unix/System.Native/pal_environment.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@

PALEXPORT char* SystemNative_GetEnv(const char* variable);

PALEXPORT char** SystemNative_GetEnviron(void);
PALEXPORT char** SystemNative_GetEnviron(int32_t *releaseMemory);

PALEXPORT void SystemNative_FreeEnviron(char** environ);
52 changes: 33 additions & 19 deletions src/libraries/Native/Unix/System.Native/pal_environment.m
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
return getenv(variable);
}

static char *empty_key_value_pair = "=";

static void get_environ_helper(const void *key, const void *value, void *context)
{
char ***temp_environ_ptr = (char***)context;
Expand All @@ -30,35 +32,47 @@ static void get_environ_helper(const void *key, const void *value, void *context
key_value_pair[utf8_key_length] = '=';
strcpy(key_value_pair + utf8_key_length + 1, utf8_value);
}
else
{
// In case of failed allocation add pointer to preallocated entry. This is
// ignored on the managed side and skipped over in SystemNative_FreeEnviron.
key_value_pair = empty_key_value_pair;
}

**temp_environ_ptr = key_value_pair;
(*temp_environ_ptr)++;
}

char** SystemNative_GetEnviron()
{
static char **environ;
char **temp_environ;
char **temp_environ_ptr;

// NOTE: This function is not thread-safe and it leaks one array per process. This is
// intentional behavior and the managed code is expected to take additional guards
// around this call.
if (environ == NULL)
CFDictionaryRef environment = (CFDictionaryRef)[[NSProcessInfo processInfo] environment];
int count = CFDictionaryGetCount(environment);
temp_environ = (char **)malloc((count + 1) * sizeof(char *));
if (temp_environ != NULL)
{
int environ_size = 1;
char **temp_environ;
char **temp_environ_ptr;
temp_environ_ptr = temp_environ;
CFDictionaryApplyFunction(environment, get_environ_helper, &temp_environ_ptr);
*temp_environ_ptr = NULL;
}

return temp_environ;
}

CFDictionaryRef environment = (CFDictionaryRef)[[NSProcessInfo processInfo] environment];
int count = CFDictionaryGetCount(environment);
temp_environ = (char **)malloc((count + 1) * sizeof(char *));
if (temp_environ != NULL)
void SystemNative_FreeEnviron(char** environ)
{
if (environ != NULL)
{
for (char** environ_ptr = environ; *environ_ptr != NULL; environ_ptr++)
{
temp_environ_ptr = temp_environ;
CFDictionaryApplyFunction(environment, get_environ_helper, &temp_environ_ptr);
*temp_environ_ptr = NULL;
environ = temp_environ;
if (*environ_ptr != empty_key_value_pair)
{
free(*environ_ptr);
}
}
}

return environ;
}
free(environ);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,20 @@ private static Dictionary<string, string> GetSystemEnvironmentVariables()
IntPtr block = Interop.Sys.GetEnviron();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is straight copy from NativeAOT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then I think it would be better to have it in shared location

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I would agree.

I started with a minimal fix because I wanted to limit the impact for backporting to .NET 6 branch.

Then I moved it to System.Native based on chat with @akoeplinger (and I agree that it belongs there). Proper sharing of the code with NativeAOT will need few more changes and I wanted to validate the approach first before proceeding with that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this is fine for now, we can unify with NativeAOT in main later on.

if (block != IntPtr.Zero)
{
IntPtr blockIterator = block;

// Per man page, environment variables come back as an array of pointers to strings
// Parse each pointer of strings individually
while (ParseEntry(block, out string? key, out string? value))
while (ParseEntry(blockIterator, out string? key, out string? value))
{
if (key != null && value != null)
results.Add(key, value);

// Increment to next environment variable entry
block += IntPtr.Size;
blockIterator += IntPtr.Size;
}

Interop.Sys.FreeEnviron(block);
}

return results;
Expand Down