Created attachment 206833 [details] [review]
Plug a mem leak in g_environ_unsetenv

==9458== 10 bytes in 1 blocks are definitely lost in loss record 16 of 39
==9458==    at 0x402AD89: malloc (vg_replace_malloc.c:236)
==9458==    by 0x4221A1F: vasprintf (vasprintf.c:78)
==9458==    by 0x40C6065: g_vasprintf (gprintf.c:314)
==9458==    by 0x409D894: g_strdup_vprintf (gstrfuncs.c:509)
==9458==    by 0x409D8C9: g_strdup_printf (gstrfuncs.c:535)
==9458==    by 0x40672E9: g_environ_setenv (genviron.c:156)
==9458==    by 0x80490E7: test_environ_array (environment.c:78)
==9458==    by 0x40A3DB5: test_case_run (gtestutils.c:1662)
==9458==    by 0x40A40B2: g_test_run_suite_internal (gtestutils.c:1715)
==9458==    by 0x40A417C: g_test_run_suite_internal (gtestutils.c:1726)
==9458==    by 0x40A42F9: g_test_run_suite (gtestutils.c:1771)
==9458==    by 0x40A3441: g_test_run (gtestutils.c:1319)
==9458==    by 0x80493F1: main (environment.c:108)

Bug #669412.

Review of attachment 206833 [details] [review]:

Hmm...I don't think there's any guarantee here that the provided environment was allocated with g_malloc().  Yes, it's hard to use this function without leaking, but I think we may need to instead add g_environ_unsetenv_copy() which doesn't mutate the input.  Or g_environ_unsetenv_with_free_func() ?

Dan?

Both g_environ_setenv and g_environ_unsetenv are new in 2.32, so I think we can simply amend their documentation to specify this. Note that g_environ_setenv uses g_strdup_printf() for the member it _adds_, so if we were to allow the passed-in envp to contain non-g_free-compatible memory, there would be no way to free the returned envp at all since some members would be g_free'able while others would be foo_free'able only...

Comment on attachment 206833 [details] [review]
Plug a mem leak in g_environ_unsetenv

g_environ_unsetenv() was definitely not supposed to leak. But this patch is wrong because g_unsetenv() currently uses g_environ_unsetenv() in the fallback case where the system doesn't have unsetenv(), and the system environment strings definitely aren't g_free()able. So you need to copy the existing code back to g_unsetenv() before you can add a g_free() to g_environ_unsetenv().

(In reply to comment #3)
> Both g_environ_setenv and g_environ_unsetenv are new in 2.32, so I think we can
> simply amend their documentation to specify this. Note that g_environ_setenv
> uses g_strdup_printf() for the member it _adds_,

Good point.  Ok, I'm fine with the patch as long as the documentation is updated to reflect that each member must have been allocated with g_malloc() - maybe just point to g_strfreev().

Created attachment 206863 [details] [review]
Note that g_environ_setenv() already had the requirement that the passed array was

allocated with glib's allocator since it uses g_renew on it, and also the requirement
that the elements are also allocated using the glib allocator, since it uses
g_free on them when overwriting an existing value.

Review of attachment 206863 [details] [review]:

Looks right.

Pushed to master.