From 5d3ecfce782e801dafefde2ff5e3fae0ac5406f3 Mon Sep 17 00:00:00 2001 From: Michael M Slusarz Date: Mon, 11 Mar 2024 13:59:01 -0600 Subject: [PATCH] Fix assert-crash due to excessive scores length For searches that hit multiple mailboxes (virtual mailboxes is an easy way to force a multisearch), an assert-crash will happen if a message matches both the main query and internal "maybe" queries. Fix by ensuring that scores are only added once per UID. This was broken in single mailbox search as well, but it was ignored there as there was no assert check on scores length (the extra entries were ignored during processing). In order to calcuate scores correctly (now that we are only adding them once), refactor result iteration to only run the maybe query once for all maybe queries. Add the main query to this query for score calculation purposes. This is a bit of an optimization, so nice. Fixes GitHub Issue #61 --- .github/common/fts-flatcurve-test.sh | 4 ++ .github/common/imaptest/issue-61 | 24 ++++++++++ src/fts-backend-flatcurve-xapian.cpp | 68 ++++++++++++++++------------ 3 files changed, 66 insertions(+), 30 deletions(-) create mode 100644 .github/common/imaptest/issue-61 diff --git a/.github/common/fts-flatcurve-test.sh b/.github/common/fts-flatcurve-test.sh index 165c554..1cefddc 100755 --- a/.github/common/fts-flatcurve-test.sh +++ b/.github/common/fts-flatcurve-test.sh @@ -203,6 +203,10 @@ run_test "Testing GitHub Issue #38" \ /dovecot/configs/dovecot.conf.issue-38 \ /dovecot/imaptest/issue-38/issue-38 +run_test "Testing GitHub Issue #61" \ + /dovecot/configs/dovecot.conf.virtual \ + /dovecot/imaptest/issue-61 + TESTBOX=INBOX echo echo "Testing 'doveadm fts-flatcurve remove'" diff --git a/.github/common/imaptest/issue-61 b/.github/common/imaptest/issue-61 new file mode 100644 index 0000000..4b59831 --- /dev/null +++ b/.github/common/imaptest/issue-61 @@ -0,0 +1,24 @@ +messages: 1 + +ok search or from user body body +* search 1 +ok search or header reply-to user body body +* search 1 +ok search or header reply-to foo body bar +* search +ok search or header reply-to user or header reply-to domain body body +* search 1 + +ok examine Virtual/All + +# From header works +ok search or from user body body +* search 1 + +# Panic: file fts-search.c: line 87 (level_scores_add_vuids): assertion failed: (array_count(&vuids_arr) == array_count(&br->scores)) +ok search or header reply-to user body body +* search 1 +ok search or header reply-to foo body bar +* search +ok search or header reply-to user or header reply-to domain body body +* search 1 diff --git a/src/fts-backend-flatcurve-xapian.cpp b/src/fts-backend-flatcurve-xapian.cpp index 38b8f84..77d8aaa 100644 --- a/src/fts-backend-flatcurve-xapian.cpp +++ b/src/fts-backend-flatcurve-xapian.cpp @@ -196,10 +196,11 @@ struct fts_flatcurve_xapian_query_iter { Xapian::Database *db; Xapian::Enquire *enquire; Xapian::MSetIterator i; + Xapian::MSet m; struct fts_flatcurve_xapian_query_result *result; - int curr_query; - bool next_query:1; + bool init:1; + bool main_query:1; }; static void @@ -1687,10 +1688,8 @@ fts_flatcurve_xapian_query_iter_init(struct flatcurve_fts_query *query) struct fts_flatcurve_xapian_query_iter *iter; iter = p_new(query->pool, struct fts_flatcurve_xapian_query_iter, 1); - /* -1: "Master" query - * >= 0: Current index of maybe_queries */ - iter->curr_query = -1; - iter->next_query = TRUE; + iter->init = FALSE; + iter->main_query = TRUE; iter->query = query; iter->result = p_new(query->pool, struct fts_flatcurve_xapian_query_result, 1); @@ -1701,33 +1700,36 @@ fts_flatcurve_xapian_query_iter_init(struct flatcurve_fts_query *query) struct fts_flatcurve_xapian_query_result * fts_flatcurve_xapian_query_iter_next(struct fts_flatcurve_xapian_query_iter *iter) { - Xapian::MSet m; + Xapian::Query maybe, *q = NULL; const struct flatcurve_fts_query_xapian_maybe *mquery; enum flatcurve_xapian_db_opts opts = ENUM_EMPTY(flatcurve_xapian_db_opts); - Xapian::Query *q = NULL; - if (iter->next_query) { - iter->next_query = FALSE; + if (!iter->init) { + iter->init = TRUE; /* Master query. */ - if (iter->curr_query == -1) { + if (iter->main_query) { if (iter->query->xapian->query == NULL) - ++iter->curr_query; + iter->main_query = FALSE; else q = iter->query->xapian->query; } /* Maybe queries. */ - if ((iter->curr_query >= 0) && - (array_is_created(&iter->query->xapian->maybe_queries)) && - (array_count(&iter->query->xapian->maybe_queries) > iter->curr_query)) { - mquery = array_idx(&iter->query->xapian->maybe_queries, - iter->curr_query); - q = mquery->query; + if (!iter->main_query && + array_is_created(&iter->query->xapian->maybe_queries)) { + maybe = Xapian::Query(); + array_foreach(&iter->query->xapian->maybe_queries, mquery) + maybe = Xapian::Query(Xapian::Query::OP_OR, maybe, *mquery->query); + /* Add main query to merge Xapian scores correctly. */ + if (iter->query->xapian->query != NULL) + maybe = Xapian::Query(Xapian::Query::OP_AND_MAYBE, maybe, + *iter->query->xapian->query); + q = &maybe; } - if (iter->db == NULL) + if ((q != NULL) && (iter->db == NULL)) iter->db = fts_flatcurve_xapian_read_db(iter->query->backend, opts); if ((q == NULL) || (iter->db == NULL)) @@ -1740,7 +1742,7 @@ fts_flatcurve_xapian_query_iter_next(struct fts_flatcurve_xapian_query_iter *ite iter->enquire->set_query(*q); try { - m = iter->enquire->get_mset(0, iter->db->get_doccount()); + iter->m = iter->enquire->get_mset(0, iter->db->get_doccount()); } catch (Xapian::DatabaseModifiedError &e) { /* Per documentation, this is only thrown if more than * one change has been made to the database. To @@ -1754,16 +1756,17 @@ fts_flatcurve_xapian_query_iter_next(struct fts_flatcurve_xapian_query_iter *ite i_unreached(); } - iter->i = m.begin(); + iter->i = iter->m.begin(); } - if (iter->i == m.end()) { - ++iter->curr_query; - iter->next_query = TRUE; + if (iter->i == iter->m.end()) { + if (!iter->main_query) + return NULL; + iter->main_query = iter->init = FALSE; return fts_flatcurve_xapian_query_iter_next(iter); } - iter->result->maybe = (iter->curr_query >= 0); + iter->result->maybe = !iter->main_query; iter->result->score = iter->i.get_weight(); /* MSet docid can be an "interleaved" docid generated by * Xapian::Database when handling multiple DBs at once. Instead, we @@ -1799,13 +1802,18 @@ bool fts_flatcurve_xapian_run_query(struct flatcurve_fts_query *query, if ((iter = fts_flatcurve_xapian_query_iter_init(query)) == NULL) return FALSE; while ((result = fts_flatcurve_xapian_query_iter_next(iter)) != NULL) { - if (result->maybe || query->xapian->maybe) + bool add_score = TRUE; + if (result->maybe || query->xapian->maybe) { + add_score = (!seq_range_exists(&r->uids, result->uid) && + !seq_range_exists(&r->maybe_uids, result->uid)); seq_range_array_add(&r->maybe_uids, result->uid); - else + } else seq_range_array_add(&r->uids, result->uid); - score = array_append_space(&r->scores); - score->score = (float)result->score; - score->uid = result->uid; + if (add_score) { + score = array_append_space(&r->scores); + score->score = (float)result->score; + score->uid = result->uid; + } } fts_flatcurve_xapian_query_iter_deinit(&iter); return TRUE;