4 ms·

FWIW the decision to leak memory on Mac actually goes back ~26 years to FreeBSD - https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=5604 https://bugs.freebsd.or

by bhawks 2y ago

FWIW the decision to leak memory on Mac actually goes back ~26 years to FreeBSD - https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=5604 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=5604 which OSX inherited. I would not be surprised that Windows setenv has BSD roots due to licensing. 26 years ago people knew this API was broken but didn't fix it due to inertia of breaking buggy programs further. There really shouldn't be a need to change your own process's envvars. For subprocesses just use the proper exec function. For anything else there should be a clear API to call rather than changing a global variable and hoping some code far away from yours rereads it and handles things correctly.

jandrese 2y ago
I only partially disagree with the sentiment that it is "impossible to fix". For the current API that is true, but a fairly minor modification API would make it possible. All getenv() has to do is strdup() the return value before sending it back and leaving it on the programmer to free the memory when they are done with it. This does mean that the programmer will need to call getenv() again if they think the value might change, but I think that is a reasonable tradeoff. This change would make old programs leak memory every time they call getenv() without the subsequent free(), but since the current version also leaks memory that doesn't seem like a dealbreaker. As an added bonus the new version could be made thread safe by wrapping the strdup() in a mutex and doing similar work on the setenv() side.
- bhawks 2y ago
  There exists a ton of working code that does not fiddle with setenv and now would have memory leaks if they don't change their code? Plus I would now need to test if my stdlib requires freeing memory or not because if I try to free on an older libc it is not going to work out well. I don't think the value works out. From a hygiene perspective - freeing the return of another API is an anti pattern. If you need the caller to release objects there should provide a FooLib_bar_destory(bar) or similar.
  - jandrese 2y ago
    Yes, it would cause existing programs to leak memory. Most of the time these leaks would be fairly minor, but some programs could leak a lot if they call getenv() inside of a loop for some reason. Personally, if the return object is a basic C type, I'm not a fan of creating a wrapper function to call free(). This is one of those code purity things that I don't think buys you anything.

by bhawks 2y ago

jandrese 2y ago

I only partially disagree with the sentiment that it is "impossible to fix". For the current API that is true, but a fairly minor modification API would make it possible. All getenv() has to do is strdup() the return value before sending it back and leaving it on the programmer to free the memory when they are done with it. This does mean that the programmer will need to call getenv() again if they think the value might change, but I think that is a reasonable tradeoff. This change would make old programs leak memory every time they call getenv() without the subsequent free(), but since the current version also leaks memory that doesn't seem like a dealbreaker. As an added bonus the new version could be made thread safe by wrapping the strdup() in a mutex and doing similar work on the setenv() side.

bhawks 2y ago
There exists a ton of working code that does not fiddle with setenv and now would have memory leaks if they don't change their code? Plus I would now need to test if my stdlib requires freeing memory or not because if I try to free on an older libc it is not going to work out well. I don't think the value works out. From a hygiene perspective - freeing the return of another API is an anti pattern. If you need the caller to release objects there should provide a FooLib_bar_destory(bar) or similar.
- jandrese 2y ago
  Yes, it would cause existing programs to leak memory. Most of the time these leaks would be fairly minor, but some programs could leak a lot if they call getenv() inside of a loop for some reason. Personally, if the return object is a basic C type, I'm not a fan of creating a wrapper function to call free(). This is one of those code purity things that I don't think buys you anything.