There is a memory leak in the setenv() function. If you overwrite a value with a longer value, the memory allocated for the shorter value is never freed. Also, in certain failure cases (such as failure in the realloc() function), the proper return value is returned (-1) but the existing environment is destroyed. How-To-Repeat: Memory leak is exhibited by this program: #include <stdlib.h> #define BSIZE 1024 char buf[BSIZE + 1]; int main(int ac, char *av[]) { int x; memset(buf, 'b', BSIZE); buf[BSIZE] = 0; for (x = 0; 1; x++) { buf[x % BSIZE] = 0; setenv("foo", buf, 1); buf[x % BSIZE] = 'b'; } return(0); } Also, notice what happens to "environ" in the original code when the realloc() function call fails. Also, the "alloced" flag is incorrectly set if the original malloc() fails. Overall, this function exhibits SHODDY PROGRAMMING!! :-)
FreeBSD-gnats@FreeBSD.ORG writes: > Thank you very much for your problem report. > It has the internal identification `bin/5604'. > The individual assigned to look at your > report is: freebsd-bugs. > > >Category: bin > >Responsible: freebsd-bugs > >Synopsis: setenv(3) function has memory leak, other bugs > >Arrival-Date: Fri Jan 30 14:20:01 PST 1998 Just realized the patch is incorrect in two ways: - It doesn't fix the same memory leak in unsetenv(). - The original environ[] entries established by the loader are not allocated with malloc() (correct?) Therefore they should not be free'd. Working on a better patch... -Archie ___________________________________________________________________________ Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com
State Changed From-To: open->closed waiting for better times
State Changed From-To: closed->suspended Archie thinks it should be suspended, I guess he's right...
Has there been progress on a fix, or should the behavior be left as-is? PR bin/10341 references the same problem. Guy Helmer ghelmer@freebsd.org
Guy Helmer writes: > Has there been progress on a fix, or should the behavior be left as-is? > PR bin/10341 references the same problem. The fact of the matter is that (according to the forces of inertia) "expected behavior" *implies* a memory leak. This is because if you do something like this: char *s; s = getenv("FOO"); setenv("FOO", "new contents", 1); then the pointer "s" is supposed to still be valid. That is, the setenv() routine is not allowed to call free(s). This is what the FreeBSD forces of inertia have claimed anyway. I think there must be very few programs that rely on this, and the ones that do are bogus anyway, and we should just fix it. Morever, as I remember it was never made clear what POSIX specifies about this. But the FOI prevailed when I tried to fix this before. A workaround is to never overwrite a string with a shorter string. Overwrite with a string of the same length as the original and then sprintf() the shorter string onto the new one. -Archie ___________________________________________________________________________ Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com
I suspect that it isn't possible to implement setenv(3) in a manner that doesn't leak memory. The approach used in Solaris and OSF/1 (I'm not sure what POSIX mandates) is not to have a setenv(3) at all. Instead putenv(3) directly manipulates the environment and requires that the string pointed to by its argument remain valid (ie not be an automatic variable). This pushes the memory management issue onto the application. Until someone has a brainstorm and actually solves the bug, how about we document the problem: Index: getenv.3 =================================================================== RCS file: /home/CVSROOT/./src/lib/libc/stdlib/getenv.3,v retrieving revision 1.2 diff -u -r1.2 getenv.3 --- getenv.3 1999/07/12 20:47:45 1.2 +++ getenv.3 1999/08/10 21:46:15 @@ -150,3 +150,13 @@ .Fn putenv function appeared in .Bx 4.3 Reno . +.Sh BUGS +Successive calls to +.Fn setenv +or +.Fn putenv +assigning a differently sized +.Ar value +to the same +.Ar name +will result in a memory leak. Peter -- Peter Jeremy (VK2PJ) peter.jeremy@alcatel.com.au Alcatel Australia Limited 41 Mandible St Phone: +61 2 9690 5019 ALEXANDRIA NSW 2015 Fax: +61 2 9690 5982
In view of the upcoming 4.0-RELEASE freeze, together with the apparent impossibility to implement a setenv(3) that doesn't have memory leaks, could you please consider committing the following patch which at least documents the problem. I would be satisfied with this as a `fix' for PR bin/10341 (though it doesn't address the second part of bin/5604). Index: src/lib/libc/stdlib/getenv.3 =================================================================== RCS file: /home/CVSROOT/src/lib/libc/stdlib/getenv.3,v retrieving revision 1.3 diff -u -r1.3 getenv.3 --- src/lib/libc/stdlib/getenv.3 1999/08/28 00:01:31 1.3 +++ src/lib/libc/stdlib/getenv.3 1999/10/05 06:26:19 @@ -150,3 +150,13 @@ .Fn putenv function appeared in .Bx 4.3 Reno . +.Sh BUGS +Successive calls to +.Fn setenv +or +.Fn putenv +assigning a differently sized +.Ar value +to the same +.Ar name +will result in a memory leak. Peter
Peter Jeremy writes: > In view of the upcoming 4.0-RELEASE freeze, together with the apparent > impossibility to implement a setenv(3) that doesn't have memory leaks, > could you please consider committing the following patch which at > least documents the problem. I would be satisfied with this as a `fix' > for PR bin/10341 (though it doesn't address the second part of bin/5604). Thanks, I'll do it. -Archie ___________________________________________________________________________ Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com
While we are on this topic, how about the following patch to setenv.c? o Back out rev 1.4 - reallocf() failure will clobber existing `environ'. o Do not override `environ' if realloc() fails (Obtained from: OpenBSD). o Set `alloced' only when memory was actually allocated. This will hopefully fix the 2nd part of this PR. <PS>Any objections if I close PR 10341 as a duplicate of PR 5604?</PS> -- Ruslan Ermilov Sysadmin and DBA of the ru@ucb.crimea.ua United Commercial Bank, ru@FreeBSD.org FreeBSD committer, +380.652.247.647 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age
State Changed From-To: suspended->closed "Memory leak" is documented in getenv.3, "other bugs" are fixed in setenv.c.
On 2000-Jan-27 20:17:39 +1100, Ruslan Ermilov <ru@FreeBSD.ORG> wrote: >While we are on this topic, how about the following patch to setenv.c? Seems good. (And I notice you've committed it). ><PS>Any objections if I close PR 10341 as a duplicate of PR 5604?</PS> I previously told Archie that I'd be satisfied with documenting the bug as a fix. From a different perspective, since we've fixed the first half of PR5604, but not touched the second half - which is what PR10341 covers, we might have been better off closing 5604 and leaving 10341 to remind us of the real problem :-/. On Thu, 27 Jan 2000 22:10:13 +1100, Bruce Evans <bde@zeta.org.au> wrote: >PR 10341 has the only example that I know of where the memory leak >matters, but we all know this now :-). I agree that example was contrived. The problem _did_ bite me though - I was doing some exhaustive testing on a function that did time conversions in multiple timezones, and the ctime(3) functions don't have any way to specify a timezone other than via $TZ, so I was continually calling putenv(3) and wondered about the process bloat (10341) and poor performance (10342). Peter