Sloppy error checking in MS code

[Philip Butler]

I am going to start carrying a stick to bed to check for the floor.   
I am doing that tonight.

My new year's resolution (for the year 2040 probably) is to check all  
of my return codes....  :)

Phil

On Dec 16, 2006, at 2:33 PM, Julian Field wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
>
> Ken A wrote:
>> 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,
> Thanks for backing me up. Yes, I don't check the return value of every
> single call I make, but show me a programmer who does? (No doubt  
> someone
> will at this point). The most common point that is made is "what if  
> the
> system runs out of space" at this point, and all sorts of things  
> will be
> failing at this time, there isn't any need to create more errors at  
> this
> point, they just create noise. Yes, I quite happily admit I don't  
> check
> the result from everything I do. But if you want a practical piece of
> software that runs at a reasonable speed.
>
> Don't forget. Every morning you make the assumption that the floor is
> there when you step out of bed. Show me the person who checks with a
> stick that the floor is where they think it is, before putting their
> weight on the first foot out of bed in the morning. Yes, you make
> assumptions too.
>
> Jules.
>
> Jules
>
> - --
> Julian Field MEng CITP
> www.MailScanner.info
> Buy the MailScanner book at www.MailScanner.info/store
>
> MailScanner customisation, or any advanced system administration help?
> Contact me at Jules at Jules.FM
>
> PGP footprint: EE81 D763 3DB0 0BFD E1DC 7222 11F6 5947 1415 B654
> For all your IT requirements visit www.transtec.co.uk
>
>
>
> -----BEGIN PGP SIGNATURE-----
> Version: PGP Desktop 9.5.2 (Build 4075)
> Comment: Fetch my public key foot-print from www.mailscanner.info
> Charset: ISO-8859-1
>
> wj8DBQFFhEpdEfZZRxQVtlQRAmMbAKDhK1tvfzEssr/qAW4ZoitNlKU5hgCgg5nX
> 6Sg7uid31y+O0TBPmXSGCWc=
> =7gKa
> -----END PGP SIGNATURE-----
>
> -- 
> This message has been scanned for viruses and
> dangerous content by MailScanner, and is
> believed to be clean.
> For all your IT requirements visit www.transtec.co.uk
>
> --
> MailScanner mailing list
> mailscanner at lists.mailscanner.info
> http://lists.mailscanner.info/mailman/listinfo/mailscanner
>
> Before posting, read http://wiki.mailscanner.info/posting
>
> Support MailScanner development - buy the book off the website!