Sloppy error checking in MS code

[Glenn Steen]

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.

-- 
-- Glenn
email: glenn < dot > steen < at > gmail < dot > com
work: glenn < dot > steen < at > ap1 < dot > se