Sloppy error checking in MS code

[Ken A]

Glenn Steen wrote:
> On 16/12/06, Matt Kettler <mkettler at evi-inc.com> wrote:
>> Mike Jakubik wrote:
>> > Recently on the postfix mailling lists, there was a debate on the usage
>> > of postfix  + mailscanner. I claimed that I've been using the two for
>> > some time now, without any problems. However one of the folks on the
>> > lists stated the following:
>> >
>> > ---
>> > On Thursday December 14 2006 07:30, Mike Jakubik wrote:
>> >> > Could you backup this documented fact please? Are you sure you 
>> are not
>> >> > referring to a bug in an obsolete version of mailscanner? If this
>> > was an
>> >> > issue, im quite certain some of my users would complain.
>> >
>> > Look at its code. There are literally hundreds of instances where a
>> > status from a system call is just ignored (getline, print, close, 
>> flush,
>> > stat, mkdir, chown, unlink, exec, system..., sometimes even: open).
>> > _Anything_ can happen, and you will never know what hit you,
>> > much less be able to track it down easily, especially if the
>> > problem is intermittent.
>> >
>> >  Mark
>> > ---
>> >
>> > I'm not a perl expert, could someone verify if this is the case? Can MS
>> > really eat emails without any warning?
>>
>> Hmm, reading PFDiskStore.pm, it does look like many system calls 
>> aren't checked.
>>
>> Some cases, it doesn't really matter, because a later system call is 
>> checked,
>> and that will fail if the previous one did. (ie: mkdir, followed by a 
>> rename
>> into that dir, where mkdir isn't checked, but rename is. Clearly if 
>> the mkdir
>> failed to make the directory, rename is going to fail and get caught 
>> that way)
>>
>> There are some other cases where I can't tell if it's bad enough to cause
>> problems, or just bad form.
>>
>> I suspect that  the un-checked chown and chmod in this file could be 
>> bad if they
>> failed.. they *shouldn't* ever fail.. but at the same time, it would 
>> be nice to
>> check for failure and generate a message.
>>
>> It also would be nice if the unlink in DeleteUnlock() was checked, 
>> much like it
>> is in the SMDiskStore.pm for sendmail.
>>
>> That said, my code is NOT the latest, and Julian may have already 
>> fixed both of
>> the above..
>>
> I've done no such audit as you've started Matt... But really, if this
> was a big problem, I'm more than certain we would've ... encoutered
> ... it already.
> I'm not saying that code audits shouldn't be done, and I'm usually a
> stickler for sane error checking/handling. But I can't shake the
> feeling this is motly just more of the usual FUD from the postfix
> devel camp.
> 
> Since switching to the HOLD method, I've only lost messages when I
> fat-fingered the MS config... And Jules has subsequently
> "idiot-proofed" those settings (the * Actions). Might be just dumb
> luck, I suppose, that it hasn't happened, but I doubt it:-).
> 
> That said, I'm really in forward of critically looking the PF code
> over. I've been doing some looking, but then mostly to ascertain how
> it works (or in some few cases... not), and to try determine whether
> the PF crowd have any base for some of their allegations (unsafe
> handling of queue files... AFAICS, this is untrue).
> Unforunately I will have very limited time to help before X-mas/new 
> years...
> Then again, I'm not sure it's a problem needing immediate attention.
> 

This kind of thing is really up to the author. I'm sure it's something 
he's thought about and not done for a reason. One person's 'sloppy' is 
another's 'efficient'. Programmers can tend to get a bit over zealous 
about such issues. More often than not, the issue that has a /potential/ 
to happen, never really happens in the software's lifetime. So the only 
_real_ issue is a theoretical, academic argument that most of us have no 
time for. :-\
Ken A
Pacific.Net