[dovecot-cvs] dovecot/src/lib-index mail-transaction-log-append.c, 1.17.2.1, 1.17.2.2 mail-transaction-log.c, 1.111.2.5, 1.111.2.6

tss at dovecot.org tss at dovecot.org
Tue Jan 16 21:58:51 UTC 2007


Update of /var/lib/cvs/dovecot/src/lib-index
In directory talvi:/tmp/cvs-serv1078

Modified Files:
      Tag: branch_1_0
	mail-transaction-log-append.c mail-transaction-log.c 
Log Message:
Fixed a race condition in transaction log rotation, which could have caused
log data to be overwritten.



Index: mail-transaction-log-append.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-index/mail-transaction-log-append.c,v
retrieving revision 1.17.2.1
retrieving revision 1.17.2.2
diff -u -d -r1.17.2.1 -r1.17.2.2
--- mail-transaction-log-append.c	11 Jun 2006 19:01:28 -0000	1.17.2.1
+++ mail-transaction-log-append.c	16 Jan 2007 21:58:48 -0000	1.17.2.2
@@ -360,9 +360,10 @@
 	 (log)->head->sync_offset == (idx_hdr)->log_file_int_offset && \
 	 (log)->head->sync_offset == (idx_hdr)->log_file_ext_offset)
 
-int mail_transaction_log_append(struct mail_index_transaction *t,
-				uint32_t *log_file_seq_r,
-				uoff_t *log_file_offset_r)
+static int
+mail_transaction_log_append_locked(struct mail_index_transaction *t,
+				   uint32_t *log_file_seq_r,
+				   uoff_t *log_file_offset_r)
 {
 	struct mail_index_view *view = t->view;
 	struct mail_index *index;
@@ -377,26 +378,12 @@
 	index = mail_index_view_get_index(view);
 	log = index->log;
 
-	if (!t->log_updates) {
-		/* nothing to append */
-		*log_file_seq_r = 0;
-		*log_file_offset_r = 0;
-		return 0;
-	}
-
-	if (log->index->log_locked) {
-		i_assert(t->external);
-	} else {
-		if (mail_transaction_log_lock_head(log) < 0)
-			return -1;
-
+	if (!index->log_locked) {
 		/* update sync_offset */
 		if (mail_transaction_log_file_map(log->head,
 						  log->head->sync_offset,
-						  (uoff_t)-1) < 0) {
-			mail_transaction_log_file_unlock(log->head);
+						  (uoff_t)-1) < 0)
 			return -1;
-		}
 	}
 
 	if (log->head->sync_offset > MAIL_TRANSACTION_LOG_ROTATE_SIZE &&
@@ -405,28 +392,21 @@
 	    ARE_ALL_TRANSACTIONS_IN_INDEX(log, index->hdr)) {
 		/* we might want to rotate, but check first that everything is
 		   synced in index. */
-		if (mail_index_lock_shared(log->index, TRUE, &lock_id) < 0) {
-			if (!log->index->log_locked)
-				mail_transaction_log_file_unlock(log->head);
+		if (mail_index_lock_shared(index, TRUE, &lock_id) < 0)
 			return -1;
-		}
 
 		/* we need the latest log_file_*_offsets. It's important to
 		   use this function instead of mail_index_map() as it may
 		   have generated them by reading log files. */
 		if (mail_index_get_latest_header(index, &idx_hdr) <= 0) {
 			mail_index_unlock(index, lock_id);
-			if (!log->index->log_locked)
-				mail_transaction_log_file_unlock(log->head);
 			return -1;
 		}
-		mail_index_unlock(log->index, lock_id);
+		mail_index_unlock(index, lock_id);
 
 		if (ARE_ALL_TRANSACTIONS_IN_INDEX(log, &idx_hdr)) {
-			if (mail_transaction_log_rotate(log, TRUE) < 0) {
-				/* that didn't work. well, try to continue
-				   anyway */
-			}
+			if (mail_transaction_log_rotate(log, TRUE) < 0)
+				return -1;
 		}
 	}
 
@@ -502,7 +482,7 @@
 			ret = pwrite_full(file->fd, &file->first_append_size,
 					  sizeof(uint32_t), append_offset);
 			if (ret < 0) {
-				mail_index_file_set_syscall_error(log->index,
+				mail_index_file_set_syscall_error(index,
 					file->filepath, "pwrite()");
 			}
 		} else {
@@ -527,9 +507,36 @@
 
 	*log_file_seq_r = file->hdr.file_seq;
 	*log_file_offset_r = file->sync_offset;
-
-	if (!log->index->log_locked)
-		mail_transaction_log_file_unlock(file);
 	return ret;
 }
 
+int mail_transaction_log_append(struct mail_index_transaction *t,
+				uint32_t *log_file_seq_r,
+				uoff_t *log_file_offset_r)
+{
+	struct mail_index *index;
+	int ret;
+
+	if (!t->log_updates) {
+		/* nothing to append */
+		*log_file_seq_r = 0;
+		*log_file_offset_r = 0;
+		return 0;
+	}
+
+	index = mail_index_view_get_index(t->view);
+
+	if (index->log_locked) {
+		i_assert(t->external);
+	} else {
+		if (mail_transaction_log_lock_head(index->log) < 0)
+			return -1;
+	}
+
+	ret = mail_transaction_log_append_locked(t, log_file_seq_r,
+						 log_file_offset_r);
+
+	if (!index->log_locked)
+		mail_transaction_log_file_unlock(index->log->head);
+	return ret;
+}

Index: mail-transaction-log.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-index/mail-transaction-log.c,v
retrieving revision 1.111.2.5
retrieving revision 1.111.2.6
diff -u -d -r1.111.2.5 -r1.111.2.6
--- mail-transaction-log.c	28 Dec 2006 16:27:42 -0000	1.111.2.5
+++ mail-transaction-log.c	16 Jan 2007 21:58:48 -0000	1.111.2.6
@@ -492,31 +492,37 @@
 }
 
 static int
-mail_transaction_log_file_create2(struct mail_transaction_log *log,
-				  const char *path, int new_fd,
+mail_transaction_log_file_create2(struct mail_transaction_log_file *file,
+				  bool lock, int new_fd,
 				  struct dotlock **dotlock,
 				  dev_t dev, ino_t ino, uoff_t file_size)
 {
-	struct mail_index *index = log->index;
+	struct mail_index *index = file->log->index;
 	struct mail_transaction_log_header hdr;
 	struct stat st;
 	const char *path2;
 	int old_fd, ret;
-	bool found;
+	bool rename_existing;
+
+	i_assert(!lock || file->log->head->locked);
 
 	/* log creation is locked now - see if someone already created it */
-	old_fd = nfs_safe_open(path, O_RDWR);
-	if (old_fd != -1) {
+	if (lock) {
+		/* don't even bother checking the existing file, but rename it
+		   if it exists */
+		rename_existing = TRUE;
+	} else if ((old_fd = nfs_safe_open(file->filepath, O_RDWR)) != -1) {
 		if ((ret = fstat(old_fd, &st)) < 0) {
-                        mail_index_file_set_syscall_error(index, path,
+                        mail_index_file_set_syscall_error(index, file->filepath,
                                                           "fstat()");
 		} else if (st.st_ino == ino && CMP_DEV_T(st.st_dev, dev) &&
 			   (uoff_t)st.st_size == file_size) {
 			/* same file, still broken */
 		} else {
                         /* file changed, use the new file */
-                        (void)file_dotlock_delete(dotlock);
-			return old_fd;
+			(void)file_dotlock_delete(dotlock);
+			file->fd = old_fd;
+			return 0;
 		}
 
 		(void)close(old_fd);
@@ -526,29 +532,35 @@
                         /* fstat() failure, return after closing fd.. */
                         return -1;
                 }
-		found = TRUE;
+		rename_existing = TRUE;
 	} else if (errno != ENOENT) {
-		mail_index_file_set_syscall_error(index, path, "open()");
+		mail_index_file_set_syscall_error(index, file->filepath,
+						  "open()");
 		return -1;
 	} else {
-		found = FALSE;
+		rename_existing = FALSE;
 	}
 
-	if (mail_transaction_log_init_hdr(log, &hdr) < 0)
+	if (mail_transaction_log_init_hdr(file->log, &hdr) < 0)
 		return -1;
 
 	if (write_full(new_fd, &hdr, sizeof(hdr)) < 0) {
-		mail_index_file_set_syscall_error(index, path,
+		mail_index_file_set_syscall_error(index, file->filepath,
 						  "write_full()");
 		return -1;
 	}
 
+	if (lock) {
+		if (mail_transaction_log_file_lock(file) < 0)
+			return -1;
+	}
+
 	/* keep two log files */
-	if (found) {
-		path2 = t_strconcat(path, ".2", NULL);
-		if (rename(path, path2) < 0) {
-                        mail_index_set_error(index,
-                        	"rename(%s, %s) failed: %m", path, path2);
+	if (rename_existing) {
+		path2 = t_strconcat(file->filepath, ".2", NULL);
+		if (rename(file->filepath, path2) < 0 && errno != ENOENT) {
+                        mail_index_set_error(index, "rename(%s, %s) failed: %m",
+					     file->filepath, path2);
 			/* ignore the error. we don't care that much about the
 			   second log file and we're going to overwrite this
 			   first one. */
@@ -560,63 +572,65 @@
 		return -1;
 
 	/* success */
-	return new_fd;
+	file->fd = new_fd;
+	return 0;
 }
 
 static int
-mail_transaction_log_file_create(struct mail_transaction_log *log,
-				 const char *path,
-				 dev_t dev, ino_t ino, uoff_t file_size)
+mail_transaction_log_file_create(struct mail_transaction_log_file *file,
+				 bool lock, dev_t dev, ino_t ino,
+				 uoff_t file_size)
 {
+	struct mail_index *index = file->log->index;
 	struct dotlock *dotlock;
 	struct stat st;
 	mode_t old_mask;
 	int fd;
 
-	i_assert(!MAIL_INDEX_IS_IN_MEMORY(log->index));
+	i_assert(!MAIL_INDEX_IS_IN_MEMORY(index));
 
-	if (stat(log->index->dir, &st) < 0) {
+	if (stat(index->dir, &st) < 0) {
 		if (ENOTFOUND(errno)) {
 			/* the whole index directory was deleted, which means
 			   the mailbox was deleted by another process.
 			   fail silently. */
-			mail_index_mark_corrupted(log->index);
+			mail_index_mark_corrupted(index);
 			return -1;
 		}
-		mail_index_file_set_syscall_error(log->index, log->index->dir,
-						  "stat()");
+		mail_index_file_set_syscall_error(index, index->dir, "stat()");
 		return -1;
 	}
 
 	/* With dotlocking we might already have path.lock created, so this
 	   filename has to be different. */
-	old_mask = umask(log->index->mode ^ 0666);
-	fd = file_dotlock_open(&log->new_dotlock_settings, path, 0, &dotlock);
+	old_mask = umask(index->mode ^ 0666);
+	fd = file_dotlock_open(&file->log->new_dotlock_settings,
+			       file->filepath, 0, &dotlock);
 	umask(old_mask);
 
 	if (fd == -1) {
-		mail_index_file_set_syscall_error(log->index, path,
+		mail_index_file_set_syscall_error(index, file->filepath,
 						  "file_dotlock_open()");
 		return -1;
 	}
 
-	if (log->index->gid != (gid_t)-1 &&
-	    fchown(fd, (uid_t)-1, log->index->gid) < 0) {
-		mail_index_file_set_syscall_error(log->index, path, "fchown()");
+	if (index->gid != (gid_t)-1 &&
+	    fchown(fd, (uid_t)-1, index->gid) < 0) {
+		mail_index_file_set_syscall_error(index, file->filepath,
+						  "fchown()");
 		(void)file_dotlock_delete(&dotlock);
 		return -1;
 	}
 
         /* either fd gets used or the dotlock gets deleted and returned fd
            is for the existing file */
-        fd = mail_transaction_log_file_create2(log, path, fd, &dotlock,
-                                               dev, ino, file_size);
-	if (fd < 0) {
+        if (mail_transaction_log_file_create2(file, lock, fd, &dotlock,
+					      dev, ino, file_size) < 0) {
 		if (dotlock != NULL)
 			(void)file_dotlock_delete(&dotlock);
 		return -1;
 	}
-	return fd;
+	return 0;
 }
 
 static void
@@ -661,104 +675,72 @@
 	*p = file;
 }
 
+static struct mail_transaction_log_file *
+mail_transaction_log_file_alloc(struct mail_transaction_log *log,
+				const char *path)
+{
+	struct mail_transaction_log_file *file;
+
+	file = i_new(struct mail_transaction_log_file, 1);
+	file->log = log;
+	file->filepath = i_strdup(path);
+	file->fd = -1;
+	return file;
+}
+
 static int
-mail_transaction_log_file_fd_open(struct mail_transaction_log *log,
-                                  struct mail_transaction_log_file **file_r,
-				  const char *path, int fd, bool head,
-				  bool ignore_estale)
+mail_transaction_log_file_fd_open(struct mail_transaction_log_file *file,
+				  bool head, bool ignore_estale)
 {
-        struct mail_transaction_log_file *file;
 	struct stat st;
-	int ret;
-
-	i_assert(!MAIL_INDEX_IS_IN_MEMORY(log->index));
 
-	*file_r = NULL;
+	i_assert(!MAIL_INDEX_IS_IN_MEMORY(file->log->index));
 
-	if (fstat(fd, &st) < 0) {
+	if (fstat(file->fd, &st) < 0) {
                 if (errno != ESTALE || !ignore_estale) {
-                        mail_index_file_set_syscall_error(log->index, path,
-                                                          "fstat()");
+			mail_index_file_set_syscall_error(file->log->index,
+							  file->filepath,
+							  "fstat()");
                 }
-		close_keep_errno(fd);
 		return -1;
 	}
 
-	file = i_new(struct mail_transaction_log_file, 1);
-	file->log = log;
-	file->filepath = i_strdup(path);
-	file->fd = fd;
 	file->st_dev = st.st_dev;
 	file->st_ino = st.st_ino;
 	file->last_mtime = st.st_mtime;
 	file->last_size = st.st_size;
 
-	ret = mail_transaction_log_file_read_hdr(file, head, ignore_estale);
-	if (ret < 0) {
-		mail_transaction_log_file_free(file);
-		return -1;
-	}
-
-	*file_r = file;
-	return ret;
+	return mail_transaction_log_file_read_hdr(file, head, ignore_estale);
 }
 
-
-static struct mail_transaction_log_file *
-mail_transaction_log_file_fd_open_or_create(struct mail_transaction_log *log,
-                                            const char *path, int fd,
-                                            bool *retry_r, bool try_retry)
+static int
+mail_transaction_log_file_fd_open_or_create(struct mail_transaction_log_file
+					    	*file, bool try_retry)
 {
-	struct mail_transaction_log_file *file;
-	struct stat st;
 	int ret;
 
-        *retry_r = FALSE;
-
-	ret = mail_transaction_log_file_fd_open(log, &file, path, fd, TRUE,
-						!try_retry);
-        if (ret < 0) {
-                *retry_r = errno == ESTALE && try_retry;
-                return NULL;
-        }
-
+	ret = mail_transaction_log_file_fd_open(file, TRUE, !try_retry);
 	if (ret == 0) {
-		/* corrupted header */
-		fd = mail_transaction_log_file_create(log, path, file->st_dev,
-						      file->st_ino,
-						      file->last_size);
-		if (fd == -1)
-			ret = -1;
-		else if (fstat(fd, &st) < 0) {
-                        mail_index_file_set_syscall_error(log->index, path,
-							  "fstat()");
-			(void)close(fd);
-			fd = -1;
+		/* corrupted header, recreate the file */
+		if (mail_transaction_log_file_create(file, FALSE,
+						     file->st_dev,
+						     file->st_ino,
+						     file->last_size) < 0)
 			ret = -1;
+		else {
+			ret = mail_transaction_log_file_fd_open(file, TRUE,
+								FALSE);
+			if (ret == 0) {
+				/* newly created transaction log corrupted */
+				return -1;
+			}
 		}
-
-		if (fd != -1) {
-			(void)close(file->fd);
-			file->fd = fd;
-
-			file->st_dev = st.st_dev;
-			file->st_ino = st.st_ino;
-			file->last_mtime = st.st_mtime;
-			file->last_size = st.st_size;
-
-			memset(&file->hdr, 0, sizeof(file->hdr));
-			ret = mail_transaction_log_file_read_hdr(file, TRUE,
-								 !try_retry);
-		}
-	}
-	if (ret <= 0) {
-                *retry_r = errno == ESTALE && try_retry;
-		mail_transaction_log_file_free(file);
-		return NULL;
 	}
+	if (ret < 0)
+		return errno == ENOENT && try_retry ? 0 : -1;
 
         mail_transaction_log_file_add_to_head(file);
-	return file;
+	return 1;
 }
 
 static struct mail_transaction_log_file *
@@ -789,36 +771,41 @@
 {
         struct mail_transaction_log_file *file;
         unsigned int i;
-        bool retry;
-        int fd;
+	int ret;
 
 	if (MAIL_INDEX_IS_IN_MEMORY(log->index))
 		return mail_transaction_log_file_alloc_in_memory(log);
 
+	file = mail_transaction_log_file_alloc(log, path);
+
         for (i = 0; ; i++) {
-                fd = nfs_safe_open(path, O_RDWR);
-                if (fd == -1) {
+                file->fd = nfs_safe_open(path, O_RDWR);
+                if (file->fd == -1) {
                         if (errno != ENOENT) {
-                                mail_index_file_set_syscall_error(log->index,
+				mail_index_file_set_syscall_error(log->index,
                                                                   path,
                                                                   "open()");
-                                return NULL;
+				break;
                         }
 
                         /* doesn't exist, try creating it */
-                        fd = mail_transaction_log_file_create(log, path,
-                                                              0, 0, 0);
-                        if (fd == -1)
-                                return NULL;
+			if (mail_transaction_log_file_create(file, FALSE,
+							     0, 0, 0) < 0)
+				break;
                 }
 
-                file = mail_transaction_log_file_fd_open_or_create(log, path,
-                		fd, &retry, i == MAIL_INDEX_ESTALE_RETRY_COUNT);
-                if (file != NULL || !retry)
-                        return file;
+		ret = mail_transaction_log_file_fd_open_or_create(file,
+                		i == MAIL_INDEX_ESTALE_RETRY_COUNT);
+		if (ret > 0)
+			return file;
+		if (ret < 0)
+			break;
 
                 /* ESTALE - retry */
-        }
+	}
+
+	mail_transaction_log_file_free(file);
+	return NULL;
 }
 
 static struct mail_transaction_log_file *
@@ -827,36 +814,40 @@
 {
 	struct mail_transaction_log_file *file;
         unsigned int i;
-        int fd, ret;
+        int ret;
 
+	file = mail_transaction_log_file_alloc(log, path);
         for (i = 0;; i++) {
-                fd = nfs_safe_open(path, O_RDWR);
-                if (fd == -1) {
+                file->fd = nfs_safe_open(path, O_RDWR);
+                if (file->fd == -1) {
                         mail_index_file_set_syscall_error(log->index, path,
                                                           "open()");
-                        return NULL;
+			break;
                 }
 
-                ret = mail_transaction_log_file_fd_open(log, &file, path,
-                		fd, TRUE, i < MAIL_INDEX_ESTALE_RETRY_COUNT);
-                if (ret > 0)
-                        break;
+		ret = mail_transaction_log_file_fd_open(file,
+                		TRUE, i < MAIL_INDEX_ESTALE_RETRY_COUNT);
+		if (ret > 0) {
+			/* success */
+			mail_transaction_log_file_add_to_head(file);
+			return file;
+		}
 
-                /* error / corrupted */
-                if (ret == 0)
-                        mail_transaction_log_file_free(file);
-                else {
-                        if (errno == ESTALE &&
-                            i < MAIL_INDEX_ESTALE_RETRY_COUNT) {
-                                /* try again */
-                                continue;
-                        }
-                }
-                return NULL;
+		if (ret == 0) {
+			/* corrupted */
+			break;
+		}
+		if (errno != ESTALE ||
+		    i == MAIL_INDEX_ESTALE_RETRY_COUNT) {
+			/* syscall error */
+			break;
+		}
+
+		/* ESTALE - try again */
         }
 
-	mail_transaction_log_file_add_to_head(file);
-	return file;
+	mail_transaction_log_file_free(file);
+	return NULL;
 }
 
 void mail_transaction_logs_clean(struct mail_transaction_log *log)
@@ -877,8 +868,7 @@
 	struct mail_transaction_log_file *file;
 	const char *path = log->head->filepath;
         struct stat st;
-        bool retry;
-	int fd;
+	int ret;
 
 	i_assert(log->head->locked);
 
@@ -893,26 +883,22 @@
 			return -1;
 		}
 
-		fd = mail_transaction_log_file_create(log, path,
-						      st.st_dev, st.st_ino,
-						      st.st_size);
-		if (fd == -1)
+		file = mail_transaction_log_file_alloc(log, path);
+		if (mail_transaction_log_file_create(file, lock, st.st_dev,
+						     st.st_ino,
+						     st.st_size) < 0) {
+			mail_transaction_log_file_free(file);
 			return -1;
+		}
 
-                file = mail_transaction_log_file_fd_open_or_create(log, path,
-                					fd, &retry, FALSE);
-                if (file == NULL) {
-			i_assert(!retry);
-                        return -1;
-                }
-	}
-
-	if (lock) {
-		if (mail_transaction_log_file_lock(file) < 0) {
-			mail_transaction_logs_clean(log);
+                ret = mail_transaction_log_file_fd_open_or_create(file, FALSE);
+		if (ret <= 0) {
+			i_assert(ret != 0);
+			mail_transaction_log_file_free(file);
 			return -1;
 		}
 	}
+
 	i_assert(file->locked == lock);
 
 	if (--log->head->refcount == 0)
@@ -1028,8 +1014,11 @@
 		}
 	}
 
-	ret = mail_transaction_log_file_fd_open(log, &file, path, fd,
-						FALSE, TRUE);
+
+	file = mail_transaction_log_file_alloc(log, path);
+	file->fd = fd;
+
+	ret = mail_transaction_log_file_fd_open(file, FALSE, TRUE);
 	if (ret <= 0) {
 		bool stale = errno == ESTALE;
 



More information about the dovecot-cvs mailing list