From: Timo Sirainen <timo.sirainen@dovecot.fi>
Date: Mon, 21 Aug 2017 12:11:30 +0000 (+0300)
Subject: lib-storage: Fix error logging after mail_storage_set_internal_error()
X-Git-Tag: 2.3.0.rc1~1139
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fe7a56906e8bdf6e42cc981b4a62e31bac4a30f4;p=thirdparty%2Fdovecot%2Fcore.git

lib-storage: Fix error logging after mail_storage_set_internal_error()

The function doesn't actually set the last_internal_error and it also
doesn't update last_error_is_internal, so calling
mail_storage_get_last_internal_error() afterwards could be returning an
error for some different older error.

Since there's no parameter to set the internal error string, just reset
last_error_is_internal=FALSE.
---

diff --git a/src/lib-storage/mail-storage.c b/src/lib-storage/mail-storage.c
index 1b16c5c872..8f8f1a4957 100644
--- a/src/lib-storage/mail-storage.c
+++ b/src/lib-storage/mail-storage.c
@@ -533,6 +533,10 @@ void mail_storage_set_internal_error(struct mail_storage *storage)
 	i_free(storage->error_string);
 	storage->error_string = i_strdup(str);
 	storage->error = MAIL_ERROR_TEMP;
+
+	/* this function doesn't set last_internal_error, so
+	   last_error_is_internal can't be TRUE. */
+	storage->last_error_is_internal = FALSE;
 }
 
 void mail_storage_set_critical(struct mail_storage *storage,
@@ -541,6 +545,12 @@ void mail_storage_set_critical(struct mail_storage *storage,
 	char *old_error = storage->last_internal_error;
 	va_list va;
 
+	storage->last_internal_error = NULL;
+	/* critical errors may contain sensitive data, so let user
+	   see only "Internal error" with a timestamp to make it
+	   easier to look from log files the actual error message. */
+	mail_storage_set_internal_error(storage);
+
 	va_start(va, fmt);
 	storage->last_internal_error = i_strdup_vprintf(fmt, va);
 	va_end(va);
@@ -550,11 +560,6 @@ void mail_storage_set_critical(struct mail_storage *storage,
 	/* free the old_error only after the new error is generated, because
 	   the old_error may be one of the parameters. */
 	i_free(old_error);
-
-	/* critical errors may contain sensitive data, so let user
-	   see only "Internal error" with a timestamp to make it
-	   easier to look from log files the actual error message. */
-	mail_storage_set_internal_error(storage);
 }
 
 const char *mail_storage_get_last_internal_error(struct mail_storage *storage,
diff --git a/src/lib-storage/test-mail-storage.c b/src/lib-storage/test-mail-storage.c
index 9bea9632b1..d9e3ef251b 100644
--- a/src/lib-storage/test-mail-storage.c
+++ b/src/lib-storage/test-mail-storage.c
@@ -38,7 +38,18 @@ static void test_mail_storage_errors(void)
 	test_assert(mail_error == MAIL_ERROR_TEMP);
 	test_assert(!storage.last_error_is_internal);
 
-	/* internal error without specifying what it is */
+	/* set internal error in preparation for the next test */
+	test_expect_error_string("critical0");
+	mail_storage_set_critical(&storage, "critical0");
+	test_expect_no_more_errors();
+	test_assert(strstr(mail_storage_get_last_error(&storage, &mail_error), MAIL_ERRSTR_CRITICAL_MSG) != NULL);
+	test_assert(mail_error == MAIL_ERROR_TEMP);
+	test_assert(strcmp(mail_storage_get_last_internal_error(&storage, &mail_error), "critical0") == 0);
+	test_assert(mail_error == MAIL_ERROR_TEMP);
+	test_assert(storage.last_error_is_internal);
+
+	/* internal error without specifying what it is. this needs to clear
+	   the previous internal error. */
 	mail_storage_set_internal_error(&storage);
 	test_assert(strstr(mail_storage_get_last_error(&storage, &mail_error), MAIL_ERRSTR_CRITICAL_MSG) != NULL);
 	test_assert(mail_error == MAIL_ERROR_TEMP);