[pacman-dev] [PATCH] Fix double close of the lock file
According to FOPEN(3), using fclose on an fdopen'd file stream also closes the underlying file descriptor. This happened in _alpm_lckmk (util.c), which meant that when alpm_trans_release closed it again, the log file (which reused the original file descriptor) was closed instead. Signed-off-by: Jonathan Conder <jonno.conder@gmail.com> --- lib/libalpm/handle.c | 1 - lib/libalpm/handle.h | 2 +- lib/libalpm/trans.c | 13 +++++-------- lib/libalpm/util.c | 7 +++---- lib/libalpm/util.h | 2 +- 5 files changed, 10 insertions(+), 15 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index aa34cf4..8f44e94 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -49,7 +49,6 @@ pmhandle_t *_alpm_handle_new() ALPM_LOG_FUNC; CALLOC(handle, 1, sizeof(pmhandle_t), RET_ERR(PM_ERR_MEMORY, NULL)); - handle->lckfd = -1; return(handle); } diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 1834994..6884666 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -34,7 +34,7 @@ typedef struct _pmhandle_t { pmdb_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (pmdb_t *) */ FILE *logstream; /* log file stream pointer */ - int lckfd; /* lock file descriptor if one exists */ + FILE *lckstream; /* lock file stream pointer if one exists */ pmtrans_t *trans; /* callback functions */ diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 02612ec..e13a7b2 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -73,8 +73,8 @@ int SYMEXPORT alpm_trans_init(pmtransflag_t flags, /* lock db */ if(!(flags & PM_TRANS_FLAG_NOLOCK)) { - handle->lckfd = _alpm_lckmk(); - if(handle->lckfd == -1) { + handle->lckstream = _alpm_lckmk(); + if(handle->lckstream == NULL) { RET_ERR(PM_ERR_HANDLE_LOCK, -1); } } @@ -260,12 +260,9 @@ int SYMEXPORT alpm_trans_release() /* unlock db */ if(!nolock_flag) { - if(handle->lckfd != -1) { - int fd; - do { - fd = close(handle->lckfd); - } while(fd == -1 && errno == EINTR); - handle->lckfd = -1; + if(handle->lckstream != NULL) { + fclose(handle->lckstream); + handle->lckstream = NULL; } if(_alpm_lckrm()) { _alpm_log(PM_LOG_WARNING, _("could not remove lock file %s\n"), diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 32eaa44..4d152ee 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -197,7 +197,7 @@ char *_alpm_strtrim(char *str) } /* Create a lock file */ -int _alpm_lckmk() +FILE *_alpm_lckmk() { int fd; char *dir, *ptr; @@ -220,10 +220,9 @@ int _alpm_lckmk() fprintf(f, "%ld\n", (long)getpid()); fflush(f); fsync(fd); - fclose(f); - return(fd); + return(f); } - return(-1); + return(NULL); } /* Remove a lock file */ diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 8a3154a..edd51d5 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -62,7 +62,7 @@ int _alpm_makepath(const char *path); int _alpm_makepath_mode(const char *path, mode_t mode); int _alpm_copyfile(const char *src, const char *dest); char *_alpm_strtrim(char *str); -int _alpm_lckmk(); +FILE *_alpm_lckmk(); int _alpm_lckrm(); int _alpm_unpack_single(const char *archive, const char *prefix, const char *fn); int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int breakfirst); -- 1.7.4
On 5 February 2011 13:39, Jonathan Conder <jonno.conder@gmail.com> wrote:
According to FOPEN(3), using fclose on an fdopen'd file stream also closes the underlying file descriptor. This happened in _alpm_lckmk (util.c), which meant that when alpm_trans_release closed it again, the log file (which reused the original file descriptor) was closed instead.
Signed-off-by: Jonathan Conder <jonno.conder@gmail.com> ---
Could I get at least an ack for this please? I sent it a week ago and haven't heard anything.
lib/libalpm/handle.c | 1 - lib/libalpm/handle.h | 2 +- lib/libalpm/trans.c | 13 +++++-------- lib/libalpm/util.c | 7 +++---- lib/libalpm/util.h | 2 +- 5 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index aa34cf4..8f44e94 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -49,7 +49,6 @@ pmhandle_t *_alpm_handle_new() ALPM_LOG_FUNC;
CALLOC(handle, 1, sizeof(pmhandle_t), RET_ERR(PM_ERR_MEMORY, NULL)); - handle->lckfd = -1;
return(handle); } diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 1834994..6884666 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -34,7 +34,7 @@ typedef struct _pmhandle_t { pmdb_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (pmdb_t *) */ FILE *logstream; /* log file stream pointer */ - int lckfd; /* lock file descriptor if one exists */ + FILE *lckstream; /* lock file stream pointer if one exists */ pmtrans_t *trans;
/* callback functions */ diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 02612ec..e13a7b2 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -73,8 +73,8 @@ int SYMEXPORT alpm_trans_init(pmtransflag_t flags,
/* lock db */ if(!(flags & PM_TRANS_FLAG_NOLOCK)) { - handle->lckfd = _alpm_lckmk(); - if(handle->lckfd == -1) { + handle->lckstream = _alpm_lckmk(); + if(handle->lckstream == NULL) { RET_ERR(PM_ERR_HANDLE_LOCK, -1); } } @@ -260,12 +260,9 @@ int SYMEXPORT alpm_trans_release()
/* unlock db */ if(!nolock_flag) { - if(handle->lckfd != -1) { - int fd; - do { - fd = close(handle->lckfd); - } while(fd == -1 && errno == EINTR); - handle->lckfd = -1; + if(handle->lckstream != NULL) { + fclose(handle->lckstream); + handle->lckstream = NULL; } if(_alpm_lckrm()) { _alpm_log(PM_LOG_WARNING, _("could not remove lock file %s\n"), diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 32eaa44..4d152ee 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -197,7 +197,7 @@ char *_alpm_strtrim(char *str) }
/* Create a lock file */ -int _alpm_lckmk() +FILE *_alpm_lckmk() { int fd; char *dir, *ptr; @@ -220,10 +220,9 @@ int _alpm_lckmk() fprintf(f, "%ld\n", (long)getpid()); fflush(f); fsync(fd); - fclose(f); - return(fd); + return(f); } - return(-1); + return(NULL); }
/* Remove a lock file */ diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 8a3154a..edd51d5 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -62,7 +62,7 @@ int _alpm_makepath(const char *path); int _alpm_makepath_mode(const char *path, mode_t mode); int _alpm_copyfile(const char *src, const char *dest); char *_alpm_strtrim(char *str); -int _alpm_lckmk(); +FILE *_alpm_lckmk(); int _alpm_lckrm(); int _alpm_unpack_single(const char *archive, const char *prefix, const char *fn); int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int breakfirst); -- 1.7.4
On Fri, Feb 11, 2011 at 6:51 PM, Jonathan Conder <jonno.conder@gmail.com> wrote:
On 5 February 2011 13:39, Jonathan Conder <jonno.conder@gmail.com> wrote:
According to FOPEN(3), using fclose on an fdopen'd file stream also closes the underlying file descriptor. This happened in _alpm_lckmk (util.c), which meant that when alpm_trans_release closed it again, the log file (which reused the original file descriptor) was closed instead.
Signed-off-by: Jonathan Conder <jonno.conder@gmail.com> ---
Could I get at least an ack for this please? I sent it a week ago and haven't heard anything.
Yeah I just haven't had a chance to look closely, but I haven't lost track of it. -Dan
On 12 February 2011 14:52, Dan McGee <dpmcgee@gmail.com> wrote:
On Fri, Feb 11, 2011 at 6:51 PM, Jonathan Conder <jonno.conder@gmail.com> wrote:
On 5 February 2011 13:39, Jonathan Conder <jonno.conder@gmail.com> wrote:
According to FOPEN(3), using fclose on an fdopen'd file stream also closes the underlying file descriptor. This happened in _alpm_lckmk (util.c), which meant that when alpm_trans_release closed it again, the log file (which reused the original file descriptor) was closed instead.
Signed-off-by: Jonathan Conder <jonno.conder@gmail.com> ---
Could I get at least an ack for this please? I sent it a week ago and haven't heard anything.
Yeah I just haven't had a chance to look closely, but I haven't lost track of it.
Ok, thanks. No rush. Jonathan
On 5 February 2011 13:39, Jonathan Conder <jonno.conder@gmail.com> wrote:
According to FOPEN(3), using fclose on an fdopen'd file stream also closes the underlying file descriptor. This happened in _alpm_lckmk (util.c), which meant that when alpm_trans_release closed it again, the log file (which reused the original file descriptor) was closed instead.
I wrote a small test case for this in order to better explain the bug. You can compile it with gcc test.c -o test -Wall -pedantic -lalpm It will create a log file at /tmp/pacman.log and can be run without root privileges. The expected contents of this log should be two lines, as you can probably tell from the source. However, without my patch the second line is not logged. Thanks, Jonathan
On Sat, Feb 5, 2011 at 1:39 AM, Jonathan Conder <jonno.conder@gmail.com> wrote:
According to FOPEN(3), using fclose on an fdopen'd file stream also closes the underlying file descriptor. This happened in _alpm_lckmk (util.c), which meant that when alpm_trans_release closed it again, the log file (which reused the original file descriptor) was closed instead.
Signed-off-by: Jonathan Conder <jonno.conder@gmail.com> ---
So no one ever spotted this because logs are only made during a transaction ? Anyway nice finding :) Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com>
----- Original message -----
On Sat, Feb 5, 2011 at 1:39 AM, Jonathan Conder <jonno.conder@gmail.com> wrote:
According to FOPEN(3), using fclose on an fdopen'd file stream also closes the underlying file descriptor. This happened in _alpm_lckmk (util.c), which meant that when alpm_trans_release closed it again, the log file (which reused the original file descriptor) was closed instead.
Signed-off-by: Jonathan Conder <jonno.conder@gmail.com> ---
So no one ever spotted this because logs are only made during a transaction ? Anyway nice finding :)
Pretty much, yeah. Even then it might not affect pacman if the log file gets opened before a transaction is initialized. However, it does affect my frontend, and maybe it would have caused other bugs in future. Thanks, it was a tough one to track down :)
Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com>
participants (3)
-
Dan McGee
-
Jonathan Conder
-
Xavier Chantry