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 <[email protected]> wrote: > On Thu, Oct 1, 2009 at 2:03 AM, Rick McGuire <[email protected]> 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 > [email protected] > https://lists.sourceforge.net/lists/listinfo/oorexx-devel > ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/oorexx-devel
