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