From: Rick M. <obj...@gm...> - 2009-10-01 17:24:09
|
I'd say leave the 4.0 branch alone, open the bug, and do a complete cleanup of this area to use more "modern" methods. That gives us a little more time to catch the gotchas. Rick On Thu, Oct 1, 2009 at 1:18 PM, Mark Miesfeld <mie...@gm...> wrote: > On Thu, Oct 1, 2009 at 2:03 AM, Rick McGuire <obj...@gm...> wrote: >> Ok, I've taken a look at the code, and I don't understand how this >> works either, and it certainly could lead to some crashes or memory >> problems because that pointer in the einvironment is still pointing to >> memory it should not be. It would appear the appropriate fix would be >> to call putenv() to set the target variable to a null string value. >> That will ensure the new value is set properly. > > This whole area seems to be a mess to me. The > SetEnvironmentVariable() sys_process_export(), and > restoreEnvironment() functions all have this code at the start: > > if (!putflag) > { /* first change in the */ > /* environment ? */ > /* copy all entries to dynamic memory */ > /*for all entries in the env */ > for (;*Environment != NULL;Environment++) > { > length = strlen(*Environment)+1; /* get the size of the string */ > /* and alloc space for it */ > Env_Var_String = (char *)malloc(length); > memcpy(Env_Var_String,*Environment,length);/* copy the string */ > putenv(Env_Var_String); /* and make it part of env */ > } > putflag = 1; /* prevent do it again */ > } > > >From a comment I found in some old Gnu code: > > /* On Solaris, HP-UX and IRIX there is no function to clear an environment > variable. So we look for the variable in the environ table and delete it > by setting the entry to NULL. This can clearly cause some memory leaks > but free cannot be used on this context as not all strings in the environ > have been allocated using malloc. To avoid this memory leak another > method can be used. It consists in forcing the reallocation of all the > strings in the environ table using malloc on the first call on the > functions related to environment variable management. The disavantage > is that if a program makes a direct call to getenv the return string > may be deallocated at some point. */ > > I think this malloc'ng a copy of every environment string and putting > it back into environ was an attempt to support the clearing of an > environment variable on platforms that didn't have unsetenv() and > avoid the memory leak by using the method referenced above. > > AIX didn't have unsetenv() in 5.1 but does in 5.3 > > All 3 of the functions have the: > > if (del) /* if there was an old entry */ > { > free(del); /* free it */ > } > > code. Which seems inherently unsafe, but the reason for it is to free > the memory malloc'd in the original copying of all the environment > strings. (Or malloc'd in subsequent replacment strings.) All the man > pages for putenv() that I have seen say the string passed in becomes > part of the environment and should not be altered or freed. So, we > don't really know what putenv() is going to do. > > All of these 3 areas could be rewritten to use unsetenv(), getenv(), > and setenv() which would be cleaner and safer. > > unsetenv() is on Linux and Mac. For AIX, I just found a reference > that unsetenv() is on AIX 5.2, which is the earliest we say we > support. For Solaris we say we support 2.8, which doesn't have > unsetenv(), but I think 2.10 does. (Although I couldn't find anything > definite.) > > I have this patch for ValueFunction.cpp > > Index: interpreter/platform/unix/ValueFunction.cpp > =================================================================== > --- interpreter/platform/unix/ValueFunction.cpp (revision 5228) > +++ interpreter/platform/unix/ValueFunction.cpp (working copy) > @@ -103,11 +103,16 @@ > del = *Environment; /* remember it for deletion */ > } > > - if (Value != (RexxString *) TheNilObject) > + if (Value == (RexxString *) TheNilObject) > { > + sprintf(Env_Var_String, "%s=%s",Name->getStringData(),""); > + } > + else > + { > sprintf(Env_Var_String, > "%s=%s",Name->getStringData(),Value->getStringData()); > - putenv(Env_Var_String); > } > + putenv(Env_Var_String); > + > if(del) /* if there was a old one */ > free(del); /* free it */ > return 0; /* return success */ > > Which tests okay on Linux. > > Since we are not going to change the release code to fix value() on > Mac, maybe we should open a bug so we don't forget about it and > rewrite those sections? (I'll do the rewrite.) Or, I can just apply > the patch. Or ... ? > > -- > Mark Miesfeld > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry® Developer Conference in SF, CA > is the only developer event you need to attend this year. Jumpstart your > developing skills, take BlackBerry mobile applications to market and stay > ahead of the curve. Join us from November 9-12, 2009. Register now! > http://p.sf.net/sfu/devconf > _______________________________________________ > Oorexx-devel mailing list > Oor...@li... > https://lists.sourceforge.net/lists/listinfo/oorexx-devel > |