Bug 1635 - safe_lock() doesn't check return status when unlinking temp lock file
Summary: safe_lock() doesn't check return status when unlinking temp lock file
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: 2.50
Hardware: Other other
: P5 normal
Target Milestone: 2.50
Assignee: Theo Van Dinter
URL:
Whiteboard:
Keywords: backport
Depends on:
Blocks:
 
Reported: 2003-03-14 05:20 UTC by Theo Van Dinter
Modified: 2003-03-15 01:20 UTC (History)
1 user (show)


Attachment Type Modified Status Actions Submitter/CLA Status
suggested patch patch None Theo Van Dinter [HasCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Theo Van Dinter 2003-03-14 05:20:58 UTC 
unlink $lock_tmp;

should actually be something like:

  unlink $lock_tmp || dbg("lock: $$ cannot remove temp lock file $lock_tmp: 
$!");
Comment 1 Theo Van Dinter 2003-03-14 05:21:39 UTC 
assigning to myself
Comment 2 Theo Van Dinter 2003-03-14 05:37:53 UTC 
Created attachment 745 [details]
suggested patch
Comment 3 Theo Van Dinter 2003-03-14 05:38:44 UTC 
attached patch, added backport keyword.  any comments? :)
Comment 4 Daniel Quinlan 2003-03-14 10:37:00 UTC 
ISSUE: you don't want to warn on the first unlink.  There's a race
condition.  Two or more processes could try to delete the stale lock
at the same time which is okay because they have yet to get the lock
and that's still safe.

You can warn on the second unlink if you want.
Comment 5 Theo Van Dinter 2003-03-14 10:49:13 UTC 
Subject: Re:  safe_lock() doesn't check return status when unlinking temp lock file

On Fri, Mar 14, 2003 at 10:37:01AM -0800, bugzilla-daemon@hughes-family.org wrote:
> ISSUE: you don't want to warn on the first unlink.  There's a race
> condition.  Two or more processes could try to delete the stale lock
> at the same time which is okay because they have yet to get the lock
> and that's still safe.

Ok, but what's the issue?  It's a warn, not a die.

There is a race condition, but putting up a warning is still the correct
behavior.  At that point in the code, we've already announced that we're
breaking the lock.  If the break doesn't happen (as the other process
has already broken it), we want to say "we didn't break the lock".

If that process then grabs the lock, it'll look wierd in the debug output
("trying to break lock", "failed to break lock", "got lock"), but no
more than the other version (no warn there) where the process says it'll
break the lock and the other process grabs it, then the debug output says
"trying to break lock", "waiting for lock" ...

The only two race conditions here is: process A says it'll break the
lock, and process B can get the lock between the break and lock, and
process A says it'll break the lock but can't because process B either
already broke it or gave up its lock.  A "warn" just properly gives the
state that "we tried to break the lock, but failed for some reason",
followed by the reason (file not found, permissions, etc.)
Comment 6 Daniel Quinlan 2003-03-14 11:08:43 UTC 
I just don't want to warn for anything that's not an error condition or
something to worry about.  Why open ourselves up for having to explain
that a warning can be ignored because it's harmless.  Just don't print
a warning if it's harmless!
Comment 7 Theo Van Dinter 2003-03-14 11:14:10 UTC 
Subject: Re:  safe_lock() doesn't check return status when unlinking temp lock file

On Fri, Mar 14, 2003 at 11:08:44AM -0800, bugzilla-daemon@hughes-family.org wrote:
> Just don't print a warning if it's harmless!

Well ... What's the purpose of a "warning" if you really mean "error"?
The whole point of a warning is that something happened that you probably
should be aware of (like "I can't remove this file").  Just because
the program doesn't abort because of it doesn't mean you shouldn't tell
people that something that should of worked failed.
Comment 8 Daniel Quinlan 2003-03-14 11:42:41 UTC 
OKAY

(I'd be happier if we exempted ENOENT since that's not an error.  Your call.
Even with ENOENT, I could go either way: leave it out or put it in.)
Comment 9 Malte S. Stretz 2003-03-15 10:20:33 UTC 
applied to STABLE