From 6cf18519f0804755385871d84384834465b2d68e Mon Sep 17 00:00:00 2001 From: Timo Sirainen Date: Fri, 17 Feb 2017 18:19:46 +0200 Subject: [PATCH] lib-storage: Fix error handling when sorting mails. All errors were treated the same as if message had been expunged. This caused potentially wrong results to be sent to the client without any indication that they're wrong. --- src/lib-storage/index/index-search.c | 6 +- src/lib-storage/index/index-sort-private.h | 5 +- src/lib-storage/index/index-sort-string.c | 107 +++++++++------ src/lib-storage/index/index-sort.c | 147 ++++++++++++++------- src/lib-storage/index/index-sort.h | 2 +- 5 files changed, 172 insertions(+), 95 deletions(-) diff --git a/src/lib-storage/index/index-search.c b/src/lib-storage/index/index-search.c index 49b02fe920..331a04cebf 100644 --- a/src/lib-storage/index/index-search.c +++ b/src/lib-storage/index/index-search.c @@ -1285,8 +1285,10 @@ int index_storage_search_deinit(struct mail_search_context *_ctx) if (ctx->mail_ctx.wanted_headers != NULL) mailbox_header_lookup_unref(&ctx->mail_ctx.wanted_headers); - if (ctx->mail_ctx.sort_program != NULL) - index_sort_program_deinit(&ctx->mail_ctx.sort_program); + if (ctx->mail_ctx.sort_program != NULL) { + if (index_sort_program_deinit(&ctx->mail_ctx.sort_program) < 0) + ret = -1; + } if (ctx->thread_ctx != NULL) mail_thread_deinit(&ctx->thread_ctx); array_free(&ctx->mail_ctx.results); diff --git a/src/lib-storage/index/index-sort-private.h b/src/lib-storage/index/index-sort-private.h index 327332176d..54907f0fd5 100644 --- a/src/lib-storage/index/index-sort-private.h +++ b/src/lib-storage/index/index-sort-private.h @@ -15,11 +15,14 @@ struct mail_search_sort_program { ARRAY_TYPE(uint32_t) seqs; unsigned int iter_idx; + + bool failed; }; +/* Returns 1 on success, 0 if mail is already expunged, -1 on other errors. */ int index_sort_header_get(struct mail *mail, uint32_t seq, enum mail_sort_type sort_type, string_t *dest); -int index_sort_node_cmp_type(struct mail *mail, +int index_sort_node_cmp_type(struct mail_search_sort_program *program, const enum mail_sort_type *sort_program, uint32_t seq1, uint32_t seq2); diff --git a/src/lib-storage/index/index-sort-string.c b/src/lib-storage/index/index-sort-string.c index 6effdaf298..814828fcde 100644 --- a/src/lib-storage/index/index-sort-string.c +++ b/src/lib-storage/index/index-sort-string.c @@ -42,9 +42,9 @@ struct sort_string_context { bool reverse:1; bool seqs_nonsorted:1; bool broken:1; + bool failed:1; }; -static char expunged_msg; static struct sort_string_context *static_zero_cmp_context; static void index_sort_list_reset_broken(struct sort_string_context *ctx); @@ -247,7 +247,7 @@ static int sort_node_zero_string_cmp(const struct mail_sort_node *n1, if (ret != 0) return !ctx->reverse ? ret : -ret; - return index_sort_node_cmp_type(ctx->program->temp_mail, + return index_sort_node_cmp_type(ctx->program, ctx->program->sort_program + 1, n1->seq, n2->seq); } @@ -273,8 +273,11 @@ static void index_sort_zeroes(struct sort_string_context *ctx) i_assert(nodes[i].seq <= ctx->last_seq); T_BEGIN { - (void)index_sort_header_get(mail, nodes[i].seq, - sort_type, str); + if (index_sort_header_get(mail, nodes[i].seq, + sort_type, str) < 0) { + nodes[i].no_update = TRUE; + ctx->failed = TRUE; + } ctx->sort_strings[nodes[i].seq] = str_len(str) == 0 ? "" : p_strdup(pool, str_c(str)); @@ -287,9 +290,9 @@ static void index_sort_zeroes(struct sort_string_context *ctx) array_sort(&ctx->zero_nodes, sort_node_zero_string_cmp); } -static const char * +static bool index_sort_get_expunged_string(struct sort_string_context *ctx, uint32_t idx, - string_t *str) + string_t *str, const char **result_r) { struct mail *mail = ctx->program->temp_mail; enum mail_sort_type sort_type = ctx->program->sort_program[0]; @@ -307,8 +310,10 @@ index_sort_get_expunged_string(struct sort_string_context *ctx, uint32_t idx, trust it. If it's expunged, we already verified that there are no non-expunged messages. */ if (idx > 0 && nodes[idx-1].sort_id == sort_id && - ctx->sort_strings[nodes[idx].seq] != NULL) - return ctx->sort_strings[nodes[idx].seq]; + ctx->sort_strings[nodes[idx].seq] != NULL) { + *result_r = ctx->sort_strings[nodes[idx].seq]; + return TRUE; + } /* Go forwards as long as there are identical sort IDs. If we find one that's not expunged, update string table for all messages with @@ -325,7 +330,7 @@ index_sort_get_expunged_string(struct sort_string_context *ctx, uint32_t idx, break; } if (index_sort_header_get(mail, nodes[i].seq, - sort_type, str) >= 0) { + sort_type, str) > 0) { result = str_len(str) == 0 ? "" : p_strdup(ctx->sort_string_pool, str_c(str)); break; @@ -333,41 +338,49 @@ index_sort_get_expunged_string(struct sort_string_context *ctx, uint32_t idx, } if (result == NULL) { /* unknown */ - return &expunged_msg; + return FALSE; } /* fill all identical sort_ids with the same value */ for (i = idx; i > 0 && nodes[i-1].sort_id == sort_id; i--) ; for (i = idx; i < count && nodes[i].sort_id == sort_id; i++) ctx->sort_strings[nodes[i].seq] = result; - return result; + *result_r = result; + return TRUE; } -static const char * +static int index_sort_get_string(struct sort_string_context *ctx, - uint32_t idx, uint32_t seq) + uint32_t idx, uint32_t seq, const char **str_r) { struct mail *mail = ctx->program->temp_mail; - int ret; + int ret = 1; if (ctx->sort_strings[seq] == NULL) T_BEGIN { string_t *str; + const char *result; str = t_str_new(256); ret = index_sort_header_get(mail, seq, ctx->program->sort_program[0], str); - if (str_len(str) > 0) { - ctx->sort_strings[seq] = - p_strdup(ctx->sort_string_pool, str_c(str)); - } else if (ret >= 0) { - ctx->sort_strings[seq] = ""; + if (ret < 0) + ctx->failed = TRUE; + else if (ret == 0) { + if (!index_sort_get_expunged_string(ctx, idx, str, &result)) + ctx->sort_strings[seq] = ""; + else { + /* found the expunged string - return success */ + ctx->sort_strings[seq] = result; + ret = 1; + } } else { - ctx->sort_strings[seq] = - index_sort_get_expunged_string(ctx, idx, str); + ctx->sort_strings[seq] = str_len(str) == 0 ? "" : + p_strdup(ctx->sort_string_pool, str_c(str)); } } T_END; - return ctx->sort_strings[seq]; + *str_r = ctx->sort_strings[seq]; + return ret; } static void @@ -375,7 +388,7 @@ index_sort_bsearch(struct sort_string_context *ctx, const char *key, unsigned int start_idx, unsigned int *idx_r, const char **prev_str_r) { - const struct mail_sort_node *nodes; + struct mail_sort_node *nodes; const char *str, *str2; unsigned int idx, left_idx, right_idx, prev; int ret; @@ -385,17 +398,19 @@ index_sort_bsearch(struct sort_string_context *ctx, const char *key, idx = left_idx = start_idx; while (left_idx < right_idx) { idx = (left_idx + right_idx) / 2; - str = index_sort_get_string(ctx, idx, nodes[idx].seq); - if (str != &expunged_msg) + if (index_sort_get_string(ctx, idx, nodes[idx].seq, &str) > 0) ret = strcmp(key, str); else { - /* put expunged messages first */ + /* put expunged (and otherwise failed) messages first */ + nodes[idx].no_update = TRUE; ret = 1; for (prev = idx; prev > 0; ) { prev--; - str2 = index_sort_get_string(ctx, prev, - nodes[prev].seq); - if (str2 != &expunged_msg) { + if (index_sort_get_string(ctx, prev, + nodes[prev].seq, + &str2) <= 0) + nodes[prev].no_update = TRUE; + else { ret = strcmp(key, str2); if (ret <= 0) { idx = prev; @@ -424,9 +439,11 @@ index_sort_bsearch(struct sort_string_context *ctx, const char *key, prev = idx; do { prev--; - str2 = index_sort_get_string(ctx, prev, - nodes[prev].seq); - } while (str2 == &expunged_msg && prev > 0 && + ret = index_sort_get_string(ctx, prev, + nodes[prev].seq, &str2); + if (ret <= 0) + nodes[prev].no_update = TRUE; + } while (ret <= 0 && prev > 0 && nodes[prev-1].sort_id == nodes[prev].sort_id); *prev_str_r = str2; } @@ -450,9 +467,11 @@ static void index_sort_merge(struct sort_string_context *ctx) prev_str = NULL; for (zpos = nzpos = 0; zpos < zcount && nzpos < nzcount; ) { zstr = ctx->sort_strings[znodes[zpos].seq]; - nzstr = index_sort_get_string(ctx, nzpos, nznodes[nzpos].seq); + ret = index_sort_get_string(ctx, nzpos, nznodes[nzpos].seq, &nzstr); + if (ret <= 0) + nznodes[nzpos].no_update = TRUE; - if (nzstr != &expunged_msg) + if (ret > 0) ret = strcmp(zstr, nzstr); else if (prev_str != NULL && strcmp(zstr, prev_str) == 0) { /* identical to previous message, must keep them @@ -586,19 +605,19 @@ index_sort_add_ids_range(struct sort_string_context *ctx, } if (nodes[left_idx].sort_id != 0 && !no_left_str) { - left_str = index_sort_get_string(ctx, left_idx, - nodes[left_idx].seq); - if (left_str == &expunged_msg) { + if (index_sort_get_string(ctx, left_idx, + nodes[left_idx].seq, &left_str) <= 0) { /* not equivalent with any message */ + nodes[left_idx].no_update = TRUE; left_str = NULL; } left_idx++; } if (nodes[right_idx].sort_id != 0 && !no_right_str) { - right_str = index_sort_get_string(ctx, right_idx, - nodes[right_idx].seq); - if (right_str == &expunged_msg) { + if (index_sort_get_string(ctx, right_idx, + nodes[right_idx].seq, &right_str) <= 0) { /* not equivalent with any message */ + nodes[right_idx].no_update = TRUE; right_str = NULL; } right_idx--; @@ -610,12 +629,12 @@ index_sort_add_ids_range(struct sort_string_context *ctx, share. some messages' sort strings may be equivalent, so give them the same sort IDs. */ for (i = left_idx; i <= right_idx; i++) { - str = index_sort_get_string(ctx, i, nodes[i].seq); - if (str == &expunged_msg) { + if (index_sort_get_string(ctx, i, nodes[i].seq, &str) <= 0) { /* it doesn't really matter what we give to this message, since it's only temporary and we don't know its correct position anyway. so let's assume it's equivalent to previous message. */ + nodes[i].no_update = TRUE; nodes[i].sort_id = left_sort_id; continue; } @@ -721,7 +740,7 @@ static int sort_node_cmp(const struct mail_sort_node *n1, if (n1->sort_id > n2->sort_id) return !ctx->reverse ? 1 : -1; - return index_sort_node_cmp_type(ctx->program->temp_mail, + return index_sort_node_cmp_type(ctx->program, ctx->program->sort_program + 1, n1->seq, n2->seq); } @@ -859,6 +878,8 @@ void index_sort_list_finish_string(struct mail_search_sort_program *program) /* NOTE: we already freed nonzero_nodes and made it point to sorted_nodes. */ } + if (ctx->failed) + program->failed = TRUE; array_free(&ctx->zero_nodes); i_free(ctx); diff --git a/src/lib-storage/index/index-sort.c b/src/lib-storage/index/index-sort.c index 25e06c9689..66d139abe2 100644 --- a/src/lib-storage/index/index-sort.c +++ b/src/lib-storage/index/index-sort.c @@ -31,12 +31,19 @@ ARRAY_DEFINE_TYPE(mail_sort_node_float, struct mail_sort_node_float); struct sort_cmp_context { struct mail_search_sort_program *program; - struct mail *mail; bool reverse; }; static struct sort_cmp_context static_node_cmp_context; +static void +index_sort_program_set_mail_failed(struct mail_search_sort_program *program, + struct mail *mail) +{ + if (mailbox_get_last_mail_error(mail->box) != MAIL_ERROR_EXPUNGED) + program->failed = TRUE; +} + static void index_sort_list_add_arrival(struct mail_search_sort_program *program, struct mail *mail) @@ -46,8 +53,10 @@ index_sort_list_add_arrival(struct mail_search_sort_program *program, node = array_append_space(nodes); node->seq = mail->seq; - if (mail_get_received_date(mail, &node->date) < 0) + if (mail_get_received_date(mail, &node->date) < 0) { + index_sort_program_set_mail_failed(program, mail); node->date = 0; + } } static void @@ -60,11 +69,14 @@ index_sort_list_add_date(struct mail_search_sort_program *program, node = array_append_space(nodes); node->seq = mail->seq; - if (mail_get_date(mail, &node->date, &tz) < 0) + if (mail_get_date(mail, &node->date, &tz) < 0) { + index_sort_program_set_mail_failed(program, mail); node->date = 0; - else if (node->date == 0) { - if (mail_get_received_date(mail, &node->date) < 0) + } else if (node->date == 0) { + if (mail_get_received_date(mail, &node->date) < 0) { + index_sort_program_set_mail_failed(program, mail); node->date = 0; + } } } @@ -77,20 +89,24 @@ index_sort_list_add_size(struct mail_search_sort_program *program, node = array_append_space(nodes); node->seq = mail->seq; - if (mail_get_virtual_size(mail, &node->size) < 0) + if (mail_get_virtual_size(mail, &node->size) < 0) { + index_sort_program_set_mail_failed(program, mail); node->size = 0; + } } -static uoff_t index_sort_get_pop3_order(struct mail *mail) +static int index_sort_get_pop3_order(struct mail *mail, uoff_t *size_r) { const char *str; - uoff_t size; - if (mail_get_special(mail, MAIL_FETCH_POP3_ORDER, &str) < 0 || - str_to_uoff(str, &size) < 0) - return (uint32_t)-1; - else - return size; + if (mail_get_special(mail, MAIL_FETCH_POP3_ORDER, &str) < 0) { + *size_r = (uint32_t)-1; + return -1; + } + + if (str_to_uoff(str, size_r) < 0) + *size_r = (uint32_t)-1; + return 0; } static void @@ -102,17 +118,19 @@ index_sort_list_add_pop3_order(struct mail_search_sort_program *program, node = array_append_space(nodes); node->seq = mail->seq; - node->size = index_sort_get_pop3_order(mail); + (void)index_sort_get_pop3_order(mail, &node->size); } -static float index_sort_get_relevancy(struct mail *mail) +static int index_sort_get_relevancy(struct mail *mail, float *result_r) { const char *str; - if (mail_get_special(mail, MAIL_FETCH_SEARCH_RELEVANCY, &str) < 0) - return 0; - else - return strtod(str, NULL); + if (mail_get_special(mail, MAIL_FETCH_SEARCH_RELEVANCY, &str) < 0) { + *result_r = 0; + return -1; + } + *result_r = strtod(str, NULL); + return 0; } static void @@ -124,7 +142,7 @@ index_sort_list_add_relevancy(struct mail_search_sort_program *program, node = array_append_space(nodes); node->seq = mail->seq; - node->num = index_sort_get_relevancy(mail); + (void)index_sort_get_relevancy(mail, &node->num); } void index_sort_list_add(struct mail_search_sort_program *program, @@ -147,7 +165,7 @@ static int sort_node_date_cmp(const struct mail_sort_node_date *n1, if (n1->date > n2->date) return !ctx->reverse ? 1 : -1; - return index_sort_node_cmp_type(ctx->mail, + return index_sort_node_cmp_type(ctx->program, ctx->program->sort_program + 1, n1->seq, n2->seq); } @@ -173,7 +191,7 @@ static int sort_node_size_cmp(const struct mail_sort_node_size *n1, if (n1->size > n2->size) return !ctx->reverse ? 1 : -1; - return index_sort_node_cmp_type(ctx->mail, + return index_sort_node_cmp_type(ctx->program, ctx->program->sort_program + 1, n1->seq, n2->seq); } @@ -199,7 +217,7 @@ static int sort_node_float_cmp(const struct mail_sort_node_float *n1, if (n1->num > n2->num) return !ctx->reverse ? 1 : -1; - return index_sort_node_cmp_type(ctx->mail, + return index_sort_node_cmp_type(ctx->program, ctx->program->sort_program + 1, n1->seq, n2->seq); } @@ -224,7 +242,6 @@ void index_sort_list_finish(struct mail_search_sort_program *program) { i_zero(&static_node_cmp_context); static_node_cmp_context.program = program; - static_node_cmp_context.mail = program->temp_mail; static_node_cmp_context.reverse = (program->sort_program[0] & MAIL_SORT_FLAG_REVERSE) != 0; @@ -330,7 +347,7 @@ index_sort_program_init(struct mailbox_transaction_context *t, return program; } -void index_sort_program_deinit(struct mail_search_sort_program **_program) +int index_sort_program_deinit(struct mail_search_sort_program **_program) { struct mail_search_sort_program *program = *_program; @@ -340,7 +357,10 @@ void index_sort_program_deinit(struct mail_search_sort_program **_program) index_sort_list_finish(program); mail_free(&program->temp_mail); array_free(&program->seqs); + + int ret = program->failed ? -1 : 0; i_free(program); + return ret; } static int @@ -417,12 +437,17 @@ int index_sort_header_get(struct mail *mail, uint32_t seq, switch (sort_type & MAIL_SORT_MASK) { case MAIL_SORT_SUBJECT: - if ((ret = mail_get_first_header(mail, "Subject", &str)) <= 0) - return ret; + ret = mail_get_first_header(mail, "Subject", &str); + if (ret < 0) + break; + if (ret == 0) { + /* nonexistent header */ + return 1; + } str = imap_get_base_subject_cased(pool_datastack_create(), str, &reply_or_fw); str_append(dest, str); - return 0; + return 1; case MAIL_SORT_CC: ret = get_first_mailbox(mail, "Cc", &str); break; @@ -441,15 +466,21 @@ int index_sort_header_get(struct mail *mail, uint32_t seq, default: i_unreached(); } + if (ret < 0) { + if (mailbox_get_last_mail_error(mail->box) == MAIL_ERROR_EXPUNGED) + return 0; + return -1; + } (void)uni_utf8_to_decomposed_titlecase(str, strlen(str), dest); - return ret; + return 1; } -int index_sort_node_cmp_type(struct mail *mail, +int index_sort_node_cmp_type(struct mail_search_sort_program *program, const enum mail_sort_type *sort_program, uint32_t seq1, uint32_t seq2) { + struct mail *mail = program->temp_mail; enum mail_sort_type sort_type; time_t time1, time2; uoff_t size1, size2; @@ -469,39 +500,51 @@ int index_sort_node_cmp_type(struct mail *mail, str1 = t_str_new(256); str2 = t_str_new(256); - (void)index_sort_header_get(mail, seq1, sort_type, str1); - (void)index_sort_header_get(mail, seq2, sort_type, str2); + if (index_sort_header_get(mail, seq1, sort_type, str1) < 0) + index_sort_program_set_mail_failed(program, mail); + if (index_sort_header_get(mail, seq2, sort_type, str2) < 0) + index_sort_program_set_mail_failed(program, mail); ret = strcmp(str_c(str1), str_c(str2)); } T_END; break; case MAIL_SORT_ARRIVAL: mail_set_seq(mail, seq1); - if (mail_get_received_date(mail, &time1) < 0) + if (mail_get_received_date(mail, &time1) < 0) { + index_sort_program_set_mail_failed(program, mail); time1 = 0; + } mail_set_seq(mail, seq2); - if (mail_get_received_date(mail, &time2) < 0) - time1 = 0; + if (mail_get_received_date(mail, &time2) < 0) { + index_sort_program_set_mail_failed(program, mail); + time2 = 0; + } ret = time1 < time2 ? -1 : (time1 > time2 ? 1 : 0); break; case MAIL_SORT_DATE: mail_set_seq(mail, seq1); - if (mail_get_date(mail, &time1, &tz) < 0) + if (mail_get_date(mail, &time1, &tz) < 0) { + index_sort_program_set_mail_failed(program, mail); time1 = 0; - else if (time1 == 0) { - if (mail_get_received_date(mail, &time1) < 0) + } else if (time1 == 0) { + if (mail_get_received_date(mail, &time1) < 0) { + index_sort_program_set_mail_failed(program, mail); time1 = 0; + } } mail_set_seq(mail, seq2); - if (mail_get_date(mail, &time2, &tz) < 0) + if (mail_get_date(mail, &time2, &tz) < 0) { + index_sort_program_set_mail_failed(program, mail); time2 = 0; - else if (time2 == 0) { - if (mail_get_received_date(mail, &time2) < 0) + } else if (time2 == 0) { + if (mail_get_received_date(mail, &time2) < 0) { + index_sort_program_set_mail_failed(program, mail); time2 = 0; + } } ret = time1 < time2 ? -1 : @@ -509,21 +552,27 @@ int index_sort_node_cmp_type(struct mail *mail, break; case MAIL_SORT_SIZE: mail_set_seq(mail, seq1); - if (mail_get_virtual_size(mail, &size1) < 0) + if (mail_get_virtual_size(mail, &size1) < 0) { + index_sort_program_set_mail_failed(program, mail); size1 = 0; + } mail_set_seq(mail, seq2); - if (mail_get_virtual_size(mail, &size2) < 0) + if (mail_get_virtual_size(mail, &size2) < 0) { + index_sort_program_set_mail_failed(program, mail); size2 = 0; + } ret = size1 < size2 ? -1 : (size1 > size2 ? 1 : 0); break; case MAIL_SORT_RELEVANCY: mail_set_seq(mail, seq1); - float1 = index_sort_get_relevancy(mail); + if (index_sort_get_relevancy(mail, &float1) < 0) + index_sort_program_set_mail_failed(program, mail); mail_set_seq(mail, seq2); - float2 = index_sort_get_relevancy(mail); + if (index_sort_get_relevancy(mail, &float2) < 0) + index_sort_program_set_mail_failed(program, mail); /* NOTE: higher relevancy is returned first, unlike with all other number based sort keys */ @@ -534,9 +583,11 @@ int index_sort_node_cmp_type(struct mail *mail, /* 32bit numbers would be enough, but since there is already existing code for uoff_t in sizes, just use them. */ mail_set_seq(mail, seq1); - size1 = index_sort_get_pop3_order(mail); + if (index_sort_get_pop3_order(mail, &size1) < 0) + index_sort_program_set_mail_failed(program, mail); mail_set_seq(mail, seq2); - size2 = index_sort_get_pop3_order(mail); + if (index_sort_get_pop3_order(mail, &size2) < 0) + index_sort_program_set_mail_failed(program, mail); ret = size1 < size2 ? -1 : (size1 > size2 ? 1 : 0); @@ -550,7 +601,7 @@ int index_sort_node_cmp_type(struct mail *mail, } if (ret == 0) { - return index_sort_node_cmp_type(mail, sort_program+1, + return index_sort_node_cmp_type(program, sort_program+1, seq1, seq2); } diff --git a/src/lib-storage/index/index-sort.h b/src/lib-storage/index/index-sort.h index fdcb4f748c..b0e2770779 100644 --- a/src/lib-storage/index/index-sort.h +++ b/src/lib-storage/index/index-sort.h @@ -6,7 +6,7 @@ struct mail_search_sort_program; struct mail_search_sort_program * index_sort_program_init(struct mailbox_transaction_context *t, const enum mail_sort_type *sort_program); -void index_sort_program_deinit(struct mail_search_sort_program **program); +int index_sort_program_deinit(struct mail_search_sort_program **program); void index_sort_list_add(struct mail_search_sort_program *program, struct mail *mail); -- 2.47.3