On Sun, Jan 15, 2017 at 11:33:41AM +0100, Simon Ruderich wrote:
> > else if ((f = fopen (path, "r")) != NULL)
> > {
> > struct utimbuf times;
> > + int ch = 0;
>
> The first iteration of the while-loop will initialize ch. I would
> drop the initialization here as it confused me into thinking the
> "&& ch" check in the if below verifies if the loop was run at
> least once. But that will be always true.I strongly disagree with this: Your confusion notwithstanding, the code was perfectly clear. It's much safer and good practice to always initialize your variables to a value that's safe. Honestly, just last Friday my team had to deal with a bug where a function failed to follow this best practice, written like you describe, and the code changed in such a way that initializing the variable was *required* but didn't happen, leading to the code making a decision based on random garbage in a stack-allocated variable, ultimately causing data corruption and a possible crash. Simply because the original author didn't initialize the variable properly, and the modifier failed to notice that best coding standards had not been followed. Mutt is a security-sensitive application, and defensive programming and best practices should be followed throughout. Just because it's not particularly harmful in this specific case is not a reason to be lax. > > + /* Some mailbox creation tools erroneously append a blank line to > > + * a file before appending a mail message. This allows mutt to > > + * detect magic for and thus open those files. */ > > + while ((ch = fgetc(f)) && (ch == '\n' || ch == '\r')); > > I'd move the ; to th next line to make it more visible or use > continue, e.g. > > while (...) > continue; This one is just a style matter--I feel much less strongly about it--but I don't agree with this point either. I've found the empty statement is actually a common idiom in a lot of code I've worked on, and competent programmers should be able to recognize it. I might suggest an additional space before the semi-colon, but the construct is fine. [Re-quoting the original patch to provide proper context] + if (!feof(f) && ch) + ungetc(ch, f); > ch will be either EOF on error or end-of-file or contain the > first byte. Why the check for != 0 here? This will break if there > was an error during reading and will put EOF (Oxff) into the > stream. It will do no such thing. If the file is EOF the ungetch() will not execute. However, if ch is 0--expected or not--it will fail to put the byte back into the stream. That's *possibly* a bug... but it's not clear. As you say, the null is unexpected, so the assumption may be that if the file starts with a bunch of white space before the first line, and that whitespace is possibly terminated by a null, so long as the next character starts the mbox (i.e. contains "From "...) it's still a valid mbox. That seems like it's perfectly fine. This can only occur at the start of the file, and only if the character that immediately precedes the start of the mbox data is a NULL. And that's completely uninteresting. I actually think it would be fine to include the expression: || ch == '\0' in the while loop's conditional. It's harmless at worst, and may be useful. > ch will only be 0 if there's a NUL-byte in the file which > is unexpected for Mboxes but not a problem with the issue the > comment is talking about. It's unexpected but not at all impossible... This patch exists entirely to deal with a condition that is not expected. =8^) All in all, I think the original version of the patch is preferable. -- Derek D. Martin http://www.pizzashack.org/ GPG Key ID: 0xDFBEAD02 -=-=-=-=- This message is posted from an invalid address. Replying to it will result in undeliverable mail due to spam prevention. Sorry for the inconvenience.
pgppN3Kdz8zwr.pgp
Description: PGP signature
