Windows registry handling / hugs CVS repository?

31 Mar 2001

      Hi,

I think that the way hugs uses the registry under Windows is dangerous and
buggy.  This message describes the problems and proposes some alternative
designs.  I welcome comments and suggestions. 

"dangerous":

In the current design, any options changes made interactively using ":set" from
the hugs prompt are ALWAYS saved immediately to the registry.  This is all well
and good when the user actually made an intended change, but it is an brutally
unforgiving of mistakes.

Consider the common scenario of someone attempting to add a component to the
search path using something like:
	Prelude> :set -Pc:\my\lib\path
Ooops!  The user forgot to include a ';' in the search path to append or prepend
this path to the existing search path.  Thanks to the automatic save-to-registry
"feature", there is no way to undo this change.  Even if the user quits and
restarts hugs, the search path will only contain this one component.  The
original hugs default search path, as well as any changes made to the search
path since hugs was installed on the system, are now GONE FOREVER!  In fact, the
only way I know of to recover the default search path is to re-install hugs from
scratch.

"buggy":

During a desperate last minute demo preparation recently (for Paul Hudak's
class), I had to append a few things to my search path using something like:
  Prelude> :set -P;c:\some\big\long\hairy\path
At some point, I crossed some small magic size limit, and *poof!*:  next time I
started hugs, all of my path changes since hugs had been installed silently
disappeared, and the path reverted to the system default path!   If you want to
see this bug yourself, simply run the above command a bunch of times (maybe 6 to
10), restart hugs, and do a ":set" to see the bogus value for path.

I did a little digging around in the code (from Feb2001 release of hugs), and
found the following

First, in hugs.c, the implementation of optionsToStr() looks like this:

static String local optionsToStr() {
     static char buffer[2000];

     (...bunch of code to add things to buffer, with no buffer overrun
checks...)

     return buffer;
}		

Then, in machdep.c, we have:

static Sting local readRegString(...)
...
{
#if HUGS_FOR_WINDOWS
    static char buf[2048];
#else
    static char buf[300];
#endif
    if (queryValue(...buf, sizeof(buf)) == REG_SZ) {
	/* success */
	return buf;
     } else {
        return def;  /* system default option settings */
     }
}

Here is my short list of bugs in the above code:

1.  Fixed size buffers are a dangerous practice.  If you must use them, at least
make sure they are large enough to hold the largest conceivable value and then
some.  In this case, 2048 bytes is a reasonable size; 300 is not.  It turns out
that the HUGS_FOR_WINDOWS preprocessor symbol is only defined when building the
rarely used Windows GUI for hugs, not the ordinary Windows version.  I *STRONGLY
RECOMMEND* just getting rid of the #if..#endif, and using the larger buffer
size.  I'm fairly certain this is the cause of the bug I noted above.

2.  It's unfortunate that the code in the first function doesn't check for
buffer overruns, but at least a 2000 byte buffer is (somewhat) reasonable.

3.  Both of these functions return a pointer to automatic (i.e. stack allocated)
variables.  This is a classic dangling pointer bug, and the behavior of such a
program is undefined according to the ANSI C standard.  (IF you happen not to
have any function calls between where the pointer is returned and where the
pointer is used in the calling code, and IF your compiler happens to implement
automatic variables using a stack, then you might "get lucky" and your program
might behave as intended, but there is no guarantee this will work.)  A simple
solution is to allocate the buffer in the caller's stack frame and pass a
pointer to the callee.

-----

My radical alternative design proposal:

-  Do NOT automatically save settings to the Windows registry.  I hope the
"dangerous" paragraph above is enough to convince anyone that this is just a bad
idea.

-  As a convenience, after reading HUGSFLAGS from the environment, also read an
environment variable HUGSPATH.  Allow empty path components to be used to
specify the current path (to prepend / append), as we do now.

If absolutely necessary, we could provide a ":regsave" command to allow the user
to explicitly save changes made to hugs options to the registry.  Personally, I
don't think this is necessary, but I'd be happy to hear others' opinions.

I welcome comments / discussion on this proposal.  Using environment variables
for such settings is cross-platform, well understood, reasonably standard, and
avoids the dangers of this automatic save-to-registry.

I volunteer to implement the above changes, if someone would be so kind as to
point me towards the hugs CVS repository.

	-antony

-- 
Antony Courtney  
Grad. Student, Dept. of Computer Science, Yale University
antony@apocalypse.org          http://www.apocalypse.org/pub/u/antony

Antony Courtney

Andre Santos

tags

participants (2)