Re: [pacman-dev] [PATCH] Fix double close of the lock file
On 5 February 2011 13:39, Jonathan Conder jonno.con...@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
#include stdio.h
#include alpm.h
int main(int argc, const char *argv[]) {
if(alpm_initialize() 0) {
return(1);
}
alpm_option_set_root(/);
alpm_option_set_dbpath(/tmp);
alpm_option_set_logfile(/tmp/pacman.log);
if(alpm_trans_init(0, NULL, NULL, NULL) 0) {
return(2);
}
alpm_logaction((char *) line 1\n);
if(alpm_trans_release() 0) {
return(3);
}
alpm_logaction((char *) line 2\n);
if(alpm_release() 0) {
return(4);
}
return(0);
}
Re: [pacman-dev] [PATCH] Fix double close of the lock file
On Sat, Feb 5, 2011 at 1:39 AM, Jonathan Conder jonno.con...@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.con...@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.xav...@gmail.com
Re: [pacman-dev] [PATCH] Fix double close of the lock file
- Original message - On Sat, Feb 5, 2011 at 1:39 AM, Jonathan Conder jonno.con...@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.con...@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.xav...@gmail.com
Re: [pacman-dev] [PATCH] Fix double close of the lock file
On 5 February 2011 13:39, Jonathan Conder jonno.con...@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.con...@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
Re: [pacman-dev] [PATCH] Fix double close of the lock file
On Fri, Feb 11, 2011 at 6:51 PM, Jonathan Conder jonno.con...@gmail.com wrote: On 5 February 2011 13:39, Jonathan Conder jonno.con...@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.con...@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
Re: [pacman-dev] [PATCH] Fix double close of the lock file
On 12 February 2011 14:52, Dan McGee dpmc...@gmail.com wrote: On Fri, Feb 11, 2011 at 6:51 PM, Jonathan Conder jonno.con...@gmail.com wrote: On 5 February 2011 13:39, Jonathan Conder jonno.con...@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.con...@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
