On November 18, 2002 at 20:43, Jon Steinhart wrote:
Oh, some details.
1. A second getenv() call would not break the code. The copy was really
unnecessary.
As pointed out earlier, making the copy may be needed depending on the
implementation of getenv() for a given platform. And for all we know,
the person who original wrote the code had an environment where it
was required.
2. It's hard for me to imagine a situation where getpwuid() would #1 get
called a second time and #2 for a different uid, which is the only that
a problem would occur.
It is hard for us to imagine alot of things, but I never fault a
programmer for being extra paranoid. Paranoia makes programs more
robust against unforseen events, and can pay off big time when such
events occur.
3. If there's a NULL passwd->pw_dir then libc is broken and should be fixed.
Better that this gets pointed out and fixed rather than covered up so tha
t
it stays unnoticed and broken.
Oh, and I've wasted some of my volunteer time trying to figure out what the
code did before changing it. I'd waste less if there was less code. Best way
to accomplish this is to get rid of the code that doesn't do anything.
Again, it is safe to be paranoid. It is not unheard of where an OS's
libc have bugs, and software that properly checks all data is okay in
my book. I'll give a real-world example related to nmh: When I did
some changes to nmh code to get it to compile on cygwin, there was a
system call that did NOT return the proper value. The call was broken
or was stubbed and had not been implemented by the cygwin maintainers.
Plus, if you take security seriously, one should always check data
retrieved from components you do not have direct control over.
Even if you think, "this will never happen," the day it does, you'll
kick yourself in the head and have a possible security vulnerability
on your hands. It's like when people say, "the data will never
be larger than X," so they hard-code an array size, and when the
data is larger than X, and buffer overflows occur.
I do agree that MH/nmh does have duplicate code in areas that can
be reduced down into reusable components (especially some of the
time stuff when I played with making nmh compile under cygwin).
I also agree that there are areas in the code that makes a new person
wondering why in the hell certain code exists.
However, in my experience, when I do not have complete knowledge of
an existing code base, I hesitate to remove any code that "appears"
to be unnecessary. For all I know, the original author encountered
some abhorent scenario that requires the code to exist. Remember,
the MH/nmh code base is *very* old, so there is alot of environmental
experience that we have never been exposed to.
In cases where I think something should not exist, I typically add
a comment in the code like "/* XXX: Why is this here? */" or "/*
XXX: What the heck does this do? */", and seek advice from other
maintainers, and if possible the original author, on the purpose of
the code, like exactly what you have done by asking about the code
to this list. But, do not be surprised that other may disagree with
you.
Then, if people agree a chunk of code can be axed, after it is removed,
make sure to test it.
My $0.02,
--ewh