[pacman-dev] [PATCH] alpm_handle: store lock file descriptor
There is a brief window between opening the file descriptor and creating a stream to it. If the process was interrupted during that window the lock file will not be removed correctly. Tracking the file descriptor allows us to remove the lock file regardless of whether or not the stream was ever created. Fixes FS#35603 Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/handle.c | 29 +++++++++++++++++------------ lib/libalpm/handle.h | 1 + 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 53c86c5..c9a70c9 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -91,11 +91,10 @@ void _alpm_handle_free(alpm_handle_t *handle) /** Lock the database */ int _alpm_handle_lock(alpm_handle_t *handle) { - int fd; char *dir, *ptr; ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream == NULL, return 0); + ASSERT(handle->lockfd < 0, return 0); /* create the dir of the lockfile first */ dir = strdup(handle->lockfile); @@ -110,14 +109,13 @@ int _alpm_handle_lock(alpm_handle_t *handle) FREE(dir); do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); - } while(fd == -1 && errno == EINTR); - if(fd >= 0) { - FILE *f = fdopen(fd, "w"); - fprintf(f, "%ld\n", (long)getpid()); - fflush(f); - fsync(fd); - handle->lckstream = f; + handle->lockfd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + } while(handle->lockfd == -1 && errno == EINTR); + if(handle->lockfd >= 0) { + handle->lckstream = fdopen(handle->lockfd, "w"); + fprintf(handle->lckstream, "%ld\n", (long)getpid()); + fflush(handle->lckstream); + fsync(handle->lockfd); return 0; } return -1; @@ -127,10 +125,17 @@ int _alpm_handle_lock(alpm_handle_t *handle) int _alpm_handle_unlock(alpm_handle_t *handle) { ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream != NULL, return 0); + ASSERT(handle->lockfd >= 0, return 0); + + if(handle->lckstream) { + fclose(handle->lckstream); + } else { + /* no stream was opened, close the file descriptor directly */ + close(handle->lockfd); + } - fclose(handle->lckstream); handle->lckstream = NULL; + handle->lockfd = -1; if(unlink(handle->lockfile) && errno != ENOENT) { return -1; diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5e84d58..0395db9 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -55,6 +55,7 @@ struct __alpm_handle_t { alpm_db_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (alpm_db_t *) */ FILE *logstream; /* log file stream pointer */ + int lockfd; /* lock file descriptor */ FILE *lckstream; /* lock file stream pointer if one exists */ alpm_trans_t *trans; -- 1.8.4.1
There was a brief window between opening the file descriptor and creating a stream to it. If the process was interrupted during that window the lock file would not be removed correctly. The pid is no longer printed to the lock file as this was virtually meaningless for lock files on NFS. Fixes FS#35603 Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- Don't print the pid to the lock file per discussion on IRC. lib/libalpm/handle.c | 24 ++++++++---------------- lib/libalpm/handle.h | 2 +- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 53c86c5..3dc5357 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -91,11 +91,10 @@ void _alpm_handle_free(alpm_handle_t *handle) /** Lock the database */ int _alpm_handle_lock(alpm_handle_t *handle) { - int fd; char *dir, *ptr; ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream == NULL, return 0); + ASSERT(handle->lockfd < 0, return 0); /* create the dir of the lockfile first */ dir = strdup(handle->lockfile); @@ -110,27 +109,20 @@ int _alpm_handle_lock(alpm_handle_t *handle) FREE(dir); do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); - } while(fd == -1 && errno == EINTR); - if(fd >= 0) { - FILE *f = fdopen(fd, "w"); - fprintf(f, "%ld\n", (long)getpid()); - fflush(f); - fsync(fd); - handle->lckstream = f; - return 0; - } - return -1; + handle->lockfd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + } while(handle->lockfd == -1 && errno == EINTR); + + return (handle->lockfd >= 0 ? 0 : -1); } /** Remove a lock file */ int _alpm_handle_unlock(alpm_handle_t *handle) { ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream != NULL, return 0); + ASSERT(handle->lockfd >= 0, return 0); - fclose(handle->lckstream); - handle->lckstream = NULL; + close(handle->lockfd); + handle->lockfd = -1; if(unlink(handle->lockfile) && errno != ENOENT) { return -1; diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5e84d58..a4e2cf7 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -55,7 +55,7 @@ struct __alpm_handle_t { alpm_db_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (alpm_db_t *) */ FILE *logstream; /* log file stream pointer */ - FILE *lckstream; /* lock file stream pointer if one exists */ + int lockfd; /* lock file descriptor */ alpm_trans_t *trans; #ifdef HAVE_LIBCURL -- 1.8.4.1
There was a brief window between opening the file descriptor and creating a stream to it. If the process was interrupted during that window the lock file would not be removed correctly. The pid is no longer printed to the lock file as this was virtually meaningless for lock files on NFS. Fixes FS#35603 Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- Initialize lockfd = -1 because using stdin as the lock is bad... lib/libalpm/handle.c | 25 +++++++++---------------- lib/libalpm/handle.h | 2 +- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 53c86c5..4871e1e 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -44,6 +44,7 @@ alpm_handle_t *_alpm_handle_new(void) CALLOC(handle, 1, sizeof(alpm_handle_t), return NULL); handle->deltaratio = 0.0; + handle->lockfd = -1; return handle; } @@ -91,11 +92,10 @@ void _alpm_handle_free(alpm_handle_t *handle) /** Lock the database */ int _alpm_handle_lock(alpm_handle_t *handle) { - int fd; char *dir, *ptr; ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream == NULL, return 0); + ASSERT(handle->lockfd < 0, return 0); /* create the dir of the lockfile first */ dir = strdup(handle->lockfile); @@ -110,27 +110,20 @@ int _alpm_handle_lock(alpm_handle_t *handle) FREE(dir); do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); - } while(fd == -1 && errno == EINTR); - if(fd >= 0) { - FILE *f = fdopen(fd, "w"); - fprintf(f, "%ld\n", (long)getpid()); - fflush(f); - fsync(fd); - handle->lckstream = f; - return 0; - } - return -1; + handle->lockfd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + } while(handle->lockfd == -1 && errno == EINTR); + + return (handle->lockfd >= 0 ? 0 : -1); } /** Remove a lock file */ int _alpm_handle_unlock(alpm_handle_t *handle) { ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream != NULL, return 0); + ASSERT(handle->lockfd >= 0, return 0); - fclose(handle->lckstream); - handle->lckstream = NULL; + close(handle->lockfd); + handle->lockfd = -1; if(unlink(handle->lockfile) && errno != ENOENT) { return -1; diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5e84d58..a4e2cf7 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -55,7 +55,7 @@ struct __alpm_handle_t { alpm_db_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (alpm_db_t *) */ FILE *logstream; /* log file stream pointer */ - FILE *lckstream; /* lock file stream pointer if one exists */ + int lockfd; /* lock file descriptor */ alpm_trans_t *trans; #ifdef HAVE_LIBCURL -- 1.8.4.1
On 27/10/13 01:32, Andrew Gregory wrote:
There was a brief window between opening the file descriptor and creating a stream to it. If the process was interrupted during that window the lock file would not be removed correctly.
The pid is no longer printed to the lock file as this was virtually meaningless for lock files on NFS.
Fixes FS#35603
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
Can someone else also look at commit acd92694 and ensure we are not regressing here. I think this is fine...
Initialize lockfd = -1 because using stdin as the lock is bad...
lib/libalpm/handle.c | 25 +++++++++---------------- lib/libalpm/handle.h | 2 +- 2 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 53c86c5..4871e1e 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -44,6 +44,7 @@ alpm_handle_t *_alpm_handle_new(void)
CALLOC(handle, 1, sizeof(alpm_handle_t), return NULL); handle->deltaratio = 0.0; + handle->lockfd = -1;
return handle; } @@ -91,11 +92,10 @@ void _alpm_handle_free(alpm_handle_t *handle) /** Lock the database */ int _alpm_handle_lock(alpm_handle_t *handle) { - int fd; char *dir, *ptr;
ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream == NULL, return 0); + ASSERT(handle->lockfd < 0, return 0);
/* create the dir of the lockfile first */ dir = strdup(handle->lockfile); @@ -110,27 +110,20 @@ int _alpm_handle_lock(alpm_handle_t *handle) FREE(dir);
do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); - } while(fd == -1 && errno == EINTR); - if(fd >= 0) { - FILE *f = fdopen(fd, "w"); - fprintf(f, "%ld\n", (long)getpid()); - fflush(f); - fsync(fd); - handle->lckstream = f; - return 0; - } - return -1; + handle->lockfd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + } while(handle->lockfd == -1 && errno == EINTR); + + return (handle->lockfd >= 0 ? 0 : -1); }
/** Remove a lock file */ int _alpm_handle_unlock(alpm_handle_t *handle) { ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream != NULL, return 0); + ASSERT(handle->lockfd >= 0, return 0);
- fclose(handle->lckstream); - handle->lckstream = NULL; + close(handle->lockfd); + handle->lockfd = -1;
if(unlink(handle->lockfile) && errno != ENOENT) { return -1; diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5e84d58..a4e2cf7 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -55,7 +55,7 @@ struct __alpm_handle_t { alpm_db_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (alpm_db_t *) */ FILE *logstream; /* log file stream pointer */ - FILE *lckstream; /* lock file stream pointer if one exists */ + int lockfd; /* lock file descriptor */ alpm_trans_t *trans;
#ifdef HAVE_LIBCURL
On 29/10/13 15:53, Allan McRae wrote:
On 27/10/13 01:32, Andrew Gregory wrote:
There was a brief window between opening the file descriptor and creating a stream to it. If the process was interrupted during that window the lock file would not be removed correctly.
The pid is no longer printed to the lock file as this was virtually meaningless for lock files on NFS.
Fixes FS#35603
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
Can someone else also look at commit acd92694 and ensure we are not regressing here. I think this is fine...
Updating the test case that was posted with commit acd92694 [1] shows this is not regressing. [1] https://mailman.archlinux.org/pipermail/pacman-dev/2011-February/012552.html Pulled to my working branch. A
participants (2)
-
Allan McRae
-
Andrew Gregory