On Tue, Jan 17, 2017 at 07:15:49PM -0600, Derek Martin wrote: >> 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^)
Please re-read my comment. The issue is not a possible 0-byte in the file, but that your code breaks that 0-byte. > All in all, I think the original version of the patch is preferable. I disagree strongly, also see the following comments. Incorrect code is not preferable. On Tue, Jan 17, 2017 at 07:45:25PM -0600, Derek Martin wrote: > On Tue, Jan 17, 2017 at 07:15:49PM -0600, Derek Martin wrote: >> On Sun, Jan 15, 2017 at 11:33:41AM +0100, Simon Ruderich wrote: >> [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. > > Sorry, I realized I slightly misread the comment. On ERROR, it may > well put EOF into the file stream (though I'm not sure--the file is in > an error state so not sure what the actual behavior is here), though It won't put an EOF, but an 0xFF into the stream, which is a value not originally present in the file. > again that's hardly a concern. If the file can't be read due to an > error, there's no practical difference in the behavior. In both cases > the subsequent fgets() will fail due to the error, and return... you > guessed it, EOF. In either case, the subsequent strncmp() will be Except that the return value for the fgets() wasn't checked at all which means the result of the mutt_strncmp() was undefined behavior. Anyway, the code is still wrong and only because it doesn't break at the moment is no argument to keep it there. > compared to EOF, and the function will fail, return -1, and log that > it can't read the file. I can't understand why you're arguing against the improved version of the patch. I spotted a possible issue, reported it and it was fixed. So what's the problem? > It might be slightly preferable to change the if block to: > > if (!feof(f) && !ferror(f) && ch) > ungetc(ch, f); > > But it makes exactly zero difference. Also: > > if (!feof(f) && !ferror(f)) > ungetc(ch, f); > > Not clearly better. There's no need to check for feof() or ferror() as fgetc() returning EOF tells you everything you need to know. The "&& ch" is just plain wrong, see above. Please keep the code as it is now. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
signature.asc
Description: PGP signature
