Amended critical section locking to be NFS-safe

darcs-hash:20050930195021-344c5-9e4760fdb29c80e32589bf62159911a57c6779d2.gz

3 files changed, 230 insertions, 88 deletions

diff --git a/common.c b/common.c
index ee9b2d9f..8768f8e0 100644
--- a/common.c
+++ b/common.c
@@ -55,10 +55,10 @@ parts of fish.
 #define ERROR_MAX_COUNT 1
 
 /**
-   The number of nanoseconds to wait between polls when attempting to acquire 
+   The number of microseconds to wait between polls when attempting to acquire 
    a lockfile
 */
-#define LOCKPOLLINTERVAL 1000000
+#define LOCKPOLLINTERVAL 10
 
 struct termios shell_modes;      
 
@@ -1071,35 +1071,160 @@ wchar_t *unescape( wchar_t * in, int escape_special )
 }
 
 /**
+   Convert a pid_t to a decimal string representation.
+   The str parameter must contain sufficient space.
+   The conservatively approximate maximum number of characters a pid_t will 
+   represent is given by: (int)(3.1 * sizeof(pid_t) + CHAR_BIT + 1)
+   Returns the length of the string
+*/
+static int pidtostr( pid_t pid, char *str )
+{
+	int len, i = 0;
+	int dig;
+
+	/* Store digits in reverse order into string */
+	while( pid != 0 )
+	{
+		dig = pid % 10;
+		str[i] = '0' + dig;
+		pid = ( pid - dig ) / 10;
+		i++;
+	}
+	len = i;
+	/* Reverse digits */
+	i /= 2;
+	while( i )
+	{
+		i--;
+		dig = str[i];
+		str[i] = str[len -  1 - i];
+		str[len - 1 - i] = dig;
+	}
+	return len;
+}
+
+/**
+   Create a copy of str adding this process's decimalised pid
+   The memory returned should be freed using free()
+*/
+static char *dup_str_with_pid( const char *str )
+{
+	int pidlen, len = strlen( str );
+	/*
+	   allocate enough extra space for max size of pid when converted to a 
+	   string; 3.1 ~= log(10)2
+	*/
+	char *newstr = malloc( len + 1 + 
+			(int)(3.1 * sizeof(pid_t) * CHAR_BIT + 1) );
+	
+	if ( newstr == NULL )
+	{
+		return newstr;
+	}
+	memcpy( newstr, str, len );
+	pidlen = pidtostr( getpid(), newstr + len );
+	newstr[len + pidlen] = '\0';
+	return newstr;
+}
+
+/**
    Attempt to acquire a lock based on a lockfile, waiting LOCKPOLLINTERVAL 
    nanoseconds between polls and timing out after timeout seconds, 
-   thereafter forcibly attempting to obtain the lock.  Returns the opened 
-   lockfile's descriptor or -1 if not possible to open the file or on error.
-   Contains a race condition when the lockfile is on an NFS filesystem 
-   according to Linux manpage open(2)
+   thereafter forcibly attempting to obtain the lock.
+   Returns 1 on success, 0 on failure.
+   A temporary file based on adding the pid to the lockfile name is used, so 
+   be aware that such a file will be deleted if it already exists.
+   To remove the lock the lockfile must be unlinked.
 */
-int acquire_lock_file(const char *lockfile, const int timeout)
+int acquire_lock_file( const char *lockfile, const int timeout, int force )
 {
-	int ret = -1;
+	int fd, timed_out = 0;
+	int ret = 0; /* early exit returns failure */
 	struct timespec pollint;
 	struct timeval start, end;
 	double elapsed;
+	struct stat statbuf;
 
-	if (gettimeofday(&start, NULL) == 0) {
-		pollint.tv_sec = 0;
-		pollint.tv_nsec = LOCKPOLLINTERVAL;
-		while ((ret = open(lockfile, O_EXCL|O_CREAT|O_RDONLY)) == -1) {
-			if (gettimeofday(&end, NULL) != 0)
-				break;
-			elapsed = end.tv_sec + end.tv_usec/1000000.0 - 
-				(start.tv_sec + start.tv_usec/1000000.0);
-			if (elapsed >= timeout) {
-				(void)unlink(lockfile);
-				ret = open(lockfile, O_EXCL|O_CREAT|O_RDONLY);
+	/*
+	  (Re)create a unique file and check that it has one only link.
+	  The check would be subject to a race if the filename were not unique.
+	*/
+	char *linkfile = dup_str_with_pid( lockfile );
+	if( linkfile == NULL )
+	{
+		goto done;
+	}
+	(void)unlink( linkfile );
+	if( ( fd = open( linkfile, O_CREAT|O_RDONLY ) ) == -1 )
+	{
+		goto done;
+	}
+	close( fd );
+	if( stat( linkfile, &statbuf ) != 0 || statbuf.st_nlink != 1 )
+	{
+		goto done;
+	}
+
+	if( gettimeofday( &start, NULL ) != 0 )
+	{
+		goto done;
+	}
+	end = start;
+	pollint.tv_sec = 0;
+	pollint.tv_nsec = LOCKPOLLINTERVAL * 1000000;
+	do 
+	{
+		/*
+		  Try to create a hard link to the unique file from the 
+		  lockfile.  This will only succeed if the lockfile does not 
+		  already exist.  It is guaranteed to provide race-free 
+		  semantics over NFS which the alternative of calling 
+		  open(O_EXCL|O_CREAT) on the lockfile is not.  The lock 
+		  succeeds if the call to link returns 0 or the link count on 
+		  the unique file increases to 2.
+		*/
+		if( link( linkfile, lockfile ) == 0 ||
+		    ( stat( linkfile, &statbuf ) == 0 && 
+				statbuf.st_nlink == 2 ) )
+		{
+			/* Successful lock */
+			ret = 1;
+			break;
+		}
+		elapsed = end.tv_sec + end.tv_usec/1000000.0 - 
+			( start.tv_sec + start.tv_usec/1000000.0 );
+		/* 
+		  The check for elapsed < 0 is to deal with the unlikely event 
+		  that after the loop is entered the system time is set forward
+		  past the loop's end time.  This would otherwise result in a 
+		  (practically) infinite loop.
+		*/
+		if( timed_out || elapsed >= timeout || elapsed < 0 )
+		{
+			if ( timed_out == 0 && force )
+			{
+				/*
+				  Timed out and force was specified - attempt to
+				  remove stale lock and try a final time
+				*/
+				(void)unlink( lockfile );
+				timed_out = 1;
+				continue;
+			}
+			else
+			{
+				/*
+				  Timed out and final try was unsuccessful or 
+				  force was not specified
+				*/
 				break;
 			}
-			(void)nanosleep(&pollint, NULL);
-		}	
-	}
+		}
+		nanosleep( &pollint, NULL );
+	} while( gettimeofday( &end, NULL ) == 0 );
+done:
+	/* The linkfile is not needed once the lockfile has been created */
+	(void)unlink( linkfile );
+	free( linkfile );
 	return ret;
 }
diff --git a/common.h b/common.h
index 9e2ea82e..c5d60a8a 100644
--- a/common.h
+++ b/common.h
@@ -247,4 +247,4 @@ wchar_t *unescape( wchar_t * in, int escape_special );
 void block();
 void unblock();
 
-int acquire_lock_file(const char *lockfile, const int timeout);
+int acquire_lock_file( const char *lockfile, const int timeout, int force );
diff --git a/fishd.c b/fishd.c
index 8d26a109..2ef0df0a 100644
--- a/fishd.c
+++ b/fishd.c
@@ -73,106 +73,120 @@ static int sock;
 /**
    Constructs the fish socket filename
 */
-static char *get_socket_filename(void)
+static char *get_socket_filename()
 {
 	char *name;
-	char *dir = getenv("FISHD_SOCKET_DIR");
-	char *uname = getenv("USER");
+	char *dir = getenv( "FISHD_SOCKET_DIR" );
+	char *uname = getenv( "USER" );
 
-	if (dir == NULL)
+	if( dir == NULL )
+	{
 		dir = "/tmp";
+	}
 
-	if (uname == NULL) {
+	if( uname == NULL )
+	{
 		struct passwd *pw;
-		pw = getpwuid(getuid());
-		uname = strdup(pw->pw_name);
+		pw = getpwuid( getuid() );
+		uname = strdup( pw->pw_name );
 	}
 
-	name = malloc(strlen(dir)+ strlen(uname)+ strlen(SOCK_FILENAME) + 2);
-	if (name == NULL) {
-		perror("get_socket_filename");
-		exit(EXIT_FAILURE);
+	name = malloc( strlen(dir)+ strlen(uname)+ strlen(SOCK_FILENAME) + 2 );
+	if( name == NULL )
+	{
+		wperror( L"get_socket_filename" );
+		exit( EXIT_FAILURE );
 	}
-	strcpy(name, dir);
-	strcat(name, "/");
-	strcat(name, SOCK_FILENAME);
-	strcat(name, uname);
-
-	if (strlen(name) >= UNIX_PATH_MAX) {
-		debug(1, L"Filename too long: '%s'", name);
-		exit(EXIT_FAILURE);
+	strcpy( name, dir );
+	strcat( name, "/" );
+	strcat( name, SOCK_FILENAME );
+	strcat( name, uname );
+
+	if( strlen( name ) >= UNIX_PATH_MAX )
+	{
+		debug( 1, L"Filename too long: '%s'", name );
+		exit( EXIT_FAILURE );
 	}
 	return name;
 }
 
 /**
-   Acquire the lock file for the socket
-   Returns an fd that should be closed to unlock
+   Acquire the lock for the socket
+   Returns the name of the lock file if successful or 
+   NULL if unable to obtain lock.
+   The returned string must be free()d after unlink()ing the file to release 
+   the lock
 */
-int acquire_socket_lock_file(const char *sock_name, char **lockfile)
+static char *acquire_socket_lock( const char *sock_name )
 {
-	int fd;
-	int len = strlen(sock_name);
-
-	*lockfile = malloc(len + strlen(LOCKPOSTFIX) + 1);
-	if (*lockfile == NULL) {
-		perror("get_socket");
-		exit(EXIT_FAILURE);
+	int len = strlen( sock_name );
+	char *lockfile = malloc( len + strlen( LOCKPOSTFIX ) + 1 );
+	
+	if( lockfile == NULL ) 
+	{
+		wperror( L"acquire_socket_lock" );
+		exit( EXIT_FAILURE );
 	}
-	(void)strcpy(*lockfile, sock_name);
-	(void)strcpy(*lockfile + len, LOCKPOSTFIX);
-	if ((fd = acquire_lock_file(*lockfile, LOCKTIMEOUT)) == -1) {
-		fprintf(stderr, "Unable to obtain lockfile %s, exiting", 
-							*lockfile);
-		exit(EXIT_FAILURE);
+	strcpy( lockfile, sock_name );
+	strcpy( lockfile + len, LOCKPOSTFIX );
+	if ( !acquire_lock_file( lockfile, LOCKTIMEOUT, 1 ) )
+	{
+		free( lockfile );
+		lockfile = NULL;
 	}
-	return fd;
+	return lockfile;
 }
 
 /**
    Connects to the fish socket and starts listening for connections
 */
-static int get_socket(void)
+static int get_socket()
 {
 	int s, len, doexit = 0;
 	int exitcode = EXIT_FAILURE;
 	struct sockaddr_un local;
-	char *lockfile;
 	char *sock_name = get_socket_filename();
-	int fd_lockfile;
 
-	/**
-	   Start critical section protected by lock file
+	/*
+	   Start critical section protected by lock
 	*/
-	fd_lockfile = acquire_socket_lock_file(sock_name, &lockfile);
-	debug(1, L"Acquired lockfile %s", lockfile);
+	char *lockfile = acquire_socket_lock( sock_name );
+	if( lockfile == NULL )
+	{
+		fwprintf( stderr, L"Unable to obtain lock on socket, exiting" );
+		exit( EXIT_FAILURE );
+	}
+	debug( 1, L"Acquired lockfile: %s", lockfile );
 	
 	local.sun_family = AF_UNIX;
-	strcpy(local.sun_path, sock_name);
-	len = strlen(local.sun_path) + sizeof(local.sun_family);
+	strcpy( local.sun_path, sock_name );
+	len = strlen( local.sun_path ) + sizeof( local.sun_family );
 
 	debug(1, L"Connect to socket at %s", sock_name);
 	
-	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
-		perror("socket");
+	if( ( s = socket( AF_UNIX, SOCK_STREAM, 0 ) ) == -1 )
+	{
+		wperror( L"socket" );
 		doexit = 1;
 		goto unlock;
 	}
 
-	/**
+	/*
 	   First check whether the socket has been opened by another fishd;
 	   if so, exit with success status
 	*/
-	if (connect(s, (struct sockaddr *)&local, len) == 0) {
-		debug(1, L"Socket already exists, exiting");
+	if( connect( s, (struct sockaddr *)&local, len ) == 0 )
+	{
+		debug( 1, L"Socket already exists, exiting" );
 		doexit = 1;
 		exitcode = 0;
 		goto unlock;
 	}
 	
-	unlink(local.sun_path);
-	if (bind(s, (struct sockaddr *)&local, len) == -1) {
-		perror("bind");
+	unlink( local.sun_path );
+	if( bind( s, (struct sockaddr *)&local, len ) == -1 )
+	{
+		wperror( L"bind" );
 		doexit = 1;
 		goto unlock;
 	}
@@ -185,29 +199,32 @@ static int get_socket(void)
 	}
 */
 
-	if( fcntl( s, F_SETFL, O_NONBLOCK ) != 0 ) {
+	if( fcntl( s, F_SETFL, O_NONBLOCK ) != 0 )
+	{
 		wperror( L"fcntl" );
 		close( s );
 		doexit = 1;
-	} else if (listen(s, 64) == -1) {
-		wperror(L"listen");
+	} else if( listen( s, 64 ) == -1 )
+	{
+		wperror( L"listen" );
 		doexit = 1;
 	}
 
 unlock:
-	(void)close(fd_lockfile);
-	(void)unlink(lockfile);
-	debug(1, L"Released lockfile %s", lockfile);
-	/**
-	   End critical section protected by lock file
+	(void)unlink( lockfile );
+	debug( 1, L"Released lockfile: %s", lockfile );
+	/*
+	   End critical section protected by lock
 	*/
 	
-	free(lockfile);
+	free( lockfile );
 	
-	free(sock_name);
+	free( sock_name );
 
-	if (doexit)
-		exit(exitcode);
+	if( doexit )
+	{
+		exit( exitcode );
+	}
 
 	return s;
 }