rlz/mac: Make sure a crashing process doesn't leave a stale lockfile behind.

rlz/mac/lib/rlz_value_store_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "rlz/mac/lib/rlz_value_store_mac.h"
 
 #include "base/mac/foundation_util.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/sys_string_conversions.h"
@@ skipping 217 matching lines @@
   // directory of |lock_file| must exist.
   bool TryGetCrossProcessLock(NSString* lock_filename);
 
   // Releases the lock. Should always be called, even if
   // TryGetCrossProcessLock() returns false.
   void ReleaseLock();
 
   pthread_mutex_t recursive_lock_;
   pthread_t locking_thread_;
 
-  NSDistributedLock* file_lock_;
+  int file_lock_;

[Scott Hess - ex-Googler, 2012/08/15 00:44:38]

ScopedFD?

[Nico, 2012/08/15 03:24:12]

It needs to be closed while the mutex is still held.

[Scott Hess - ex-Googler, 2012/08/15 19:55:04]

That's what reset() is for.

 } g_recursive_lock = {
   // PTHREAD_RECURSIVE_MUTEX_INITIALIZER doesn't exist before 10.7 and is buggy
   // on 10.7 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51906#c34), so emulate
   // recursive locking with a normal non-recursive mutex.
   PTHREAD_MUTEX_INITIALIZER
 };
 
 bool RecursiveCrossProcessLock::TryGetCrossProcessLock(
     NSString* lock_filename) {
   bool just_got_lock = false;
@@ skipping 11 matching lines @@
   }
 
   locking_thread_ = pthread_self();
 
   // Try to acquire file lock.
   if (just_got_lock) {
     const int kMaxTimeoutMS = 5000;  // Matches windows.
     const int kSleepPerTryMS = 200;
 
     CHECK(!file_lock_);
-    file_lock_ = [[NSDistributedLock alloc] initWithPath:lock_filename];
-
-    BOOL got_file_lock = NO;
-    int elapsedMS = 0;
-    while (!(got_file_lock = [file_lock_ tryLock]) &&
-           elapsedMS < kMaxTimeoutMS) {
-      usleep(kSleepPerTryMS * 1000);
-      elapsedMS += kSleepPerTryMS;
+    file_lock_ = open([lock_filename fileSystemRepresentation], O_CREAT);
+    if (file_lock_ == -1) {
+      file_lock_ = 0;

[Scott Hess - ex-Googler, 2012/08/15 00:44:38]

0 is a valid fd.  Suggest using -1 for invalid, just to be safe.

[Mark Mentovai, 2012/08/15 00:50:54]

But 0 is a valid FD. You should rewrite things to use -1 as the invalid lock
value (your initializer on lines 239-244, your CHECK on line 269, your reset on
line 285, your test on line 297, etc.)

[Nico, 2012/08/15 03:24:12]

Done. (But in practice 0 is stdin, so in practice this should've been fine as
is, right?)

[Mark Mentovai, 2012/08/15 13:36:28]

Nico wrote:
You can close stdin.

+      return false;
     }
 
-    if (!got_file_lock) {
-      [file_lock_ release];
-      file_lock_ = nil;
+    int flock_result = -1;
+    int elapsedMS = 0;

[Mark Mentovai, 2012/08/15 00:50:54]

elapsed_ms

[Nico, 2012/08/15 03:24:12]

Done.

+    while ((flock_result = flock(file_lock_, LOCK_EX | LOCK_NB)) == -1 &&

[Scott Hess - ex-Googler, 2012/08/15 00:44:38]

errno == EWOULDBLOCK &&

Then you can lift the if() clause and drop the else clause because it's already
handled outside the loop.

[Mark Mentovai, 2012/08/15 00:50:54]

HANDLE_EINTR

[Nico, 2012/08/15 03:24:12]

Done.

+           elapsedMS < kMaxTimeoutMS) {
+      if (errno == EWOULDBLOCK) {
+        usleep(kSleepPerTryMS * 1000);
+        elapsedMS += kSleepPerTryMS;

[Scott Hess - ex-Googler, 2012/08/15 00:44:38]

usleep() could be interrupted by a signal, in which case elapsedMS would be
bogus.  Admittedly at 200ms per shot, not super bogus, but still.  Can you use
TimeTicks() or something or the sort to drive this?  Though I guess it's just as
good as before.

Also, how much contention could there be?  As implemented, someone could get
denied the lock while someone else jumps in after and claims it.  Though, again,
just as good as before.

[Nico, 2012/08/15 03:24:12]

Added HANDLE_EINTR here too.
In practice, chrome is the only app that uses rlz. (In theory, both chrome and
canary could write to it, but canary is always unbranded and shouldn't use rlz),
so I think that's fine as is.

(Out of interest: What's a better way to do this, should there be contention?)

[Mark Mentovai, 2012/08/15 13:36:28]

HANDLE_EINTR doesn’t really help here because the goal is to get a timed sleep.
In fact, HANDLE_EINTR may hurt because it will cause any interrupted sleep to be
restarted for the full kSleepPerTryMS milliseconds, and in a pathological
“signal storm,” you’ll never leave HANDLE_EINTR’s loop around usleep.

[Nico, 2012/08/15 14:52:50]

Good (pathological) point! Removed it again.

[Scott Hess - ex-Googler, 2012/08/15 19:55:04]

I think the POSIXy way is something like:

  int end_time = time() + kTimeout;
  do {
    alarm(end_time - time());
    rc = flock(fd_, LOCK_EX);
  } while (rc == -1 && errno == EINTR && time() < end_time);
    
Of course, it needs to be rewritten with gettimeofday() and setitimer() to give
better resolution, and someone would need to verify whether this works right
with threading.

Aaaaaand ... I guess it's still open for contention if you have other signals in
the system.  But if you're lucky enough not to have other signals, and not to
need to re-lock while a previous lock's alarm is still pending, then you'll just
wait until either you get the lock or the alarm fires.  So another process
couldn't snipe you.

IOW, let's not go there.

[Mark Mentovai, 2012/08/15 20:18:39]

shess wrote:
Even without other signals, there’s no explicit guarantee that the process that
ultimately wins the exclusive lock will be the first process to request it that
blocked, so you’d be relying on an implementation detail (that incidentally is
shared by the Mac and Linux kernels).

+      } else {
+        close(file_lock_);
+        file_lock_ = 0;
+        return false;
+      }
+    }
+
+    if (flock_result == -1) {
+      close(file_lock_);

[Mark Mentovai, 2012/08/15 00:50:54]

This duplicates what’s in the loop. I think you only need this copy.

[Nico, 2012/08/15 03:24:12]

Done, together with shess's suggestion above.

+      file_lock_ = 0;
       return false;
     }
     return true;
   } else {
-    return file_lock_ != nil;
+    return file_lock_ != 0;
   }
 }
 
 void RecursiveCrossProcessLock::ReleaseLock() {
   if (file_lock_) {
-    [file_lock_ unlock];
-    [file_lock_ release];
-    file_lock_ = nil;
+    flock(file_lock_, LOCK_UN);
+    close(file_lock_);
+    file_lock_ = 0;
   }
 
   locking_thread_ = 0;
   pthread_mutex_unlock(&recursive_lock_);
 }
 
 
 // This is set during test execution, to write RLZ files into a temporary
 // directory instead of the user's Application Support folder.
 NSString* g_test_folder;
@@ skipping 34 matching lines @@
 // Returns the path of the rlz plist store, also creates the parent directory
 // path if it doesn't exist.
 NSString* RlzPlistFilename() {
   NSString* const kRlzFile = @"RlzStore.plist";
   return [CreateRlzDirectory() stringByAppendingPathComponent:kRlzFile];
 }
 
 // Returns the path of the rlz lock file, also creates the parent directory
 // path if it doesn't exist.
 NSString* RlzLockFilename() {
-  NSString* const kRlzFile = @"lockfile";
+  NSString* const kRlzFile = @"flockfile";
   return [CreateRlzDirectory() stringByAppendingPathComponent:kRlzFile];
 }
 
 }  // namespace
 
 ScopedRlzValueStoreLock::ScopedRlzValueStoreLock() {
   bool got_distributed_lock =
       g_recursive_lock.TryGetCrossProcessLock(RlzLockFilename());
   // At this point, we hold the in-process lock, no matter the value of
   // |got_distributed_lock|.
@@ skipping 77 matching lines @@
   } else {
     // Not Unsafe on OS X.
     g_test_folder =
       [[NSString alloc] initWithUTF8String:directory.AsUTF8Unsafe().c_str()];
   }
 }
 
 }  // namespace testing
 
 }  // namespace rlz_lib