lib-imap: imap_bodystructure_write() - Return error on corruption instead of assert...

This could happen if broken message_parts came from cache and
message_part->data was newly read from the mail input.

diff --git a/src/lib-imap-storage/imap-msgpart.c b/src/lib-imap-storage/imap-msgpart.c
index 024fa6edd575041309320c6c3d8bfff7a4ecbe9a..3bce11754b81adc850850538568be9c194934954 100644 (file)
--- a/src/lib-imap-storage/imap-msgpart.c
+++ b/src/lib-imap-storage/imap-msgpart.c
@@ -812,6 +812,7 @@ int imap_msgpart_bodypartstructure(struct mail *mail,
 {
        struct message_part *all_parts, *part;
        string_t *bpstruct;
+       const char *error;
        int ret;
 
        /* if we start parsing the body in here, make sure we also parse the
@@ -840,7 +841,13 @@ int imap_msgpart_bodypartstructure(struct mail *mail,
 
        if (ret >= 0) {
                bpstruct = t_str_new(256);
-               imap_bodystructure_write(part, bpstruct, TRUE);
+               if (imap_bodystructure_write(part, bpstruct, TRUE, &error) < 0) {
+                       error = t_strdup_printf(
+                               "Invalid message_part/BODYSTRUCTURE: %s", error);
+                       mail_set_cache_corrupted(mail, MAIL_FETCH_MESSAGE_PARTS,
+                                                error);
+                       return -1;
+               }
                *bpstruct_r = str_c(bpstruct);
        }
        return ret < 0 ? -1 : 1;

diff --git a/src/lib-imap/fuzz-imap-bodystructure.c b/src/lib-imap/fuzz-imap-bodystructure.c
index 65a3be54eef602db619aeb5c3bd3bdfb3f5e5bec..a9d449992414906dd9d661088d5b02aec5e25f13 100644 (file)
--- a/src/lib-imap/fuzz-imap-bodystructure.c
+++ b/src/lib-imap/fuzz-imap-bodystructure.c
@@ -28,7 +28,8 @@ FUZZ_BEGIN_STR(const char *str)
        i_zero(&parts);
 
        if (imap_bodystructure_parse(str, pool, &parts, &error) == 0) {
-               imap_bodystructure_write(&parts, dest, TRUE);
+               if (imap_bodystructure_write(&parts, dest, TRUE, &error) < 0)
+                       i_panic("Failed to write bodystructure: %s", error);
                /* The written bodystructure must be parseable *and*
                   it must come out exactly the same again */
                if (imap_bodystructure_parse(str_c(dest), pool, &parts, &error) != 0) {
@@ -37,7 +38,8 @@ FUZZ_BEGIN_STR(const char *str)
                } else {
                        const char *new_str = t_strdup(str_c(dest));
                        str_truncate(dest, 0);
-                       imap_bodystructure_write(&parts, dest, TRUE);
+                       if (imap_bodystructure_write(&parts, dest, TRUE, &error) < 0)
+                               i_panic("Failed to write reparsed bodystructure: %s", error);
                        if (strcmp(str_c(dest), new_str) != 0) {
                                i_panic("Parsed bodystructure '%s' does not match '%s'",
                                        str_sanitize_binary(new_str),

diff --git a/src/lib-imap/imap-bodystructure.c b/src/lib-imap/imap-bodystructure.c
index 28af528822a1588f8ed4e82d3135a99a6250b872..3236c972c9dc696d81c5a7f5245962186596b088 100644 (file)
--- a/src/lib-imap/imap-bodystructure.c
+++ b/src/lib-imap/imap-bodystructure.c
@@ -60,15 +60,18 @@ params_write(const struct message_part_param *params,
        str_append_c(str, ')');
 }
 
-static void
+static int
 part_write_bodystructure_siblings(const struct message_part *part,
-                                 string_t *dest, bool extended)
+                                 string_t *dest, bool extended,
+                                 const char **error_r)
 {
        for (; part != NULL; part = part->next) {
                str_append_c(dest, '(');
-               imap_bodystructure_write(part, dest, extended);
+               if (imap_bodystructure_write(part, dest, extended, error_r) < 0)
+                       return -1;
                str_append_c(dest, ')');
        }
+       return 0;
 }
 
 static void
@@ -111,16 +114,19 @@ part_write_bodystructure_common(const struct message_part_data *data,
        imap_append_nstring_nolf(str, data->content_location);
 }
 
-static void part_write_body_multipart(const struct message_part *part,
-                                     string_t *str, bool extended)
+static int part_write_body_multipart(const struct message_part *part,
+                                    string_t *str, bool extended,
+                                    const char **error_r)
 {
        const struct message_part_data *data = part->data;
 
        i_assert(part->data != NULL);
 
-       if (part->children != NULL)
-               part_write_bodystructure_siblings(part->children, str, extended);
-       else {
+       if (part->children != NULL) {
+               if (part_write_bodystructure_siblings(part->children, str,
+                                                     extended, error_r) < 0)
+                       return -1;
+       } else {
                /* no parts in multipart message,
                   that's not allowed. write a single
                   0-length text/plain structure */
@@ -134,7 +140,7 @@ static void part_write_body_multipart(const struct message_part *part,
        imap_append_string(str, data->content_subtype);
 
        if (!extended)
-               return;
+               return 0;
 
        /* BODYSTRUCTURE data */
 
@@ -143,6 +149,7 @@ static void part_write_body_multipart(const struct message_part *part,
                data->content_type_params_count, str, FALSE);
 
        part_write_bodystructure_common(data, str);
+       return 0;
 }
 
 static bool part_is_truncated(const struct message_part *part)
@@ -176,8 +183,8 @@ static bool part_is_truncated(const struct message_part *part)
        return FALSE;
 }
 
-static void part_write_body(const struct message_part *part,
-                           string_t *str, bool extended)
+static int part_write_body(const struct message_part *part,
+                          string_t *str, bool extended, const char **error_r)
 {
        const struct message_part_data *data = part->data;
        bool text;
@@ -206,7 +213,11 @@ static void part_write_body(const struct message_part *part,
                        str_append_c(str, ' ');
                        imap_append_string(str, data->content_subtype);
                }
-               i_assert(text == ((part->flags & MESSAGE_PART_FLAG_TEXT) != 0));
+               bool part_is_text = (part->flags & MESSAGE_PART_FLAG_TEXT) != 0;
+               if (text != part_is_text) {
+                       *error_r = "text flag mismatch";
+                       return -1;
+               }
        }
 
        /* ("content type param key" "value" ...) */
@@ -241,12 +252,14 @@ static void part_write_body(const struct message_part *part,
                imap_envelope_write(child_data->envelope, str);
                str_append(str, ") ");
 
-               part_write_bodystructure_siblings(part->children, str, extended);
+               if (part_write_bodystructure_siblings(part->children, str,
+                                                     extended, error_r) < 0)
+                       return -1;
                str_printfa(str, " %u", part->body_size.lines);
        }
 
        if (!extended)
-               return;
+               return 0;
 
        /* BODYSTRUCTURE data */
 
@@ -255,15 +268,17 @@ static void part_write_body(const struct message_part *part,
        str_append_c(str, ' ');
        imap_append_nstring_nolf(str, data->content_md5);
        part_write_bodystructure_common(data, str);
+       return 0;
 }
 
-void imap_bodystructure_write(const struct message_part *part,
-                             string_t *dest, bool extended)
+int imap_bodystructure_write(const struct message_part *part,
+                            string_t *dest, bool extended,
+                            const char **error_r)
 {
        if ((part->flags & MESSAGE_PART_FLAG_MULTIPART) != 0)
-               part_write_body_multipart(part, dest, extended);
+               return part_write_body_multipart(part, dest, extended, error_r);
        else
-               part_write_body(part, dest, extended);
+               return part_write_body(part, dest, extended, error_r);
 }
 
 /*

diff --git a/src/lib-imap/imap-bodystructure.h b/src/lib-imap/imap-bodystructure.h
index 7b1390b40ac025a1845524b2fa1ec98f7177f6de..a7cc6cda7baafe2d5079d2d7e72ee11fc39e3511 100644 (file)
--- a/src/lib-imap/imap-bodystructure.h
+++ b/src/lib-imap/imap-bodystructure.h
@@ -7,9 +7,11 @@ struct imap_arg;
 
 /* Write a BODY/BODYSTRUCTURE from given message_part. The message_part->data
    field must be set. part->body_size.virtual_size and .lines are also used
-   for writing it. */
-void imap_bodystructure_write(const struct message_part *part,
-                             string_t *dest, bool extended);
+   for writing it. Returns 0 on success, -1 if parts don't internally match
+   (e.g. broken cached mime.parts mixed with parsed message). */
+int imap_bodystructure_write(const struct message_part *part,
+                            string_t *dest, bool extended,
+                            const char **error_r);
 
 /* Parse BODYSTRUCTURE and save the contents to message_part->data for each
    message tree node. If the parts argument points to NULL, the message_part

diff --git a/src/lib-imap/test-imap-bodystructure.c b/src/lib-imap/test-imap-bodystructure.c
index ead1be24567398c98de516978e75e5ce4432bb2e..1b4a27a747411c26b4907f07bf98d0d6782e71dc 100644 (file)
--- a/src/lib-imap/test-imap-bodystructure.c
+++ b/src/lib-imap/test-imap-bodystructure.c
@@ -414,6 +414,7 @@ msg_parse(pool_t pool, const char *message, unsigned int max_nested_mime_parts,
 static void test_imap_bodystructure_write(void)
 {
        struct message_part *parts;
+       const char *error;
        unsigned int i;
 
        for (i = 0; i < parse_tests_count; i++) T_BEGIN {
@@ -424,16 +425,31 @@ static void test_imap_bodystructure_write(void)
                test_begin(t_strdup_printf("imap bodystructure write [%u]", i));
                parts = msg_parse(pool, test->message, 0, 0, TRUE);
 
-               imap_bodystructure_write(parts, str, TRUE);
+               test_assert(imap_bodystructure_write(parts, str, TRUE, &error) == 0);
                test_assert(strcmp(str_c(str), test->bodystructure) == 0);
 
                str_truncate(str, 0);
-               imap_bodystructure_write(parts, str, FALSE);
+               test_assert(imap_bodystructure_write(parts, str, FALSE, &error) == 0);
                test_assert(strcmp(str_c(str), test->body) == 0);
 
                pool_unref(&pool);
                test_end();
        } T_END;
+
+       T_BEGIN {
+               test_begin("imap bodystructure write - corrupted");
+               pool_t pool = pool_alloconly_create("imap bodystructure write", 1024);
+
+               parts = msg_parse(pool, "Subject: hello world", 0, 0, TRUE);
+               i_assert((parts->flags & MESSAGE_PART_FLAG_TEXT) != 0);
+               parts->flags &= ENUM_NEGATE(MESSAGE_PART_FLAG_TEXT);
+
+               string_t *str = t_str_new(128);
+               test_assert(imap_bodystructure_write(parts, str, FALSE, &error) < 0);
+               test_assert_strcmp(error, "text flag mismatch");
+               pool_unref(&pool);
+               test_end();
+       } T_END;
 }
 
 static void test_imap_bodystructure_parse(void)
@@ -461,7 +477,7 @@ static void test_imap_bodystructure_parse(void)
 
                if (ret == 0) {
                        str_truncate(str, 0);
-                       imap_bodystructure_write(parts, str, TRUE);
+                       test_assert(imap_bodystructure_write(parts, str, TRUE, &error) == 0);
                        test_assert(strcmp(str_c(str), test->bodystructure) == 0);
                } else {
                        i_error("Invalid BODYSTRUCTURE: %s", error);
@@ -550,7 +566,7 @@ static void test_imap_bodystructure_parse_full(void)
 
                if (ret == 0) {
                        str_truncate(str, 0);
-                       imap_bodystructure_write(parts, str, TRUE);
+                       test_assert(imap_bodystructure_write(parts, str, TRUE, &error) == 0);
                        test_assert(strcmp(str_c(str), test->bodystructure) == 0);
                } else {
                        i_error("Invalid BODYSTRUCTURE: %s", error);
@@ -582,7 +598,7 @@ static void test_imap_bodystructure_normalize(void)
 
                if (ret == 0) {
                        str_truncate(str, 0);
-                       imap_bodystructure_write(parts, str, TRUE);
+                       test_assert(imap_bodystructure_write(parts, str, TRUE, &error) == 0);
                        test_assert(strcmp(str_c(str), test->output) == 0);
                } else {
                        i_error("Invalid BODYSTRUCTURE: %s", error);
@@ -671,7 +687,7 @@ static void test_imap_bodystructure_truncation(void)
                                  TRUE);
 
                /* write out BODYSTRUCTURE and serialize message_parts */
-               imap_bodystructure_write(parts, str_body, TRUE);
+               test_assert(imap_bodystructure_write(parts, str_body, TRUE, &error) == 0);
                message_part_serialize(parts, str_parts);
 
                /* now deserialize message_parts and make sure they can be used

diff --git a/src/lib-storage/index/imapc/imapc-mail-fetch.c b/src/lib-storage/index/imapc/imapc-mail-fetch.c
index 663748bde7be589365e4e28eaec13776caa89a23..4275c657f00ae8f385c9112d1bc15312f2430bea 100644 (file)
--- a/src/lib-storage/index/imapc/imapc-mail-fetch.c
+++ b/src/lib-storage/index/imapc/imapc-mail-fetch.c
@@ -8,6 +8,7 @@
 #include "istream-header-filter.h"
 #include "message-header-parser.h"
 #include "imap-arg.h"
+#include "imap-util.h"
 #include "imap-date.h"
 #include "imap-quote.h"
 #include "imap-bodystructure.h"
@@ -806,7 +807,16 @@ imapc_args_to_bodystructure(struct imapc_mail *mail,
                ret = NULL;
        } else {
                string_t *str = t_str_new(128);
-               imap_bodystructure_write(parts, str, extended);
+               if (imap_bodystructure_write(parts, str, extended, &error) < 0) {
+                       /* All the input to imap_bodystructure_write() came
+                          from imap_bodystructure_parse_args(). We should never
+                          get here. Instead, if something is wrong the
+                          parsing should have returned an error already. */
+                       str_truncate(str, 0);
+                       imap_write_args(str, args);
+                       i_panic("Failed to write parsed BODYSTRUCTURE: %s "
+                               "(original string: '%s')", error, str_c(str));
+               }
                ret = p_strdup(mail->imail.mail.data_pool, str_c(str));
        }
        pool_unref(&pool);

diff --git a/src/lib-storage/index/index-mail.c b/src/lib-storage/index/index-mail.c
index 403beba34d6cb08b847124b3cd9fd51c9762791b..87f17596df8799c77993f7816e5dacd0a4392dcf 100644 (file)
--- a/src/lib-storage/index/index-mail.c
+++ b/src/lib-storage/index/index-mail.c
@@ -808,6 +808,21 @@ static void index_mail_body_parsed_cache_message_parts(struct index_mail *mail)
        data->messageparts_saved_to_cache = TRUE;
 }
 
+static int
+index_mail_write_bodystructure(struct index_mail *mail, string_t *str,
+                              bool extended)
+{
+       const char *error;
+
+       if (imap_bodystructure_write(mail->data.parts, str, extended,
+                                    &error) < 0) {
+               mail_set_cache_corrupted(&mail->mail.mail,
+                       MAIL_FETCH_MESSAGE_PARTS, error);
+               return -1;
+       }
+       return 0;
+}
+
 static void
 index_mail_body_parsed_cache_bodystructure(struct index_mail *mail,
                                           enum index_cache_field field)
@@ -856,12 +871,12 @@ index_mail_body_parsed_cache_bodystructure(struct index_mail *mail,
        }
        if (cache_bodystructure) {
                str = str_new(mail->mail.data_pool, 128);
-               imap_bodystructure_write(data->parts, str, TRUE);
-               data->bodystructure = str_c(str);
-
-               index_mail_cache_add(mail, MAIL_CACHE_IMAP_BODYSTRUCTURE,
-                                    str_c(str), str_len(str));
-               bodystructure_cached = TRUE;
+               if (index_mail_write_bodystructure(mail, str, TRUE) == 0) {
+                       data->bodystructure = str_c(str);
+                       index_mail_cache_add(mail, MAIL_CACHE_IMAP_BODYSTRUCTURE,
+                                            str_c(str), str_len(str));
+                       bodystructure_cached = TRUE;
+               }
        } else {
                bodystructure_cached =
                        mail_cache_field_exists(_mail->transaction->cache_view,
@@ -888,11 +903,11 @@ index_mail_body_parsed_cache_bodystructure(struct index_mail *mail,
 
        if (cache_body) {
                str = str_new(mail->mail.data_pool, 128);
-               imap_bodystructure_write(data->parts, str, FALSE);
-               data->body = str_c(str);
-
-               index_mail_cache_add(mail, MAIL_CACHE_IMAP_BODY,
-                                    str_c(str), str_len(str));
+               if (index_mail_write_bodystructure(mail, str, FALSE) == 0) {
+                       data->body = str_c(str);
+                       index_mail_cache_add(mail, MAIL_CACHE_IMAP_BODY,
+                                            str_c(str), str_len(str));
+               }
        }
 }
 
@@ -1460,6 +1475,16 @@ static int index_mail_parse_bodystructure(struct index_mail *mail,
        } else {
                if (index_mail_parse_bodystructure_full(mail, field) < 0)
                        return -1;
+               if (data->parts == NULL) {
+                       /* Corrupted mime.parts detected. Retry by parsing
+                          the mail. */
+                       data->parsed_bodystructure = FALSE;
+                       data->parsed_bodystructure_header = FALSE;
+                       data->save_bodystructure_header = TRUE;
+                       data->save_bodystructure_body = TRUE;
+                       if (index_mail_parse_bodystructure_full(mail, field) < 0)
+                               return -1;
+               }
        }
        i_assert(data->parts != NULL);
 
@@ -1469,14 +1494,16 @@ static int index_mail_parse_bodystructure(struct index_mail *mail,
        case MAIL_CACHE_IMAP_BODY:
                if (data->body == NULL) {
                        str = str_new(mail->mail.data_pool, 128);
-                       imap_bodystructure_write(data->parts, str, FALSE);
+                       if (index_mail_write_bodystructure(mail, str, FALSE) < 0)
+                               return -1;
                        data->body = str_c(str);
                }
                break;
        case MAIL_CACHE_IMAP_BODYSTRUCTURE:
                if (data->bodystructure == NULL) {
                        str = str_new(mail->mail.data_pool, 128);
-                       imap_bodystructure_write(data->parts, str, TRUE);
+                       if (index_mail_write_bodystructure(mail, str, TRUE) < 0)
+                               return -1;
                        data->bodystructure = str_c(str);
                }
                break;