From e352fc03d81704654f6588516ebed97ba106b07c Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 19:21:38 +0900 Subject: [PATCH 01/38] Cassandane: add IO::File::Lockable to the deps --- cassandane/doc/README.deps | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cassandane/doc/README.deps b/cassandane/doc/README.deps index a5eb5bb607..b7d4588a57 100644 --- a/cassandane/doc/README.deps +++ b/cassandane/doc/README.deps @@ -163,7 +163,8 @@ Debian/Ubuntu copypasta: libdbd-sqlite3-perl libdigest-crc-perl libxml-simple-perl sudo cpan Math::Int64 Mail::JMAPTalk Mail::IMAPTalk \ - Net::CalDAVTalk Net::CardDAVTalk String::CRC32 + Net::CalDAVTalk Net::CardDAVTalk String::CRC32 \ + IO::File::Lockable Fedora copypasta: # there's heaps more to add here... needs to be tested From cb42eee7a9ef2453bcc989ff9d9ad5607653ab6a Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Sun, 5 Nov 2023 19:48:19 +0100 Subject: [PATCH 02/38] reconstruct - just take the exclusive lock when doing setversion --- imap/mailbox.c | 36 +----------------------------------- imap/reconstruct.c | 43 +++++++++++++++++++++++-------------------- 2 files changed, 24 insertions(+), 55 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 78b82bbdd2..7aee5ece70 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -1220,41 +1220,7 @@ EXPORTED modseq_t mailbox_modseq_dirty(struct mailbox *mailbox) EXPORTED int mailbox_setversion(struct mailbox *mailbox, int version) { - int r = 0; - - if (version && mailbox->i.minor_version != version) { - /* need to re-set the version! */ - struct mailboxlist *listitem = find_listitem(mailbox_name(mailbox)); - - assert(listitem); - assert(&listitem->m == mailbox); - - /* we need an exclusive lock on the listitem because we're renaming - * index files, so release locks and then go full exclusive */ - mailbox_unlock_index(mailbox, NULL); - r = mailbox_mboxlock_reopen(listitem, LOCK_EXCLUSIVE, LOCK_EXCLUSIVE); - - /* we need to re-open the index because we dropped the mboxname lock, - * so the file may have changed */ - if (!r) r = mailbox_open_index(mailbox); - - /* lock_internal so DELETED doesn't cause it to appear to be - * NONEXISTENT */ - if (!r) r = mailbox_lock_index_internal(mailbox, LOCK_EXCLUSIVE); - - /* perform the actual repack! */ - if (!r) r = mailbox_index_repack(mailbox, version); - - /* NOTE: this leaves the mailbox in an unlocked state internally, so - * let's release all the acutal locks */ - mailbox_unlock_index(mailbox, NULL); - - /* we're also still holding an exclusive namelock in the listitem, - * but that's OK because the only caller will be calling mailbox_close - * immediately afterwards */ - } - - return r; + return mailbox_index_repack(mailbox, version); } static void _delayed_cleanup(void *rock) diff --git a/imap/reconstruct.c b/imap/reconstruct.c index eb7b86cf36..ce0fd4e52a 100644 --- a/imap/reconstruct.c +++ b/imap/reconstruct.c @@ -521,21 +521,35 @@ static int do_reconstruct(struct findall_data *data, void *rock) struct mboxlock *namespacelock = mboxname_usernamespacelock(name); if (setversion) { - r = mailbox_open_iwl(name, &mailbox); + r = mailbox_open_exclusive(name, &mailbox); if (r) { com_err(name, r, "Failed to open mailbox to set version"); mboxname_release(&namespacelock); return 0; } - } - else { - r = mailbox_reconstruct(name, reconstruct_flags, &mailbox); - if (r) { - com_err(name, r, "%s", - (r == IMAP_IOERROR) ? error_message(errno) : "Failed to reconstruct mailbox"); - mboxname_release(&namespacelock); - return 0; + if (setversion != mailbox->i.minor_version) { + int oldversion = mailbox->i.minor_version; + /* need to re-set the version! */ + int r = mailbox_setversion(mailbox, setversion); + char *extname = mboxname_to_external(name, &recon_namespace, NULL); + if (r) { + printf("FAILED TO REPACK %s with new version %s\n", extname, error_message(r)); + } + else { + printf("Converted %s version %d to %d\n", extname, oldversion, setversion); + } + free(extname); } + mboxname_release(&namespacelock); + return 0; + } + + r = mailbox_reconstruct(name, reconstruct_flags, &mailbox); + if (r) { + com_err(name, r, "%s", + (r == IMAP_IOERROR) ? error_message(errno) : "Failed to reconstruct mailbox"); + mboxname_release(&namespacelock); + return 0; } mbentry_t *mbentry_byid = NULL; @@ -660,17 +674,6 @@ static int do_reconstruct(struct findall_data *data, void *rock) strncpy(outpath, mailbox_meta_fname(mailbox, META_HEADER), MAX_MAILBOX_NAME); - if (setversion && setversion != mailbox->i.minor_version) { - int oldversion = mailbox->i.minor_version; - /* need to re-set the version! */ - int r = mailbox_setversion(mailbox, setversion); - if (r) { - printf("FAILED TO REPACK %s with new version %s\n", extname, error_message(r)); - } - else { - printf("Converted %s version %d to %d\n", extname, oldversion, setversion); - } - } if (make_changes) { mailbox_commit(mailbox); } From 26f16728a841b607e0150b688fcedf8ee19839b5 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Sun, 5 Nov 2023 14:53:55 +0100 Subject: [PATCH 03/38] mailbox: replace errant \t this crept in during a recent commit --- imap/mailbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 7aee5ece70..64dd981072 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -3854,7 +3854,7 @@ static int mailbox_update_webdav(struct mailbox *mailbox, } } if (!buf_len(&resource)) - buf_printf(&resource, "imapuid-%u", new->uid); + buf_printf(&resource, "imapuid-%u", new->uid); webdavdb = mailbox_open_webdav(mailbox); From 811e775c077b445fa498b57e11e336deda601645 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 13:57:39 +0100 Subject: [PATCH 04/38] mailbox: add IOERROR to syslog for conversations error --- imap/mailbox.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 64dd981072..fe63cb1676 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -2691,9 +2691,10 @@ EXPORTED void mailbox_unlock_index(struct mailbox *mailbox, struct statusdata *s if (mailbox->local_cstate) { int r = conversations_commit(&mailbox->local_cstate); - if (r) - syslog(LOG_ERR, "Error committing to conversations database for mailbox %s: %s", + if (r) { + syslog(LOG_ERR, "IOERROR: Error committing to conversations database for mailbox %s: %s", mailbox_name(mailbox), error_message(r)); + } } // release the namespacelock here From d1cd4f3e441c59d9e12cb3dbf91b3f8c386d9e33 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 13:08:50 +0100 Subject: [PATCH 05/38] mailbox_rename_copy: remove stale incorrect-now comment --- imap/mailbox.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index fe63cb1676..c1c9ad0e4c 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -6611,9 +6611,6 @@ HIDDEN int mailbox_rename_nocopy(struct mailbox *oldmailbox, return r; } -/* if 'userid' is set, we perform the funky RENAME INBOX INBOX.old - semantics, regardless of whether or not the name of the mailbox is - 'user.foo'.*/ /* requires a write-locked oldmailbox pointer, since we delete it immediately afterwards */ /* This function ONLY WORKS if the type is legacy */ From b31a23723a0e51e2dfe652e7c6a83d2088bf3f07 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 13:09:33 +0100 Subject: [PATCH 06/38] mailbox_rename_copy: explain highestmodseq handling --- imap/mailbox.c | 1 + 1 file changed, 1 insertion(+) diff --git a/imap/mailbox.c b/imap/mailbox.c index c1c9ad0e4c..efaaf15624 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -6644,6 +6644,7 @@ HIDDEN int mailbox_rename_copy(struct mailbox *oldmailbox, if (!uidvalidity) uidvalidity = mboxname_nextuidvalidity(newname, oldmailbox->i.uidvalidity); + /* zero means mailbox_create will set it */ modseq_t highestmodseq = silent ? oldmailbox->i.highestmodseq : 0; /* Create new mailbox */ From d6fe2128c2691e9a52421b0a3fc76a1ba60b250b Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 13:55:27 +0100 Subject: [PATCH 07/38] mailbox: make open_mailboxes more usable * allow re-opening locked mailboxes * refcount inside the mailbox object * find existing based on the namelock name (uniqueid for new mailboxes) rather than the name This will allow regular mailbox_open to be used by JMAP and replace the mailbox cache there, as well as allowing mboxlist_changesub to open a mailbox without having to keep track of what's open now --- imap/mailbox.c | 322 ++++++++++++++++++++----------------------------- imap/mailbox.h | 8 +- 2 files changed, 140 insertions(+), 190 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index efaaf15624..cd24ee06f8 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -142,16 +142,7 @@ struct mailbox_iter { unsigned nchecktime; }; - -struct mailboxlist { - struct mailboxlist *next; - char *name; - struct mailbox m; - struct mboxlock *l; - int nopen; -}; - -static struct mailboxlist *open_mailboxes = NULL; +static struct mailbox *open_mailboxes = NULL; #define zeromailbox(m) do { memset(&m, 0, sizeof(struct mailbox)); \ (m).index_fd = -1; \ @@ -245,17 +236,16 @@ EXPORTED int open_mailboxes_exist() return open_mailboxes ? 1 : 0; } -static struct mailboxlist *create_listitem(const char *name) +static struct mailbox *create_listitem() { - struct mailboxlist *item = xzmalloc(sizeof(struct mailboxlist)); + struct mailbox *item = xzmalloc(sizeof(struct mailbox)); + zeromailbox(*item); + item->next = open_mailboxes; open_mailboxes = item; - item->nopen = 1; - zeromailbox(item->m); - item->name = xstrdup(name); - /* ensure we never print insane times */ - gettimeofday(&item->m.starttime, 0); + item->refcount = 1; + gettimeofday(&item->starttime, 0); #if defined ENABLE_OBJECTSTORE if (config_getswitch(IMAPOPT_OBJECT_STORAGE_ENABLED)) @@ -265,32 +255,32 @@ static struct mailboxlist *create_listitem(const char *name) return item; } -static struct mailboxlist *find_listitem(const char *name) +static struct mailbox *find_listitem(const char *lockname) { - struct mailboxlist *item; + struct mailbox *item; for (item = open_mailboxes; item; item = item->next) { - if (!strcmp(name, item->name)) + if (!strcmp(lockname, item->lockname)) return item; } return NULL; } -static void remove_listitem(struct mailboxlist *remitem) +static void remove_listitem(struct mailbox *remitem) { - struct mailboxlist *item; - struct mailboxlist *previtem = NULL; + struct mailbox *item; + struct mailbox *previtem = NULL; for (item = open_mailboxes; item; item = item->next) { if (item == remitem) { - if (previtem) previtem->next = item->next; else open_mailboxes = item->next; - free(item->name); - free(item); + + free(remitem->lockname); + free(remitem); #if defined ENABLE_OBJECTSTORE if (!open_mailboxes && config_getswitch(IMAPOPT_OBJECT_STORAGE_ENABLED)) // time to close the database @@ -910,10 +900,10 @@ static void mailbox_release_resources(struct mailbox *mailbox) xclose(mailbox->header_fd); /* release and unmap index */ - xclose(mailbox->index_fd); - mailbox->index_locktype = 0; /* lock was released by closing fd */ if (mailbox->index_base) map_free(&mailbox->index_base, &mailbox->index_len); + xclose(mailbox->index_fd); + mailbox->index_locktype = 0; /* lock was released by closing fd */ /* release caches */ for (i = 0; i < mailbox->caches.count; i++) { @@ -926,12 +916,12 @@ static void mailbox_release_resources(struct mailbox *mailbox) /* * Open the index file for 'mailbox' */ -static int mailbox_open_index(struct mailbox *mailbox) +static int mailbox_open_index(struct mailbox *mailbox, int index_locktype) { - struct stat sbuf; const char *fname; - int openflags = mailbox->is_readonly ? O_RDONLY : O_RDWR; + int openflags = index_locktype == LOCK_SHARED ? O_RDONLY : O_RDWR; + assert(!mailbox->index_locktype); mailbox_release_resources(mailbox); /* open and map the index file */ @@ -943,46 +933,35 @@ static int mailbox_open_index(struct mailbox *mailbox) if (mailbox->index_fd == -1) return IMAP_IOERROR; - /* don't open the cache yet, it will be loaded by lazy-loading - * later */ - - fstat(mailbox->index_fd, &sbuf); - mailbox->index_ino = sbuf.st_ino; - mailbox->index_mtime = sbuf.st_mtime; - mailbox->index_size = sbuf.st_size; - map_refresh(mailbox->index_fd, 0, &mailbox->index_base, - &mailbox->index_len, mailbox->index_size, - "index", mailbox_name(mailbox)); - return 0; } -static int mailbox_mboxlock_reopen(struct mailboxlist *listitem, int locktype, int index_locktype) + +static int mailbox_relock(struct mailbox *mailbox, int locktype, int index_locktype) { - struct mailbox *mailbox = &listitem->m; - uint32_t legacy_dirs = (mailbox_mbtype(mailbox) & MBTYPE_LEGACY_DIRS); - int r; + int r = 0; + mailbox_unlock_index(mailbox, NULL); mailbox_release_resources(mailbox); - - mboxname_release(&listitem->l); + mboxname_release(&mailbox->namelock); mboxname_release(&mailbox->local_namespacelock); - + r = mboxname_lock(mailbox->lockname, &mailbox->namelock, locktype); + if (r) return r; + r = mailbox_open_index(mailbox, index_locktype); + if (r) return r; char *userid = mboxname_to_userid(mailbox_name(mailbox)); if (userid) { int haslock = user_isnamespacelocked(userid); if (haslock) { - if (index_locktype != LOCK_SHARED) assert(haslock != LOCK_SHARED); + if (haslock == LOCK_SHARED) + assert(index_locktype == LOCK_SHARED); } else { mailbox->local_namespacelock = user_namespacelock_full(userid, index_locktype); } free(userid); } - - r = mboxname_lock(legacy_dirs ? mailbox_name(mailbox) : mailbox_uniqueid(mailbox), &listitem->l, locktype); - if (r) return r; - + r = mailbox_lock_index_internal(mailbox, index_locktype); return r; } @@ -996,34 +975,31 @@ static int mailbox_open_advanced(const char *name, const mbentry_t *mbe, struct mailbox **mailboxptr) { + assert(*mailboxptr == NULL); + + struct mailbox *mailbox = find_listitem(name); mbentry_t *mbentry = NULL; - struct mailboxlist *listitem; - struct mailbox *mailbox = NULL; int r = 0; - assert(*mailboxptr == NULL); - - listitem = find_listitem(name); /* already open? just use this one */ - if (listitem) { + if (mailbox) { /* can't reuse an exclusive locked mailbox */ - if (listitem->l->locktype & LOCK_EXCLUSIVE) - return IMAP_MAILBOX_LOCKED; - if (locktype & LOCK_EXCLUSIVE) + if (mailbox->namelock->locktype & LOCK_EXCLUSIVE) return IMAP_MAILBOX_LOCKED; - /* can't reuse an already locked index */ - if (listitem->m.index_locktype) + + /* can't promote a readonly index */ + if (mailbox->index_locktype == LOCK_SHARED && locktype == LOCK_EXCLUSIVE) return IMAP_MAILBOX_LOCKED; - listitem->nopen++; - mailbox = &listitem->m; + mailbox->refcount++; + + if (!mailbox->index_locktype) goto lockindex; - goto lockindex; + goto done; } - listitem = create_listitem(name); - mailbox = &listitem->m; + mailbox = create_listitem(); // lock the user namespace FIRST before the mailbox namespace char *userid = mboxname_to_userid(name); @@ -1054,10 +1030,9 @@ static int mailbox_open_advanced(const char *name, r = IMAP_MAILBOX_NONEXISTENT; if (r) { - if (mailbox->local_namespacelock) - mboxname_release(&mailbox->local_namespacelock); + mboxname_release(&mailbox->local_namespacelock); mboxlist_entry_free(&mbentry); - remove_listitem(listitem); + remove_listitem(mailbox); return r; } @@ -1071,27 +1046,24 @@ static int mailbox_open_advanced(const char *name, } uint32_t legacy_dirs = (mbentry->mbtype & MBTYPE_LEGACY_DIRS); - r = mboxname_lock(legacy_dirs ? name : mbentry->uniqueid, &listitem->l, locktype); + mailbox->lockname = xstrdup(legacy_dirs ? name : mbentry->uniqueid); + r = mboxname_lock(mailbox->lockname, &mailbox->namelock, locktype); if (r) { /* locked is not an error - just means we asked for NONBLOCKING */ if (r != IMAP_MAILBOX_LOCKED) xsyslog(LOG_ERR, "IOERROR: lock failed", "mailbox=<%s> error=<%s>", name, error_message(r)); - if (mailbox->local_namespacelock) - mboxname_release(&mailbox->local_namespacelock); + mboxname_release(&mailbox->local_namespacelock); mboxlist_entry_free(&mbentry); - remove_listitem(listitem); + remove_listitem(mailbox); return r; } if (!mbentry->name) mbentry->name = xstrdup(name); mailbox->mbentry = mbentry; - if (index_locktype == LOCK_SHARED) - mailbox->is_readonly = 1; - - r = mailbox_open_index(mailbox); + r = mailbox_open_index(mailbox, index_locktype); if (r) { xsyslog(LOG_ERR, "IOERROR: opening index failed", "mailbox=<%s> error=<%s>", @@ -1257,35 +1229,20 @@ EXPORTED void mailbox_close(struct mailbox **mailboxptr) { int flag; struct mailbox *mailbox = *mailboxptr; - struct mailboxlist *listitem; /* be safe against double-close */ if (!mailbox) return; - listitem = find_listitem(mailbox_name(mailbox)); - assert(listitem && &listitem->m == mailbox); - *mailboxptr = NULL; /* open multiple times? Just close this one */ - if (listitem->nopen > 1) { - listitem->nopen--; - mailbox_unlock_index(mailbox, NULL); + if (mailbox->refcount > 1) { + mailbox->refcount--; return; } - if (mailbox->index_fd != -1) { - /* drop the index lock here because we'll lose our right to it - * when try to upgrade the mboxlock anyway. */ - mailbox_unlock_index(mailbox, NULL); - } - if (mailbox->i.options & OPT_MAILBOX_DELETED) { - int r = mailbox_mboxlock_reopen(listitem, LOCK_EXCLUSIVE, LOCK_EXCLUSIVE); - if (!r) r = mailbox_open_index(mailbox); - /* lock_internal so DELETED doesn't cause it to appear to be - * NONEXISTENT - but we still need conversations so we can write changes! */ - if (!r) r = mailbox_lock_index_internal(mailbox, LOCK_EXCLUSIVE); + int r = mailbox_relock(mailbox, LOCK_EXCLUSIVE, LOCK_EXCLUSIVE); /* double check just in case a new mailbox with the same name got created * in a race condition and isn't deleted! */ if (!r && (mailbox->i.options & OPT_MAILBOX_DELETED)) { @@ -1293,7 +1250,6 @@ EXPORTED void mailbox_close(struct mailbox **mailboxptr) (mailbox_mbtype(mailbox) & MBTYPE_LEGACY_DIRS) ? NULL : mailbox_uniqueid(mailbox)); } - mailbox_unlock_index(mailbox, NULL); } else if (!in_shutdown && (mailbox->i.options & MAILBOX_CLEANUP_MASK)) { // there's cleanup to do! Schedule it for after we've replied to the user @@ -1301,6 +1257,8 @@ EXPORTED void mailbox_close(struct mailbox **mailboxptr) _delayed_cleanup, free, xstrdup(mailbox_name(mailbox))); } + /* drop the index lock */ + mailbox_unlock_index(mailbox, NULL); mailbox_release_resources(mailbox); mboxlist_entry_free(&mailbox->mbentry); @@ -1313,12 +1271,10 @@ EXPORTED void mailbox_close(struct mailbox **mailboxptr) xzfree(mailbox->h.flagname[flag]); } - if (listitem->l) mboxname_release(&listitem->l); - - if (mailbox->local_namespacelock) - mboxname_release(&mailbox->local_namespacelock); + mboxname_release(&mailbox->namelock); + mboxname_release(&mailbox->local_namespacelock); - remove_listitem(listitem); + remove_listitem(mailbox); } struct parseentry_rock { @@ -1992,11 +1948,6 @@ static int mailbox_read_index_header(struct mailbox *mailbox) if (!mailbox->index_base) return IMAP_MAILBOX_BADFORMAT; - /* need to make sure we're reading fresh data! */ - map_refresh(mailbox->index_fd, 1, &mailbox->index_base, - &mailbox->index_len, mailbox->index_size, - "index", mailbox_name(mailbox)); - r = mailbox_buf_to_index_header(mailbox->index_base, mailbox->index_len, &mailbox->i); if (r) return r; @@ -2271,6 +2222,7 @@ static int _commit_changes(struct mailbox *mailbox) int r; if (!mailbox->index_change_count) return 0; + assert(mailbox_index_islocked(mailbox, 1)); mailbox->i.dirty = 1; /* in which we throw away all our next pointers, but we don't care any more. @@ -2509,30 +2461,28 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) const char *index_fname = mailbox_meta_fname(mailbox, META_INDEX); assert(mailbox->index_fd != -1); - assert(!mailbox->index_locktype); + + // if we're already locked, don't need to lock again! + if (mailbox->index_locktype) { + if (mailbox->index_locktype == LOCK_SHARED && locktype != LOCK_SHARED) + return IMAP_MAILBOX_LOCKED; + return 0; + } char *userid = mboxname_to_userid(mailbox_name(mailbox)); if (userid) { if (!user_isnamespacelocked(userid)) { - struct mailboxlist *listitem = find_listitem(mailbox_name(mailbox)); - assert(listitem); - assert(&listitem->m == mailbox); - r = mailbox_mboxlock_reopen(listitem, LOCK_SHARED, locktype); - if (locktype == LOCK_SHARED) - mailbox->is_readonly = 1; - if (!r) r = mailbox_open_index(mailbox); + free(userid); + return mailbox_relock(mailbox, LOCK_SHARED, locktype); } free(userid); - if (r) return r; } if (locktype == LOCK_EXCLUSIVE) { - /* handle read-only case cleanly - we need to re-open read-write first! */ - if (mailbox->is_readonly) { - mailbox->is_readonly = 0; - r = mailbox_open_index(mailbox); - } - if (!r) r = lock_blocking(mailbox->index_fd, index_fname); + /* we can't upgrade a readonly mailbox */ + if (mailbox->is_readonly) + return IMAP_MAILBOX_LOCKED; + r = lock_blocking(mailbox->index_fd, index_fname); } else if (locktype == LOCK_SHARED) { r = lock_shared(mailbox->index_fd, index_fname); @@ -2542,18 +2492,25 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) fatal("invalid locktype for index", EX_SOFTWARE); } - /* double check that the index exists and has at least enough - * data to check the version number */ if (!r) { - if (!mailbox->index_base) + if (fstat(mailbox->index_fd, &sbuf) != 0) { r = IMAP_MAILBOX_BADFORMAT; - else if (mailbox->index_size < OFFSET_NUM_RECORDS) + } + else if (sbuf.st_size < OFFSET_NUM_RECORDS) { r = IMAP_MAILBOX_BADFORMAT; - if (r) - lock_unlock(mailbox->index_fd, index_fname); + } + else { + mailbox->index_ino = sbuf.st_ino; + mailbox->index_mtime = sbuf.st_mtime; + mailbox->index_size = sbuf.st_size; + map_refresh(mailbox->index_fd, 0, &mailbox->index_base, + &mailbox->index_len, mailbox->index_size, + "index", mailbox_name(mailbox)); + } } if (r) { + lock_unlock(mailbox->index_fd, index_fname); xsyslog(LOG_ERR, "IOERROR: lock index failed", "mailbox=<%s> error=<%s>", mailbox_name(mailbox), error_message(r)); @@ -2561,6 +2518,7 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) } mailbox->index_locktype = locktype; + mailbox->is_readonly = (locktype == LOCK_SHARED) ? 1 : 0; gettimeofday(&mailbox->starttime, 0); r = stat(header_fname, &sbuf); @@ -2584,14 +2542,6 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) } } - /* release caches */ - int i; - for (i = 0; i < mailbox->caches.count; i++) { - struct mappedfile *cachefile = ptrarray_nth(&mailbox->caches, i); - mappedfile_close(&cachefile); - } - ptrarray_fini(&mailbox->caches); - /* note: it's guaranteed by our outer cyrus.lock lock that the * cyrus.index and cyrus.cache files are never rewritten, so * we're safe to just extend the map if needed */ @@ -2645,6 +2595,9 @@ EXPORTED void mailbox_unlock_index(struct mailbox *mailbox, struct statusdata *s int r; const char *index_fname = mailbox_meta_fname(mailbox, META_INDEX); + if (mailbox->refcount > 1) + return; + /* this is kinda awful, but too much code expects it to work, and the * refcounting isn't good about partial commit/abort and all the * unwinding, so here you are. At least if you mailbox_abort, then @@ -2680,6 +2633,7 @@ EXPORTED void mailbox_unlock_index(struct mailbox *mailbox, struct statusdata *s "mailbox=<%s>", mailbox_name(mailbox)); mailbox->index_locktype = 0; + mailbox->is_readonly = 0; gettimeofday(&endtime, 0); timediff = timesub(&mailbox->starttime, &endtime); @@ -2697,10 +2651,16 @@ EXPORTED void mailbox_unlock_index(struct mailbox *mailbox, struct statusdata *s } } - // release the namespacelock here - if (mailbox->local_namespacelock) { - mboxname_release(&mailbox->local_namespacelock); + /* release caches */ + int i; + for (i = 0; i < mailbox->caches.count; i++) { + struct mappedfile *cachefile = ptrarray_nth(&mailbox->caches, i); + mappedfile_close(&cachefile); } + ptrarray_fini(&mailbox->caches); + + // release the namespacelock here + mboxname_release(&mailbox->local_namespacelock); } static char *mailbox_header_data_cstring(struct mailbox *mailbox) @@ -2997,6 +2957,9 @@ EXPORTED int mailbox_abort(struct mailbox *mailbox) { int r; + // we can't abort with additional references, just have to die + assert(mailbox->refcount == 1); + #ifdef WITH_DAV r = mailbox_abort_dav(mailbox); if (r) return r; @@ -3045,6 +3008,10 @@ EXPORTED int mailbox_abort(struct mailbox *mailbox) */ EXPORTED int mailbox_commit(struct mailbox *mailbox) { + /* open multiple times? we can't commit yet, so just skip committing */ + if (mailbox->refcount > 1) + return 0; + /* XXX - ibuf for alignment? */ static unsigned char buf[INDEX_HEADER_SIZE]; int n, r; @@ -3072,6 +3039,9 @@ EXPORTED int mailbox_commit(struct mailbox *mailbox) r = mailbox_commit_header(mailbox); if (r) return r; + r = _commit_changes(mailbox); + if (r) return r; + if (!mailbox->i.dirty) return 0; @@ -3081,11 +3051,6 @@ EXPORTED int mailbox_commit(struct mailbox *mailbox) mailbox_mbtype(mailbox), /*flags*/0); } - assert(mailbox_index_islocked(mailbox, 1)); - - r = _commit_changes(mailbox); - if (r) return r; - /* always update xconvmodseq, it might have been done by annotations */ r = mailbox_update_xconvmodseq(mailbox, mailbox->i.highestmodseq, /*force*/0); if (r) return r; @@ -4349,17 +4314,12 @@ EXPORTED struct conversations_state *mailbox_get_cstate_full(struct mailbox *mai /* already exists, use that one */ struct conversations_state *cstate = conversations_get_mbox(mailbox_name(mailbox)); - if (cstate) { - /* but make sure it's not read-only unless we are */ - if (!mailbox->is_readonly) assert (!cstate->is_shared); - return cstate; - } + if (cstate) return cstate; /* open the conversations DB - don't bother checking return code since it'll * only be set if it opens successfully, and we can only return NULL or an * object */ conversations_open_mbox(mailbox_name(mailbox), mailbox->is_readonly, &mailbox->local_cstate); - return mailbox->local_cstate; } @@ -5830,31 +5790,28 @@ EXPORTED int mailbox_create(const char *name, struct mailbox *mailbox = NULL; int n; int createfnames[] = { META_INDEX, META_HEADER, 0 }; - struct mailboxlist *listitem; assert(uniqueid); /* if we already have this name open then that's an error too */ - listitem = find_listitem(name); - if (listitem) return IMAP_MAILBOX_LOCKED; + uint32_t legacy_dirs = (mbtype & MBTYPE_LEGACY_DIRS); + mailbox = find_listitem(legacy_dirs ? name : uniqueid); + if (mailbox) return IMAP_MAILBOX_LOCKED; - listitem = create_listitem(name); - mailbox = &listitem->m; + mailbox = create_listitem(); /* needs to be an exclusive namelock to create a mailbox */ char *userid = mboxname_to_userid(name); - if (userid) { - int haslock = user_isnamespacelocked(userid); - assert(haslock == LOCK_EXCLUSIVE); - free(userid); - } + int haslock = user_isnamespacelocked(userid); + syslog(LOG_ERR, "CREATING MAILBOX %s", name); + assert(haslock == LOCK_EXCLUSIVE); + free(userid); - uint32_t legacy_dirs = (mbtype & MBTYPE_LEGACY_DIRS); - r = mboxname_lock(legacy_dirs ? name : uniqueid, &listitem->l, LOCK_EXCLUSIVE); + mailbox->lockname = xstrdup(legacy_dirs ? name : uniqueid); + r = mboxname_lock(mailbox->lockname, &mailbox->namelock, LOCK_EXCLUSIVE); if (r) { - if (mailbox->local_namespacelock) - mboxname_release(&mailbox->local_namespacelock); - remove_listitem(listitem); + mboxname_release(&mailbox->local_namespacelock); + remove_listitem(mailbox); return r; } @@ -6674,22 +6631,10 @@ HIDDEN int mailbox_rename_copy(struct mailbox *oldmailbox, NULL : mailbox_uniqueid(newmailbox)); if (r) goto fail; - /* Re-open index file */ - r = mailbox_open_index(newmailbox); + /* we have new files in place, so redo all the locks */ + r = mailbox_relock(newmailbox, LOCK_EXCLUSIVE, LOCK_EXCLUSIVE); if (r) goto fail; - /* cyrus.header has been copied with old uniqueid. - make a copy of new uniqueid so we can reset it */ - newuniqueid = xstrdup(mailbox_uniqueid(newmailbox)); - - /* Re-lock index */ - r = mailbox_lock_index_internal(newmailbox, LOCK_EXCLUSIVE); - - /* Reset new uniqueid */ - free(newmailbox->h.uniqueid); - newmailbox->h.uniqueid = xstrdup(newuniqueid); - newmailbox->header_dirty = 1; - /* update mailbox annotations if necessary */ r = annotate_rename_mailbox(oldmailbox, newmailbox); if (r) goto fail; @@ -7027,16 +6972,14 @@ static int mailbox_reconstruct_create(const char *name, struct mailbox **mbptr) int options = config_getint(IMAPOPT_MAILBOX_DEFAULT_OPTIONS) | OPT_POP3_NEW_UIDL; mbentry_t *mbentry = NULL; - struct mailboxlist *listitem; int r; /* make sure it's not already open. Very odd, since we already * discovered it's not openable! */ - listitem = find_listitem(name); - if (listitem) return IMAP_MAILBOX_LOCKED; + mailbox = find_listitem(name); + if (mailbox) return IMAP_MAILBOX_LOCKED; - listitem = create_listitem(name); - mailbox = &listitem->m; + mailbox = create_listitem(); // lock the user namespace FIRST before the mailbox namespace char *userid = mboxname_to_userid(name); @@ -7060,7 +7003,7 @@ static int mailbox_reconstruct_create(const char *name, struct mailbox **mbptr) /* if we can't get an exclusive lock first try, there's something * racy going on! */ uint32_t legacy_dirs = (mbentry->mbtype & MBTYPE_LEGACY_DIRS); - r = mboxname_lock(legacy_dirs ? name : mbentry->uniqueid, &listitem->l, LOCK_EXCLUSIVE); + r = mboxname_lock(legacy_dirs ? name : mbentry->uniqueid, &mailbox->namelock, LOCK_EXCLUSIVE); if (r) goto done; mailbox->mbentry = mboxlist_entry_copy(mbentry); @@ -7068,7 +7011,8 @@ static int mailbox_reconstruct_create(const char *name, struct mailbox **mbptr) syslog(LOG_NOTICE, "create new mailbox %s", name); /* Attempt to open index */ - r = mailbox_open_index(mailbox); + r = mailbox_open_index(mailbox, LOCK_EXCLUSIVE); + if (!r) r = mailbox_lock_index_internal(mailbox, LOCK_EXCLUSIVE); if (!r) r = mailbox_read_index_header(mailbox); if (r) { printf("%s: failed to read index header\n", mailbox_name(mailbox)); diff --git a/imap/mailbox.h b/imap/mailbox.h index 76191668ae..5f6128ac3f 100644 --- a/imap/mailbox.h +++ b/imap/mailbox.h @@ -257,7 +257,7 @@ struct mailbox { size_t index_len; /* mapped size */ int index_locktype; /* 0 = none, 1 = shared, 2 = exclusive */ - int is_readonly; /* true = open index and cache files readonly */ + int is_readonly; /* this matches locktype but COULD be true even if locktype == exclusive */ ino_t header_file_ino; bit32 header_file_crc; @@ -310,6 +310,12 @@ struct mailbox { struct index_change *index_changes; uint32_t index_change_alloc; uint32_t index_change_count; + + /* refcounting */ + int refcount; + char *lockname; + struct mboxlock *namelock; + struct mailbox *next; }; #define ITER_SKIP_UNLINKED (1<<0) From c5851b6cdb42e43be7ec95a434eb8d112032b1d9 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Sun, 5 Nov 2023 14:55:24 +0100 Subject: [PATCH 08/38] jmap: remove separate mailbox caching layer This is now supported by lower level open_mailboxes list. --- imap/jmap_api.c | 87 +++---------------------------------------------- imap/jmap_api.h | 6 ++-- 2 files changed, 6 insertions(+), 87 deletions(-) diff --git a/imap/jmap_api.c b/imap/jmap_api.c index ae58c8265e..7fc71565c0 100644 --- a/imap/jmap_api.c +++ b/imap/jmap_api.c @@ -388,36 +388,12 @@ HIDDEN int jmap_error_response(struct transaction_t *txn, HIDDEN int jmap_initreq(jmap_req_t *req) { memset(req, 0, sizeof(struct jmap_req)); - req->mboxes = ptrarray_new(); return 0; } -struct _mboxcache_rec { - struct mailbox *mbox; - int refcount; - int rw; -}; - HIDDEN void jmap_finireq(jmap_req_t *req) { - int i; - - for (i = 0; i < req->mboxes->count; i++) { - struct _mboxcache_rec *rec = ptrarray_nth(req->mboxes, i); - syslog(LOG_ERR, "jmap: force-closing mailbox %s (refcount=%d)", - mailbox_name(rec->mbox), rec->refcount); - mailbox_close(&rec->mbox); - free(rec); - } - /* Fail after cleaning up open mailboxes */ - if (req->mboxes->count) { - json_t *jdebug = json_pack("[s,s,s,o,o]", req->method, req->userid, req->accountid, req->args, req->response); - char *debug = json_dumps(jdebug, JSON_INDENT(2)); - assert(!debug); - } - - ptrarray_free(req->mboxes); - req->mboxes = NULL; + assert(!open_mailboxes_exist()); jmap_mbentry_cache_free(req); @@ -1080,25 +1056,7 @@ void jmap_add_id(jmap_req_t *req, const char *creation_id, const char *id) HIDDEN int jmap_openmbox(jmap_req_t *req, const char *name, struct mailbox **mboxp, int rw) { - int i, r; - struct _mboxcache_rec *rec; - - for (i = 0; i < req->mboxes->count; i++) { - rec = (struct _mboxcache_rec*) ptrarray_nth(req->mboxes, i); - if (!strcmp(name, mailbox_name(rec->mbox))) { - if (rw && !rec->rw) { - /* Lock promotions are not supported */ - syslog(LOG_ERR, "jmapmbox: failed to grab write-lock" - " on cached read-only mailbox %s", name); - return IMAP_INTERNAL; - } - /* Found a cached mailbox. Increment refcount. */ - rec->refcount++; - *mboxp = rec->mbox; - - return 0; - } - } + int r; /* Add mailbox to cache */ if (req->force_openmbox_rw) @@ -1108,11 +1066,6 @@ HIDDEN int jmap_openmbox(jmap_req_t *req, const char *name, syslog(LOG_ERR, "jmap_openmbox(%s): %s", name, error_message(r)); return r; } - rec = xzmalloc(sizeof(struct _mboxcache_rec)); - rec->mbox = *mboxp; - rec->refcount = 1; - rec->rw = rw; - ptrarray_add(req->mboxes, rec); return 0; } @@ -1128,41 +1081,9 @@ HIDDEN int jmap_openmbox_by_uniqueid(jmap_req_t *req, const char *id, return IMAP_MAILBOX_NONEXISTENT; } -HIDDEN int jmap_isopenmbox(jmap_req_t *req, const char *name) +HIDDEN void jmap_closembox(jmap_req_t *req __attribute__((unused)), struct mailbox **mboxp) { - - int i; - struct _mboxcache_rec *rec; - - for (i = 0; i < req->mboxes->count; i++) { - rec = (struct _mboxcache_rec*) ptrarray_nth(req->mboxes, i); - if (!strcmp(name, mailbox_name(rec->mbox))) - return 1; - } - - return 0; -} - -HIDDEN void jmap_closembox(jmap_req_t *req, struct mailbox **mboxp) -{ - struct _mboxcache_rec *rec = NULL; - int i; - - if (mboxp == NULL || *mboxp == NULL) return; - - for (i = 0; i < req->mboxes->count; i++) { - rec = (struct _mboxcache_rec*) ptrarray_nth(req->mboxes, i); - if (rec->mbox == *mboxp) { - if (!(--rec->refcount)) { - ptrarray_remove(req->mboxes, i); - mailbox_close(&rec->mbox); - free(rec); - } - *mboxp = NULL; - return; - } - } - syslog(LOG_INFO, "jmap: ignoring non-cached mailbox %s", mailbox_name(*mboxp)); + mailbox_close(mboxp); } struct findblob_data { diff --git a/imap/jmap_api.h b/imap/jmap_api.h index 8c1b11be39..670d81e6bc 100644 --- a/imap/jmap_api.h +++ b/imap/jmap_api.h @@ -165,7 +165,6 @@ typedef struct jmap_req { int force_openmbox_rw; /* Internal state */ - ptrarray_t *mboxes; hash_table *mbstates; hash_table *created_ids; hash_table *inmemory_blobs; @@ -278,11 +277,10 @@ extern void jmap_admin_capabilities(json_t *account_capabilities); extern void jmap_accounts(json_t *accounts, json_t *primary_accounts); /* Request-scoped mailbox cache */ -extern int jmap_openmbox(jmap_req_t *req, const char *name, - struct mailbox **mboxp, int rw); +extern int jmap_openmbox(jmap_req_t *req, const char *name, + struct mailbox **mboxp, int rw); extern int jmap_openmbox_by_uniqueid(jmap_req_t *req, const char *id, struct mailbox **mboxp, int rw); -extern int jmap_isopenmbox(jmap_req_t *req, const char *name); extern void jmap_closembox(jmap_req_t *req, struct mailbox **mboxp); extern int jmap_mboxlist_lookup(const char *name, From 0ab4408e135909fc175103031ac3fb4596b7b539 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 1 Nov 2023 07:31:56 -0400 Subject: [PATCH 09/38] subscriptions: bump the modseq on the folder if subscriptions change includes silent mode, since you can't bump on replicas! --- imap/autocreate.c | 6 +++--- imap/http_dav.c | 2 +- imap/http_dav_sharing.c | 2 +- imap/imapd.c | 8 ++++---- imap/jmap_api.c | 2 +- imap/jmap_calendar.c | 4 ++-- imap/jmap_contact.c | 4 ++-- imap/jmap_mailbox.c | 6 +++--- imap/lmtp_sieve.c | 2 +- imap/mboxlist.c | 36 ++++++++++++++++++++++++------------ imap/mboxlist.h | 2 +- imap/sync_reset.c | 2 +- imap/sync_support.c | 4 ++-- imap/user.c | 2 +- 14 files changed, 47 insertions(+), 35 deletions(-) diff --git a/imap/autocreate.c b/imap/autocreate.c index 1f057d2bac..4f663b62f4 100644 --- a/imap/autocreate.c +++ b/imap/autocreate.c @@ -529,7 +529,7 @@ static int autochangesub(struct findall_data *data, void *rock) /* ignore all user mailboxes, we only want shared */ if (mboxname_isusermailbox(name, 0)) return 0; - r = mboxlist_changesub(name, userid, auth_state, 1, 0, 1); + r = mboxlist_changesub(name, userid, auth_state, 1, 0, 1, 1); /* unless this name was explicitly chosen, ignore the failure */ if (!was_explicit) return 0; @@ -777,7 +777,7 @@ int autocreate_user(struct namespace *namespace, const char *userid) 1/*isadmin*/, userid, auth_state, MBOXLIST_CREATE_NOTIFY, NULL/*mailboxptr*/); - if (!r) r = mboxlist_changesub(inboxname, userid, auth_state, 1, 1, 1); + if (!r) r = mboxlist_changesub(inboxname, userid, auth_state, 1, 1, 1, 1); if (r) { syslog(LOG_ERR, "autocreateinbox: User %s, INBOX failed. %s", userid, error_message(r)); @@ -839,7 +839,7 @@ int autocreate_user(struct namespace *namespace, const char *userid) /* subscribe if requested */ if (strarray_find(subscribe, name, 0) >= 0) { - r = mboxlist_changesub(foldername, userid, auth_state, 1, 1, 1); + r = mboxlist_changesub(foldername, userid, auth_state, 1, 1, 1, 1); if (!r) { numsub++; syslog(LOG_NOTICE,"autocreateinbox: User %s, subscription to %s succeeded", diff --git a/imap/http_dav.c b/imap/http_dav.c index cd633ea5de..922e5d5f5d 100644 --- a/imap/http_dav.c +++ b/imap/http_dav.c @@ -4943,7 +4943,7 @@ static int meth_delete_collection(struct transaction_t *txn, /* Unsubscribe */ r = mboxlist_changesub(txn->req_tgt.mbentry->name, txn->req_tgt.userid, - httpd_authstate, 0 /* remove */, 0, 0); + httpd_authstate, 0 /* remove */, 0, 0, 0); if (r) { syslog(LOG_ERR, "mboxlist_changesub(%s, %s) failed: %s", txn->req_tgt.mbentry->name, txn->req_tgt.userid, diff --git a/imap/http_dav_sharing.c b/imap/http_dav_sharing.c index 26fe7f4e6a..d5f4c6b413 100644 --- a/imap/http_dav_sharing.c +++ b/imap/http_dav_sharing.c @@ -1090,7 +1090,7 @@ HIDDEN int notify_post(struct transaction_t *txn) /* [Un]subscribe */ r = mboxlist_changesub(mboxname, txn->req_tgt.userid, - httpd_authstate, add, 0, 0); + httpd_authstate, add, 0, 0, 0); if (r) { syslog(LOG_ERR, "mboxlist_changesub(%s, %s) failed: %s", mboxname, txn->req_tgt.userid, error_message(r)); diff --git a/imap/imapd.c b/imap/imapd.c index 59b644c403..28f570080c 100644 --- a/imap/imapd.c +++ b/imap/imapd.c @@ -8187,7 +8187,7 @@ static void cmd_delete(char *tag, char *name, int localonly, int force) if (!r && config_getswitch(IMAPOPT_DELETE_UNSUBSCRIBE)) { mboxlist_changesub(mbname_intname(mbname), imapd_userid, imapd_authstate, - /* add */ 0, /* force */ 0, /* notify? */ 1); + /* add */ 0, /* force */ 1, /* notify? */ 0, /*silent*/1); } mboxname_release(&namespacelock); @@ -8292,7 +8292,7 @@ static int renmbox(const mbentry_t *mbentry, void *rock) if (!r && config_getswitch(IMAPOPT_DELETE_UNSUBSCRIBE)) { mboxlist_changesub(mbentry->name, imapd_userid, imapd_authstate, - /* add */ 0, /* force */ 0, /* notify? */ 0); + /* add */ 0, /* force */ 1, /* notify? */ 0, /*silent*/1); } oldextname = @@ -8733,7 +8733,7 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, if (!r && config_getswitch(IMAPOPT_DELETE_UNSUBSCRIBE)) { mboxlist_changesub(oldmailboxname, imapd_userid, imapd_authstate, - /* add */ 0, /* force */ 0, /* notify? */ 1); + /* add */ 0, /* force */ 1, /* notify? */ 0, /*silent*/1); } } @@ -9209,7 +9209,7 @@ static void cmd_changesub(char *tag, char *namespace, char *name, int add) } else { char *intname = mboxname_from_external(name, &imapd_namespace, imapd_userid); - r = mboxlist_changesub(intname, imapd_userid, imapd_authstate, add, force, 1); + r = mboxlist_changesub(intname, imapd_userid, imapd_authstate, add, force, 1, 0); free(intname); } } diff --git a/imap/jmap_api.c b/imap/jmap_api.c index 7fc71565c0..90ccfc0867 100644 --- a/imap/jmap_api.c +++ b/imap/jmap_api.c @@ -2725,7 +2725,7 @@ static void send_dav_invite(const char *userid, void *val, void *rock) if (!old || !new) { /* Change subscription */ r = mboxlist_changesub(irock->mboxname, userid, httpd_authstate, - access != SHARE_NONE, 0, /*notify*/1); + access != SHARE_NONE, 0, /*notify*/1, /*silent*/0); } if (!r) { diff --git a/imap/jmap_calendar.c b/imap/jmap_calendar.c index bab816946d..fcd1e7ad7d 100644 --- a/imap/jmap_calendar.c +++ b/imap/jmap_calendar.c @@ -1653,7 +1653,7 @@ static int setcalendar_writeprops(jmap_req_t *req, if (!r && props->isSubscribed >= 0) { /* Update subscription database */ r = mboxlist_changesub(mailbox_name(mbox), req->userid, req->authstate, - props->isSubscribed, 0, /*notify*/1); + props->isSubscribed, 0, /*notify*/1, /*silent*/0); /* Set invite status for CalDAV */ buf_setcstr(&val, props->isSubscribed ? "invite-accepted" : "invite-declined"); @@ -1858,7 +1858,7 @@ static void setcalendars_destroy(jmap_req_t *req, const char *calid, jmap_myrights_delete(req, mboxname); /* Remove from subscriptions db */ - mboxlist_changesub(mboxname, req->userid, req->authstate, 0, 1, 0); + mboxlist_changesub(mboxname, req->userid, req->authstate, 0, 1, 0, 1); struct mboxevent *mboxevent = mboxevent_new(EVENT_MAILBOX_DELETE); if (mboxlist_delayed_delete_isenabled()) { diff --git a/imap/jmap_contact.c b/imap/jmap_contact.c index 5243daecc5..3f0911098c 100644 --- a/imap/jmap_contact.c +++ b/imap/jmap_contact.c @@ -5111,7 +5111,7 @@ static int setaddressbook_writeprops(jmap_req_t *req, if (!r && props->isSubscribed >= 0) { /* Update subscription database */ r = mboxlist_changesub(mboxname, req->userid, req->authstate, - props->isSubscribed, 0, /*notify*/1); + props->isSubscribed, 0, /*notify*/1, /*silent*/0); /* Set invite status for CalDAV */ buf_setcstr(&val, props->isSubscribed ? "invite-accepted" : "invite-declined"); @@ -5207,7 +5207,7 @@ static void setaddressbooks_destroy(jmap_req_t *req, const char *abookid, jmap_myrights_delete(req, mboxname); /* Remove from subscriptions db */ - mboxlist_changesub(mboxname, req->userid, req->authstate, 0, 1, 0); + mboxlist_changesub(mboxname, req->userid, req->authstate, 0, 1, 0, 1); struct mboxevent *mboxevent = mboxevent_new(EVENT_MAILBOX_DELETE); if (mboxlist_delayed_delete_isenabled()) { diff --git a/imap/jmap_mailbox.c b/imap/jmap_mailbox.c index abaecdea1e..d1b9fb31ec 100644 --- a/imap/jmap_mailbox.c +++ b/imap/jmap_mailbox.c @@ -2335,7 +2335,7 @@ static void _mbox_create(jmap_req_t *req, struct mboxset_args *args, /* Write annotations and isSubscribed */ r = _mbox_set_annots(req, args, mboxname); if (!r && args->is_subscribed > 0) { - r = mboxlist_changesub(mboxname, req->userid, httpd_authstate, 1, 0, 0); + r = mboxlist_changesub(mboxname, req->userid, httpd_authstate, 1, 0, 0, 0); } if (r) goto done; @@ -2739,7 +2739,7 @@ static void _mbox_update(jmap_req_t *req, struct mboxset_args *args, if (!r && args->is_subscribed >= 0) { r = mboxlist_changesub(mboxname, req->userid, httpd_authstate, - args->is_subscribed, 0, 0); + args->is_subscribed, 0, 0, 0); } if (!r && (args->shareWith || args->is_seenshared >= 0)) { struct mailbox *mbox = NULL; @@ -3033,7 +3033,7 @@ static void _mbox_destroy(jmap_req_t *req, const char *mboxid, mbentry->uniqueid, msgcount); /* Remove subscription */ - int r2 = mboxlist_changesub(mbentry->name, req->userid, httpd_authstate, 0, 0, 0); + int r2 = mboxlist_changesub(mbentry->name, req->userid, httpd_authstate, 0, 1, 0, 1); if (r2) { syslog(LOG_ERR, "jmap: mbox_destroy: can't unsubscribe %s:%s", mbentry->name, error_message(r2)); diff --git a/imap/lmtp_sieve.c b/imap/lmtp_sieve.c index f13a266293..f2e50cc759 100644 --- a/imap/lmtp_sieve.c +++ b/imap/lmtp_sieve.c @@ -2477,7 +2477,7 @@ static int autosieve_createfolder(const char *userid, const struct auth_state *a goto done; } - mboxlist_changesub(internalname, userid, auth_state, 1, 1, 1); + mboxlist_changesub(internalname, userid, auth_state, 1, 1, 1, 1); syslog(LOG_DEBUG, "autosievefolder: User %s, folder %s creation succeeded", userid, internalname); diff --git a/imap/mboxlist.c b/imap/mboxlist.c index 3d44e306a5..bc85a8cdcf 100644 --- a/imap/mboxlist.c +++ b/imap/mboxlist.c @@ -2897,13 +2897,13 @@ EXPORTED int mboxlist_renamemailbox(const mbentry_t *mbentry, /* Move subscription */ if (move_subscription) { int is_subscribed = mboxlist_checksub(oldname, userid) == 0; - int r2 = mboxlist_changesub(oldname, userid, auth_state, 0, 0, 0); + int r2 = mboxlist_changesub(oldname, userid, auth_state, 0, 0, 0, silent); if (r2) { syslog(LOG_ERR, "CHANGESUB: can't unsubscribe %s: %s", oldname, error_message(r2)); } if (is_subscribed) { - r2 = mboxlist_changesub(newname, userid, auth_state, 1, 0, 0); + r2 = mboxlist_changesub(newname, userid, auth_state, 1, 0, 0, silent); if (r2) { syslog(LOG_ERR, "CHANGESUB: can't subscribe %s: %s", newname, error_message(r2)); @@ -5134,13 +5134,12 @@ EXPORTED int mboxlist_checksub(const char *name, const char *userid) */ EXPORTED int mboxlist_changesub(const char *name, const char *userid, const struct auth_state *auth_state, - int add, int force, int notify) + int add, int force, int notify, int silent) { struct buf key = BUF_INITIALIZER; mbentry_t *mbentry = NULL; int r; struct db *subs; - struct mboxevent *mboxevent; init_internal(); @@ -5150,15 +5149,12 @@ EXPORTED int mboxlist_changesub(const char *name, const char *userid, char *dbname = mboxname_to_dbname(name); + mboxlist_mylookup(dbname, &mbentry, NULL, 0, 0); + if (add && !force) { /* Ensure mailbox exists and can be seen by user */ - if ((r = mboxlist_mylookup(dbname, &mbentry, NULL, 0, 0))!=0) { + if (!mbentry || (cyrus_acl_myrights(auth_state, mbentry->acl) & ACL_LOOKUP) == 0) { mboxlist_closesubs(subs); - goto done; - } - if ((cyrus_acl_myrights(auth_state, mbentry->acl) & ACL_LOOKUP) == 0) { - mboxlist_closesubs(subs); - mboxlist_entry_free(&mbentry); r = IMAP_MAILBOX_NONEXISTENT; goto done; } @@ -5185,11 +5181,26 @@ EXPORTED int mboxlist_changesub(const char *name, const char *userid, sync_log_subscribe(userid, name); mboxlist_closesubs(subs); - mboxlist_entry_free(&mbentry); buf_free(&key); + if (r) goto done; + + // bump the modseq on the folder if one exists + if (!silent && mbentry && !(mbentry->mbtype & MBTYPE_REMOTE)) { + struct mailbox *mailbox = NULL; + r = mailbox_open_iwl(name, &mailbox); + if (!r) { + mailbox_modseq_dirty(mailbox); + mboxlist_update_foldermodseq(name, mailbox->i.highestmodseq); + r = mailbox_commit(mailbox); + mailbox_close(&mailbox); + } + if (r) goto done; + } + /* prepare a MailboxSubscribe or MailboxUnSubscribe event notification */ - if (notify && r == 0) { + if (notify) { + struct mboxevent *mboxevent; mboxevent = mboxevent_new(add ? EVENT_MAILBOX_SUBSCRIBE : EVENT_MAILBOX_UNSUBSCRIBE); @@ -5199,6 +5210,7 @@ EXPORTED int mboxlist_changesub(const char *name, const char *userid, } done: + mboxlist_entry_free(&mbentry); free(dbname); return r; } diff --git a/imap/mboxlist.h b/imap/mboxlist.h index 58fd2cbd9f..dca7712d0d 100644 --- a/imap/mboxlist.h +++ b/imap/mboxlist.h @@ -387,7 +387,7 @@ int mboxlist_checksub(const char *name, const char *userid); /* Change 'user's subscription status for mailbox 'name'. */ int mboxlist_changesub(const char *name, const char *userid, const struct auth_state *auth_state, - int add, int force, int notify); + int add, int force, int notify, int silent); /* set or create quota root */ int mboxlist_setquotas(const char *root, diff --git a/imap/sync_reset.c b/imap/sync_reset.c index 9790490e0d..950747f8f1 100644 --- a/imap/sync_reset.c +++ b/imap/sync_reset.c @@ -140,7 +140,7 @@ static int reset_single(const char *userid) /* ignore failures here - the subs file gets deleted soon anyway */ for (i = sublist->count; i; i--) { const char *name = strarray_nth(sublist, i-1); - (void)mboxlist_changesub(name, userid, sync_authstate, 0, 0, 0); + (void)mboxlist_changesub(name, userid, sync_authstate, 0, 0, 0, /*silent*/1); } mbentry_t *mbentry = NULL; diff --git a/imap/sync_support.c b/imap/sync_support.c index c3919bac2f..22a00384c2 100644 --- a/imap/sync_support.c +++ b/imap/sync_support.c @@ -3729,7 +3729,7 @@ int sync_apply_changesub(struct dlist *kin, struct sync_state *sstate) if (!dlist_getatom(kin, "USERID", &userid)) return IMAP_PROTOCOL_BAD_PARAMETERS; - return mboxlist_changesub(mboxname, userid, sstate->authstate, add, add, 0); + return mboxlist_changesub(mboxname, userid, sstate->authstate, add, add, 0, /*silent*/1); } /* ====================================================================== */ @@ -3974,7 +3974,7 @@ int sync_apply_unuser(struct dlist *kin, struct sync_state *sstate) strarray_t *list = mboxlist_sublist(userid); for (i = 0; i < list->count; i++) { const char *name = strarray_nth(list, i); - mboxlist_changesub(name, userid, sstate->authstate, 0, 0, 0); + mboxlist_changesub(name, userid, sstate->authstate, 0, 0, 0, /*silent*/1); } mbentry_t *mbentry = NULL; diff --git a/imap/user.c b/imap/user.c index f31bdaa6d9..ae4c9d8d41 100644 --- a/imap/user.c +++ b/imap/user.c @@ -379,7 +379,7 @@ static int user_renamesub(const char *name, void* rock) name = newname; } - return mboxlist_changesub(name, rrock->newuser, NULL, 1, 1, 1); + return mboxlist_changesub(name, rrock->newuser, NULL, 1, 1, 1, /*silent*/1); } static int user_renamesieve(const char *olduser, const char *newuser) From 3a79090a4dbff1d16392de0ce3dfa73c16f2cfbf Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 1 Nov 2023 07:36:58 -0400 Subject: [PATCH 10/38] JMAPMailbox: test that subscribing bumps state --- cassandane/Cassandane/Cyrus/JMAPMailbox.pm | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cassandane/Cassandane/Cyrus/JMAPMailbox.pm b/cassandane/Cassandane/Cyrus/JMAPMailbox.pm index 5fd7d8c7b2..552aec2c16 100644 --- a/cassandane/Cassandane/Cyrus/JMAPMailbox.pm +++ b/cassandane/Cassandane/Cyrus/JMAPMailbox.pm @@ -946,6 +946,15 @@ sub test_mailbox_query_filteroperator ]); $self->assert(exists $res->[0][1]{updated}{$mboxids{'Ham'}}); + xlog $self, "make sure subscribing changed state"; + $self->assert_not_equals($res->[0][1]{oldState}, $res->[0][1]{newState}); + + my $state = $res->[0][1]{oldState}; + $res = $jmap->CallMethods([['Mailbox/changes', { sinceState => $state }, "R1"]]); + $self->assert_num_equals(1, scalar @{$res->[0][1]{updated}}); + $self->assert_equals($res->[0][1]{updated}[0], $mboxids{'Ham'}); + $self->assert_null($res->[0][1]{updatedProperties}); + xlog $self, "list mailboxes filtered by parentId OR role"; $res = $jmap->CallMethods([['Mailbox/query', { filter => { From ce6b09741c4c59eb97319cd3ca7291fc6932b261 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Sun, 5 Nov 2023 19:53:38 +0100 Subject: [PATCH 11/38] Replication: with uuid based paths, replica subs might not be findable --- cassandane/Cassandane/Cyrus/Replication.pm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cassandane/Cassandane/Cyrus/Replication.pm b/cassandane/Cassandane/Cyrus/Replication.pm index bc01b0db59..d67dffca8d 100644 --- a/cassandane/Cassandane/Cyrus/Replication.pm +++ b/cassandane/Cassandane/Cyrus/Replication.pm @@ -1529,6 +1529,7 @@ sub assert_user_sub_exists my ($self, $instance, $user) = @_; my $subs = $instance->get_conf_user_file($user, 'sub'); + $self->assert_not_null($subs); xlog $self, "Looking for subscriptions file $subs"; @@ -1540,6 +1541,7 @@ sub assert_user_sub_not_exists my ($self, $instance, $user) = @_; my $subs = $instance->get_conf_user_file($user, 'sub'); + return unless $subs; # user might not exist xlog $self, "Looking for subscriptions file $subs"; From 8c54b403febd9f92ff7f486229e5946f4289a1ff Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 14:47:01 +0100 Subject: [PATCH 12/38] replace jmap_closembox() - it's just mailbox_close() now --- imap/http_jmap.c | 2 +- imap/jmap_api.c | 9 +---- imap/jmap_api.h | 2 - imap/jmap_backup.c | 19 +++++---- imap/jmap_calendar.c | 80 +++++++++++++++++-------------------- imap/jmap_contact.c | 30 +++++++------- imap/jmap_core.c | 4 +- imap/jmap_ical.c | 7 ++-- imap/jmap_mail.c | 52 ++++++++++++------------ imap/jmap_mail_submission.c | 12 +++--- imap/jmap_mailbox.c | 8 ++-- imap/jmap_mdn.c | 4 +- imap/jmap_notes.c | 6 +-- imap/jmap_sieve.c | 6 +-- 14 files changed, 113 insertions(+), 128 deletions(-) diff --git a/imap/http_jmap.c b/imap/http_jmap.c index 5af4e5da2a..09c18fc424 100644 --- a/imap/http_jmap.c +++ b/imap/http_jmap.c @@ -630,7 +630,7 @@ static int jmap_getblob_default_handler(jmap_req_t *req, } done: - if (mbox) jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (body) { message_free_body(body); free(body); diff --git a/imap/jmap_api.c b/imap/jmap_api.c index 90ccfc0867..f4362f74f6 100644 --- a/imap/jmap_api.c +++ b/imap/jmap_api.c @@ -1081,11 +1081,6 @@ HIDDEN int jmap_openmbox_by_uniqueid(jmap_req_t *req, const char *id, return IMAP_MAILBOX_NONEXISTENT; } -HIDDEN void jmap_closembox(jmap_req_t *req __attribute__((unused)), struct mailbox **mboxp) -{ - mailbox_close(mboxp); -} - struct findblob_data { jmap_req_t *req; const char *from_accountid; @@ -1129,7 +1124,7 @@ static int findblob_cb(const conv_guidrec_t *rec, void *rock) r = msgrecord_find(d->mbox, rec->uid, &d->mr); if (r) { - jmap_closembox(req, &d->mbox); + mailbox_close(&d->mbox); d->mr = NULL; return r; } @@ -1247,7 +1242,7 @@ static int _jmap_findblob(jmap_req_t *req, const char *from_accountid, conversations_commit(&mycstate); } if (r) { - if (data.mbox) jmap_closembox(req, &data.mbox); + mailbox_close(&data.mbox); if (mybody) { message_free_body(mybody); free(mybody); diff --git a/imap/jmap_api.h b/imap/jmap_api.h index 670d81e6bc..1564d30f4d 100644 --- a/imap/jmap_api.h +++ b/imap/jmap_api.h @@ -281,8 +281,6 @@ extern int jmap_openmbox(jmap_req_t *req, const char *name, struct mailbox **mboxp, int rw); extern int jmap_openmbox_by_uniqueid(jmap_req_t *req, const char *id, struct mailbox **mboxp, int rw); -extern void jmap_closembox(jmap_req_t *req, struct mailbox **mboxp); - extern int jmap_mboxlist_lookup(const char *name, mbentry_t **entryptr, struct txn **tid); diff --git a/imap/jmap_backup.c b/imap/jmap_backup.c index 1b10139b2e..a2ac43bd62 100644 --- a/imap/jmap_backup.c +++ b/imap/jmap_backup.c @@ -389,7 +389,7 @@ static int restore_collection_cb(const mbentry_t *mbentry, void *rock) mailbox_name(rrock->mailbox), rrock->jrestore->cutoff, rrock->mailbox->i.changes_epoch); - jmap_closembox(rrock->req, &rrock->mailbox); + mailbox_close(&rrock->mailbox); return HTTP_UNPROCESSABLE; } @@ -497,7 +497,7 @@ static int restore_collection_cb(const mbentry_t *mbentry, void *rock) /* Close and re-open mailbox (to avoid deadlocks). We also do this on initial entry into the loop to switch from a read lock to a write lock. */ - jmap_closembox(rrock->req, &rrock->mailbox); + mailbox_close(&rrock->mailbox); r = jmap_openmbox(rrock->req, mbentry->name, &rrock->mailbox, /*rw*/1); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s for writing", @@ -517,7 +517,7 @@ static int restore_collection_cb(const mbentry_t *mbentry, void *rock) rrock->deletedmodseq = rrock->mailbox->i.deletedmodseq; if (!rrock->keep_open) - jmap_closembox(rrock->req, &rrock->mailbox); + mailbox_close(&rrock->mailbox); return r; } @@ -881,7 +881,7 @@ static int restore_addressbook_cb(const mbentry_t *mbentry, void *rock) #endif crock->group_vcard = NULL; - jmap_closembox(rrock->req, mailboxp); + mailbox_close(mailboxp); return r; } @@ -1308,7 +1308,7 @@ static int recreate_ical_resources(const mbentry_t *mbentry, message_get_uid((message_t *) nextmsg, &nextuid); mailbox_iter_done(&iter); - jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); r = jmap_openmbox(req, mbentry->name, &mailbox, /*rw*/1); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s for writing", @@ -1322,7 +1322,7 @@ static int recreate_ical_resources(const mbentry_t *mbentry, } mailbox_iter_done(&iter); - jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); return 0; } @@ -1781,7 +1781,7 @@ static int restore_message_list_cb(const mbentry_t *mbentry, void *rock) } mailbox_iter_done(&iter); - jmap_closembox(rrock->req, &mailbox); + mailbox_close(&mailbox); return 0; } @@ -2042,7 +2042,7 @@ static void restore_mailbox_cb(const char *mboxname, void *data, void *rock) if (++count % BATCH_SIZE == 0) { /* Close and re-open mailboxes (to avoid deadlocks) */ - jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); if (newmailbox) { mailbox_close(&newmailbox); @@ -2113,9 +2113,8 @@ static void restore_mailbox_cb(const char *mboxname, void *data, void *rock) mailbox_commit(mailbox); } - jmap_closembox(req, &mailbox); - done: + mailbox_close(&mailbox); mailbox_close(&newmailbox); mbname_free(&mbname); rrock->result = r; diff --git a/imap/jmap_calendar.c b/imap/jmap_calendar.c index fcd1e7ad7d..b58391dcc4 100644 --- a/imap/jmap_calendar.c +++ b/imap/jmap_calendar.c @@ -1774,7 +1774,7 @@ static int set_scheddefault(jmap_req_t *req, annotate_state_t *astate, const cha else r = IMAP_PERMISSION_DENIED; } - jmap_closembox(req, &mbox); + mailbox_close(&mbox); free(mboxname); } else { @@ -1924,7 +1924,7 @@ static void setcalendars_destroy(jmap_req_t *req, const char *calid, *err = jmap_server_error(r); } } - jmap_closembox(req, &calhome_mbox); + mailbox_close(&calhome_mbox); free(calhome_name); free(mboxname); free(defaultname); @@ -2058,7 +2058,7 @@ static void setcalendars_create(struct jmap_req *req, "mboxname=<%s> err=<%s>", mboxname, error_message(r)); mailbox_abort(mbox); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); int rr = mboxlist_deletemailbox(mboxname, 1, "", NULL, NULL, 0); if (rr) { syslog(LOG_ERR, "could not delete mailbox %s: %s", @@ -2140,7 +2140,7 @@ static void setcalendars_update(jmap_req_t *req, "mboxname=<%s> err=<%s>", mboxname, error_message(r)); mailbox_abort(mbox); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); } } if (r) { @@ -2163,7 +2163,7 @@ static void setcalendars_update(jmap_req_t *req, done: setcalendar_props_fini(&props); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); jmap_parser_fini(&parser); mbname_free(&mbname); free(mboxname); @@ -2594,7 +2594,7 @@ static int jmap_calendarevent_getblob(jmap_req_t *req, jmap_getblob_context_t *c ctx->errstr = desc; } if (ical) icalcomponent_free(ical); - if (mailbox) jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); mboxlist_entry_free(&freeme); free(mboxid); free(partid); @@ -2957,7 +2957,7 @@ static int getcalendarevents_getinstances(json_t *jsevent, if (!ical) { /* Open calendar mailbox. */ if (!rock->mailbox || strcmp(mailbox_name(rock->mailbox), mbentry->name)) { - jmap_closembox(req, &rock->mailbox); + mailbox_close(&rock->mailbox); r = jmap_openmbox(req, mbentry->name, &rock->mailbox, 0); if (r) goto done; } @@ -3368,7 +3368,7 @@ static int getcalendarevents_cb(void *vrock, struct caldav_jscal *jscal) /* Open calendar mailbox. */ if (!rock->mailbox || strcmp(mailbox_uniqueid(rock->mailbox), rock->mbentry->uniqueid)) { - jmap_closembox(req, &rock->mailbox); + mailbox_close(&rock->mailbox); r = jmap_openmbox_by_uniqueid(req, rock->mbentry->uniqueid, &rock->mailbox, 0); if (r) goto done; } @@ -4107,7 +4107,7 @@ static int jmap_calendarevent_get(struct jmap_req *req) jmap_parser_fini(&parser); jmap_get_fini(&get); if (db) caldav_close(db); - if (rock.mailbox) jmap_closembox(req, &rock.mailbox); + mailbox_close(&rock.mailbox); if (rock.mbentry) mboxlist_entry_free(&rock.mbentry); if (rock.mbname) mbname_free(&rock.mbname); if (rock.ical) icalcomponent_free(rock.ical); @@ -4556,7 +4556,7 @@ static int createevent_load_ical(jmap_req_t *req, srcical = NULL; } - jmap_closembox(req, &srcmbox); + mailbox_close(&srcmbox); } if (!create->resourcename) { @@ -4723,7 +4723,7 @@ static int createevent_store(jmap_req_t *req, done: spool_free_hdrcache(txn.req_hdrs); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); buf_free(&txn.buf); buf_free(&buf); return r; @@ -5690,7 +5690,7 @@ static void setcalendarevents_update(jmap_req_t *req, if (r) { syslog(LOG_ERR, "mailbox_rewrite_index_record (%s) failed: %s", cdata->dav.mailbox, error_message(r)); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); goto done; } mboxevent_extract_record(mboxevent, mbox, &record); @@ -5702,7 +5702,7 @@ static void setcalendarevents_update(jmap_req_t *req, mboxevent_free(&mboxevent); /* Close the mailbox we moved the event from. */ - jmap_closembox(req, &mbox); + mailbox_close(&mbox); mbox = dstmbox; dstmbox = NULL; } @@ -5820,8 +5820,8 @@ static void setcalendarevents_update(jmap_req_t *req, json_decref(update.event_patch); jstimezones_free(&update.jstzones); - if (mbox) jmap_closembox(req, &mbox); - if (dstmbox) jmap_closembox(req, &dstmbox); + mailbox_close(&mbox); + mailbox_close(&dstmbox); jmap_parser_fini(&parser); strarray_fini(&del_imapflags); strarray_fini(&schedule_addresses); @@ -6059,7 +6059,7 @@ static int setcalendarevents_destroy(jmap_req_t *req, if (r) { syslog(LOG_ERR, "mailbox_rewrite_index_record (%s) failed: %s", cdata->dav.mailbox, error_message(r)); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); goto done; } } @@ -6109,7 +6109,7 @@ static int setcalendarevents_destroy(jmap_req_t *req, is_standalone_instance ? eid->ical_recurid : NULL); done: - if (mbox) jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (oldical) icalcomponent_free(oldical); if (newical) icalcomponent_free(newical); json_decref(old_event); @@ -6336,8 +6336,8 @@ static int jmap_calendarevent_set(struct jmap_req *req) jmap_ok(req, jmap_set_reply(&set)); done: - jmap_closembox(req, &schedinbox); - jmap_closembox(req, ¬ifmbox); + mailbox_close(&schedinbox); + mailbox_close(¬ifmbox); mboxlist_entry_free(¬ifmb); jmap_parser_fini(&parser); jmap_set_fini(&set); @@ -6695,9 +6695,7 @@ static int eventquery_cb(void *vrock, struct caldav_jscal *jscal) else if (rock->expandrecur) { /* Load iCalendar data for main event */ if (!rock->mailbox || strcmp(mailbox_name(rock->mailbox), mbentry->name)) { - if (rock->mailbox) { - jmap_closembox(req, &rock->mailbox); - } + mailbox_close(&rock->mailbox); r = jmap_openmbox(req, mbentry->name, &rock->mailbox, 0); if (r) goto done; } @@ -6937,7 +6935,7 @@ static int eventquery_textsearch_run(jmap_req_t *req, if (expandrecur) { if (!mailbox || strcmp(mailbox_name(mailbox), mbentry->name)) { - if (mailbox) jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); r = jmap_openmbox(req, mbentry->name, &mailbox, 0); if (r) goto done; } @@ -6972,7 +6970,7 @@ static int eventquery_textsearch_run(jmap_req_t *req, if (searchargs) freesearchargs(searchargs); if (sortcrit) freesortcrit(sortcrit); if (query) search_query_free(query); - jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); free(sched_inboxname); free(icalbefore); free(icalafter); @@ -7297,7 +7295,7 @@ static int eventquery_run(jmap_req_t *req, args.expandrecur ? &mboxsort : sort, args.expandrecur ? 1 : nsort, eventquery_cb, &rock); - jmap_closembox(req, &rock.mailbox); + mailbox_close(&rock.mailbox); if (r_db) goto done; } @@ -7675,7 +7673,7 @@ static void _calendarevent_copy(jmap_req_t *req, return; } mboxlist_entry_free(&mbentry); - jmap_closembox(req, &src_mbox); + mailbox_close(&src_mbox); strarray_fini(&schedule_addresses); if (src_ical) icalcomponent_free(src_ical); json_decref(dst_event); @@ -7757,7 +7755,7 @@ static int jmap_calendarevent_copy(struct jmap_req *req) } done: - jmap_closembox(req, ¬ifmbox); + mailbox_close(¬ifmbox); mboxlist_entry_free(¬ifmb); json_decref(destroy_events); if (src_db) caldav_close(src_db); @@ -8040,7 +8038,7 @@ static int jmap_calendarevent_participantreply(struct jmap_req *req) r = IMAP_INTERNAL; goto done; } - jmap_closembox(req, &mbox); + mailbox_close(&mbox); /* Find participantId */ icalcomponent *comp = icalcomponent_get_first_real_component(update.oldical); @@ -8201,7 +8199,7 @@ static int jmap_calendarevent_participantreply(struct jmap_req *req) jmap_parser_fini(&parser); jmap_caleventid_free(&update.eid); if (db) caldav_close(db); - if (mbox) jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (update.oldical) icalcomponent_free(update.oldical); if (update.newical) icalcomponent_free(update.newical); jstimezones_free(&update.jstzones); @@ -8458,7 +8456,7 @@ static int principal_state_init(jmap_req_t *req, SHA1_CTX *sha1) SHA1Update(sha1, buf_base(&buf), buf_len(&buf)); buf_free(&buf); } - jmap_closembox(req, &mbox); + mailbox_close(&mbox); free(calhomename); return r; } @@ -9181,7 +9179,7 @@ static int jmap_principal_set(struct jmap_req *req) buf_free(&val); } } - jmap_closembox(req, &mbox); + mailbox_close(&mbox); free(calhomename); if (!r) { json_object_set_new(set.updated, id, json_object()); @@ -9379,9 +9377,7 @@ static int principal_getavailability_cb(void *vrock, struct caldav_jscal *jscal) if (!rock->mbox || strcmp(mailbox_uniqueid(rock->mbox), rock->mbentry->uniqueid)) { /* reset state for calendar collection */ - if (rock->mbox) { - jmap_closembox(rock->req, &rock->mbox); - } + mailbox_close(&rock->mbox); if (rock->floatingtz) { icaltimezone_free(rock->floatingtz, 1); rock->floatingtz = NULL; @@ -9559,9 +9555,7 @@ static void principal_getavailability(jmap_req_t *req, principal_getavailability_cb, &rock); caldav_jscal_filter_fini(&jscal_filter); if (r) jmap_error(req, jmap_server_error(r)); - if (rock.mbox) { - jmap_closembox(req, &rock.mbox); - } + mailbox_close(&rock.mbox); if (rock.mbentry) { mboxlist_entry_free(&rock.mbentry); } @@ -10083,7 +10077,7 @@ static void notif_get(struct jmap_req *req, } done: - jmap_closembox(req, ¬ifmbox); + mailbox_close(¬ifmbox); seqset_free(&seenuids); } @@ -10217,7 +10211,7 @@ static void notif_set(struct jmap_req *req, done: seqset_free(&seenuids); seen_close(&seendb); - jmap_closembox(req, ¬ifmbox); + mailbox_close(¬ifmbox); buf_free(&buf); } @@ -10287,7 +10281,7 @@ static void notif_changes(struct jmap_req *req, done: dynarray_free(&entries); - jmap_closembox(req, ¬ifmbox); + mailbox_close(¬ifmbox); } @@ -10878,7 +10872,7 @@ static int jmap_sharenotification_query(struct jmap_req *req) jmap_ok(req, res); done: - jmap_closembox(req, ¬ifmbox); + mailbox_close(¬ifmbox); mboxlist_entry_free(¬ifmb); jmap_query_fini(&query); jmap_parser_fini(&parser); @@ -11346,7 +11340,7 @@ static int jmap_calendareventnotification_query(struct jmap_req *req) done: if (eventids.size) free_hash_table(&eventids, NULL); - jmap_closembox(req, ¬ifmbox); + mailbox_close(¬ifmbox); jmap_query_fini(&query); jmap_parser_fini(&parser); return 0; @@ -12023,7 +12017,7 @@ static void calendarpreferences_set(struct jmap_req *req, if (r && *err == NULL) { *err = jmap_server_error(r); } - jmap_closembox(req, &calhomembox); + mailbox_close(&calhomembox); json_decref(jalertsprefs); buf_free(&buf); } diff --git a/imap/jmap_contact.c b/imap/jmap_contact.c index 3f0911098c..9cffb74b44 100644 --- a/imap/jmap_contact.c +++ b/imap/jmap_contact.c @@ -1281,7 +1281,7 @@ static void _contacts_set(struct jmap_req *req, unsigned kind, } if (!mailbox || strcmp(mailbox_name(mailbox), mbentry->name)) { - jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); r = jmap_openmbox(req, mbentry->name, &mailbox, 1); } mboxlist_entry_free(&mbentry); @@ -1315,8 +1315,8 @@ static void _contacts_set(struct jmap_req *req, unsigned kind, if (r) jmap_error(req, jmap_server_error(r)); jmap_parser_fini(&parser); jmap_set_fini(&set); - jmap_closembox(req, &newmailbox); - jmap_closembox(req, &mailbox); + mailbox_close(&newmailbox); + mailbox_close(&mailbox); carddav_close(db); } @@ -4045,7 +4045,7 @@ static int _contact_set_create(jmap_req_t *req, unsigned kind, json_t *jcard, /* we need to create and append a record */ if (!*mailbox || strcmp(mailbox_name(*mailbox), mboxname)) { - jmap_closembox(req, mailbox); + mailbox_close(mailbox); r = jmap_openmbox(req, mboxname, mailbox, 1); if (r == IMAP_MAILBOX_NONEXISTENT) { json_array_append_new(invalid, json_string("addressbookId")); @@ -4202,7 +4202,7 @@ static int _contact_set_update(jmap_req_t *req, unsigned kind, } if (!*mailbox || strcmp(mailbox_name(*mailbox), mbentry->name)) { - jmap_closembox(req, mailbox); + mailbox_close(mailbox); r = jmap_openmbox(req, mbentry->name, mailbox, 1); } if (r) { @@ -4371,7 +4371,7 @@ static int _contact_set_update(jmap_req_t *req, unsigned kind, done: mboxlist_entry_free(&mbentry); - jmap_closembox(req, &newmailbox); + mailbox_close(&newmailbox); strarray_free(flags); freeentryatts(annots); vparse_free_card(vcard); @@ -4500,8 +4500,8 @@ static void _contact_copy(jmap_req_t *req, *set_err = jmap_server_error(r); } mboxlist_entry_free(&mbentry); - jmap_closembox(req, &dst_mbox); - jmap_closembox(req, &src_mbox); + mailbox_close(&dst_mbox); + mailbox_close(&src_mbox); json_decref(dst_card); jmap_parser_fini(&myparser); } @@ -5134,7 +5134,7 @@ static int setaddressbook_writeprops(jmap_req_t *req, buf_free(&val); if (mbox) { if (r) mailbox_abort(mbox); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); } return r; } @@ -11109,7 +11109,7 @@ static int _card_set_create(jmap_req_t *req, /* we need to create and append a record */ if (!*mailbox || strcmp(mailbox_name(*mailbox), mboxname)) { - jmap_closembox(req, mailbox); + mailbox_close(mailbox); r = jmap_openmbox(req, mboxname, mailbox, 1); if (r == IMAP_MAILBOX_NONEXISTENT) { json_array_append_new(invalid, json_string("addressbookId")); @@ -11289,7 +11289,7 @@ static int _card_set_update(jmap_req_t *req, unsigned kind, } if (!*mailbox || strcmp(mailbox_name(*mailbox), mbentry->name)) { - jmap_closembox(req, mailbox); + mailbox_close(mailbox); r = jmap_openmbox(req, mbentry->name, mailbox, 1); } if (r) { @@ -11457,7 +11457,7 @@ static int _card_set_update(jmap_req_t *req, unsigned kind, done: mboxlist_entry_free(&mbentry); - jmap_closembox(req, &newmailbox); + mailbox_close(&newmailbox); freeentryatts(annots); vcardcomponent_free(vcard); free(resource); @@ -11662,7 +11662,7 @@ static int jmap_card_parse(jmap_req_t *req) json_array_append_new(parse.not_parsable, json_string(blobid)); } - if (mailbox) jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); } jmap_getblob_ctx_fini(&blob_ctx); @@ -11823,7 +11823,7 @@ static int jmap_contact_getblob(jmap_req_t *req, jmap_getblob_context_t *ctx) ctx->errstr = desc; } if (vcard) vcardcomponent_free(vcard); - if (mailbox) jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); mboxlist_entry_free(&freeme); free(mboxid); free(propname); @@ -11955,7 +11955,7 @@ static int jmap_contact_getblob(jmap_req_t *req, jmap_getblob_context_t *ctx) ctx->errstr = desc; } if (vcard) vparse_free_card(vcard); - if (mailbox) jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); mboxlist_entry_free(&freeme); free(mboxid); free(prop); diff --git a/imap/jmap_core.c b/imap/jmap_core.c index e0f0f911d1..8e6016bece 100644 --- a/imap/jmap_core.c +++ b/imap/jmap_core.c @@ -344,7 +344,7 @@ static int jmap_copyblob(jmap_req_t *req, message_free_body(body); free(body); msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); return r; } @@ -946,7 +946,7 @@ static int jmap_blob_lookup(jmap_req_t *req) if (caldav_db) caldav_close(caldav_db); if (carddav_db) carddav_close(carddav_db); mbname_free(&mbname); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); } /* Clean up memory */ diff --git a/imap/jmap_ical.c b/imap/jmap_ical.c index 57a1df1a00..d1b93f7fce 100644 --- a/imap/jmap_ical.c +++ b/imap/jmap_ical.c @@ -425,7 +425,7 @@ static int create_managedattach(struct jmapical_ctx *jmapctx, message_free_body(body); free(body); } - jmap_closembox(req, &srcmbox); + mailbox_close(&srcmbox); msgrecord_unref(&mr); buf_free(&preamble); return r; @@ -457,7 +457,7 @@ HIDDEN int jmapical_context_open_attachments(struct jmapical_ctx *jmapctx) if (!jmapctx->attachments.db) { xsyslog(LOG_ERR, "mailbox_open_webdav failed", "attachments=<%s>", mailbox_name(jmapctx->attachments.mbox)); - jmap_closembox(req, &jmapctx->attachments.mbox); + mailbox_close(&jmapctx->attachments.mbox); jmapctx->attachments.db = NULL; jmapctx->attachments.err = IMAP_INTERNAL; return jmapctx->attachments.err; @@ -558,8 +558,7 @@ HIDDEN void jmapical_context_free(struct jmapical_ctx **jmapctxp) if (!jmapctx) return; #ifndef BUILD_LMTPD - if (jmapctx->attachments.mbox) - jmap_closembox(jmapctx->req, &jmapctx->attachments.mbox); + mailbox_close(&jmapctx->attachments.mbox); if (jmapctx->attachments.db) webdav_close(jmapctx->attachments.db); #endif // BUILD_LMTPD diff --git a/imap/jmap_mail.c b/imap/jmap_mail.c index 9e651e2253..9d6804a133 100644 --- a/imap/jmap_mail.c +++ b/imap/jmap_mail.c @@ -816,7 +816,7 @@ static int _email_mailboxes_cb(const conv_guidrec_t *rec, void *rock) done: if (mr) msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); return r; } @@ -1114,7 +1114,7 @@ static int _email_find_cb(const conv_guidrec_t *rec, void *rock) done: mboxlist_entry_free(&mbentry); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); return d->mboxname ? IMAP_OK_COMPLETED : 0; } @@ -1256,7 +1256,7 @@ static int _email_is_expunged_cb(const conv_guidrec_t *rec, void *rock) } } - jmap_closembox(check->req, &mbox); + mailbox_close(&mbox); return 0; } @@ -3906,7 +3906,7 @@ static int guidsearch_run_xapian(jmap_req_t *req, struct emailsearch *search, done: if (bx) search_end_search(bx); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); mbname_free(&mbname); return r; } @@ -5850,7 +5850,7 @@ static int _snippet_get(jmap_req_t *req, json_t *filter, doneloop: if (mr) msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); free(mboxname); mboxname = NULL; if (r) goto done; @@ -5866,7 +5866,7 @@ static int _snippet_get(jmap_req_t *req, json_t *filter, if (snippet) json_decref(snippet); if (intquery) search_free_internalised(intquery); if (mboxname) free(mboxname); - if (mbox) jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (searchargs) freesearchargs(searchargs); strarray_fini(&perf_filters); if (ptrarray_size(&search_attrs)) { @@ -6441,7 +6441,7 @@ static int _email_get_keywords_cb(const conv_guidrec_t *rec, void *vrock) done: msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); return r; } @@ -6998,7 +6998,7 @@ static int _email_get_createdmodseq_cb(const conv_guidrec_t *rec, void *vrock) rock->modseq = index_record.createdmodseq; } } - jmap_closembox(rock->req, &mbox); + mailbox_close(&mbox); } done: @@ -8111,7 +8111,7 @@ static int _warmup_mboxcache_cb(const conv_guidrec_t *rec, void* vrock) if (!mboxname_isnondeliverymailbox(mailbox_name(mbox), mailbox_mbtype(mbox))) { ptrarray_append(&rock->mboxes, mbox); } - else jmap_closembox(rock->req, &mbox); + else mailbox_close(&mbox); } return r; } @@ -8154,7 +8154,7 @@ static void jmap_email_get_full(jmap_req_t *req, struct jmap_get *get, struct em if (!r) { r = _email_from_record(req, args, mr, &msg); } - jmap_closembox(req, &mbox); + mailbox_close(&mbox); } } if (!r && msg) { @@ -8174,7 +8174,7 @@ static void jmap_email_get_full(jmap_req_t *req, struct jmap_get *get, struct em /* Close cached mailboxes */ struct mailbox *mbox = NULL; while ((mbox = ptrarray_pop(&rock.mboxes))) { - jmap_closembox(req, &mbox); + mailbox_close(&mbox); } ptrarray_fini(&rock.mboxes); } @@ -8570,7 +8570,7 @@ static int jmap_email_parse(jmap_req_t *req) json_array_append_new(parse.not_parsable, json_string(blobid)); } msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); message_free_body(body); free(body); } @@ -8986,7 +8986,7 @@ static void _email_append(jmap_req_t *req, r = _copy_msgrecord(httpd_authstate, req->userid, &jmap_namespace, mbox, dst, mr); - jmap_closembox(req, &dst); + mailbox_close(&dst); if (r) goto done; } @@ -8994,7 +8994,7 @@ static void _email_append(jmap_req_t *req, if (f) fclose(f); append_removestage(stage); msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); free(mboxname); json_decref(mailboxes); if (r && *err == NULL) { @@ -11447,7 +11447,7 @@ void _email_bulkupdate_close(struct email_bulkupdate *bulk) hash_iter *iter = hash_table_iter(&bulk->plans_by_mbox_id); while (hash_iter_next(iter)) { struct email_updateplan *plan = hash_iter_val(iter); - jmap_closembox(bulk->req, &plan->mbox); + mailbox_close(&plan->mbox); } seen_close(&bulk->seendb); /* force-close on error */ hash_iter_free(&iter); @@ -12277,7 +12277,7 @@ static int _email_bulkupdate_plan(struct email_bulkupdate *bulk, ptrarray_t *upd const char *mbox_id = strarray_nth(erroneous_mboxids, i); struct email_updateplan *plan = hash_del(mbox_id, &bulk->plans_by_mbox_id); if (!plan) continue; - jmap_closembox(bulk->req, &plan->mbox); + mailbox_close(&plan->mbox); _email_updateplan_free_p(plan); } strarray_free(erroneous_mboxids); @@ -12485,7 +12485,7 @@ static int _email_bulkupdate_open(jmap_req_t *req, struct email_bulkupdate *bulk ptrarray_append(bulk->new_mboxrecs, mboxrec); _email_bulkupdate_addplan(bulk, mbox, mboxrec); } - else jmap_closembox(req, &mbox); // already reference counted + else mailbox_close(&mbox); // already reference counted } else { json_object_set_new(bulk->set_errors, update->email_id, @@ -13100,7 +13100,7 @@ static void _email_destroy_bulk(jmap_req_t *req, } } } - jmap_closembox(req, &mbox); + mailbox_close(&mbox); } /* An email not reported was not found (already expunged) */ @@ -13273,7 +13273,7 @@ static void _email_import(jmap_req_t *req, buf_reset(&content); sourcefile = NULL; msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); struct body *body = NULL; const struct body *part = NULL; @@ -13367,7 +13367,7 @@ static void _email_import(jmap_req_t *req, has_attachment, sourcefile, _email_import_cb, &content, &detail, err); msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (*err) goto done; @@ -13715,7 +13715,7 @@ static int _email_copy_writeprops_cb(const conv_guidrec_t* rec, void* _rock) mboxevent_free(&flagsset); mboxevent_free(&flagsclear); if (mr) msgrecord_unref(&mr); - jmap_closembox(rock->req, &mbox); + mailbox_close(&mbox); return r; } @@ -13759,7 +13759,7 @@ static int _email_exists_cb(const conv_guidrec_t *rec, void *rock) done: if (mr) msgrecord_unref(&mr); mboxlist_entry_free(&mbentry); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); return r; } @@ -13826,7 +13826,7 @@ static int _email_copy_pickrecord_cb(const conv_guidrec_t *rec, void *vrock) done: msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); return r; } @@ -13969,7 +13969,7 @@ static void _email_copy(jmap_req_t *req, json_t *copy_email, r = _copy_msgrecord(httpd_authstate, req->accountid, &jmap_namespace, src_mbox, dst_mbox, src_mr); } - jmap_closembox(req, &dst_mbox); + mailbox_close(&dst_mbox); free(dst_mboxname); } mboxlist_entry_free(&mbentry); @@ -14003,7 +14003,7 @@ static void _email_copy(jmap_req_t *req, json_t *copy_email, free(blob_id); strarray_fini(&dst_mboxnames); if (src_mr) msgrecord_unref(&src_mr); - jmap_closembox(req, &src_mbox); + mailbox_close(&src_mbox); json_decref(new_keywords); } @@ -14338,7 +14338,7 @@ static int jmap_emailheader_getblob(jmap_req_t *req, jmap_getblob_context_t *ctx } ctx->errstr = desc; } - if (mailbox) jmap_closembox(req, &mailbox); + mailbox_close(&mailbox); free(emailid); free(mboxname); return res; diff --git a/imap/jmap_mail_submission.c b/imap/jmap_mail_submission.c index 4f879f5e71..8b6da09966 100644 --- a/imap/jmap_mail_submission.c +++ b/imap/jmap_mail_submission.c @@ -793,7 +793,7 @@ static void _emailsubmission_create(jmap_req_t *req, * message open. But we don't want to long-lock the * mailbox while sending the mail over to a SMTP host */ msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (!*sm) { /* Open the SMTP connection */ @@ -917,7 +917,7 @@ static void _emailsubmission_create(jmap_req_t *req, if (fd_msg != -1) close(fd_msg); if (msg) json_decref(msg); if (mr) msgrecord_unref(&mr); - if (mbox) jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (myenvelope) json_decref(myenvelope); free(mboxname); buf_free(&buf); @@ -1370,7 +1370,7 @@ static int jmap_emailsubmission_get(jmap_req_t *req) mailbox_iter_done(&iter); } - if (mbox) jmap_closembox(req, &mbox); + mailbox_close(&mbox); /* Build response */ get.state = modseqtoa(jmap_modseq(req, MBTYPE_JMAPSUBMIT, @@ -1645,7 +1645,7 @@ static int jmap_emailsubmission_set(jmap_req_t *req) } done: - jmap_closembox(req, &submbox); + mailbox_close(&submbox); jmap_parser_fini(&parser); jmap_set_fini(&set); json_decref(success_emailids); @@ -1723,7 +1723,7 @@ static int jmap_emailsubmission_changes(jmap_req_t *req) } mailbox_iter_done(&iter); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); /* Set new state */ // XXX - this is wrong! If we want to do this, we need to sort all the changes by @@ -2206,7 +2206,7 @@ static int jmap_emailsubmission_query(jmap_req_t *req) } mailbox_iter_done(&iter); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); /* Sort results */ if (sortcrit) { diff --git a/imap/jmap_mailbox.c b/imap/jmap_mailbox.c index d1b9fb31ec..baf42a5993 100644 --- a/imap/jmap_mailbox.c +++ b/imap/jmap_mailbox.c @@ -2766,7 +2766,7 @@ static void _mbox_update(jmap_req_t *req, struct mboxset_args *args, mboxlist_update_foldermodseq(mailbox_name(mbox), mbox->i.highestmodseq); } } - jmap_closembox(req, &mbox); + mailbox_close(&mbox); } if (r) goto done; @@ -2898,8 +2898,8 @@ static int _mbox_on_destroy_move(jmap_req_t *req, if (r && !result->err) { result->err = jmap_server_error(r); } - jmap_closembox(req, &dst_mbox); - jmap_closembox(req, &src_mbox); + mailbox_close(&dst_mbox); + mailbox_close(&src_mbox); ptrarray_fini(&move_msgrecs); return r; } @@ -2978,7 +2978,7 @@ static void _mbox_destroy(jmap_req_t *req, const char *mboxid, result->err = json_pack("{s:s}", "type", "mailboxHasEmail"); } mailbox_iter_done(&iter); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (result->err) goto done; } } diff --git a/imap/jmap_mdn.c b/imap/jmap_mdn.c index 5b7b066ff5..f745ec4ac7 100644 --- a/imap/jmap_mdn.c +++ b/imap/jmap_mdn.c @@ -420,7 +420,7 @@ static json_t *generate_mdn(struct jmap_req *req, done: if (r && err == NULL) err = jmap_server_error(r); if (mr) msgrecord_unref(&mr); - if (mbox) jmap_closembox(req, &mbox); + mailbox_close(&mbox); free(mboxname); buf_free(&buf); @@ -644,7 +644,7 @@ static int jmap_mdn_parse(jmap_req_t *req) json_array_append_new(parse.not_parsable, json_string(blobid)); } msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); message_free_body(body); free(body); buf_free(&buf); diff --git a/imap/jmap_notes.c b/imap/jmap_notes.c index 227d6d149d..2a473305b7 100644 --- a/imap/jmap_notes.c +++ b/imap/jmap_notes.c @@ -503,7 +503,7 @@ static int jmap_note_get(jmap_req_t *req) get.state = buf_release(&buf); jmap_ok(req, jmap_get_reply(&get)); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); done: jmap_parser_fini(&parser); @@ -955,7 +955,7 @@ static int jmap_note_set(jmap_req_t *req) jmap_ok(req, jmap_set_reply(&set)); done: - jmap_closembox(req, &mbox); + mailbox_close(&mbox); jmap_parser_fini(&parser); jmap_set_fini(&set); return 0; @@ -1057,7 +1057,7 @@ static int jmap_note_changes(jmap_req_t *req) } } mailbox_iter_done(&iter); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); buf_free(&buf); if (changes.max_changes) { diff --git a/imap/jmap_sieve.c b/imap/jmap_sieve.c index 832602bc88..9b416584fa 100644 --- a/imap/jmap_sieve.c +++ b/imap/jmap_sieve.c @@ -397,7 +397,7 @@ static const char *script_findblob(struct jmap_req *req, const char *id, content += record.header_size; msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); } } @@ -2023,7 +2023,7 @@ static int jmap_sieve_test(struct jmap_req *req) r = sieve_script_parse_string(NULL, content, &errors, &s); msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (r != SIEVE_OK) { err = json_pack("{s:s, s:s}", "type", "invalidScript", @@ -2141,7 +2141,7 @@ static int jmap_sieve_test(struct jmap_req *req) free_msg(&m); msgrecord_unref(&mr); - jmap_closembox(req, &mbox); + mailbox_close(&mbox); if (!envelope) { strarray_fini(&env_from); strarray_fini(&env_to); From f601f32ce1df83126a588567ebca0eeaa708e123 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 15:11:13 +0100 Subject: [PATCH 13/38] mailbox_entry_free checks if null already --- imap/jmap_calendar.c | 10 ++++------ imap/jmap_contact.c | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/imap/jmap_calendar.c b/imap/jmap_calendar.c index b58391dcc4..ec3eb5a6cf 100644 --- a/imap/jmap_calendar.c +++ b/imap/jmap_calendar.c @@ -1041,7 +1041,7 @@ static int jmap_calendar_get(struct jmap_req *req) } } - if (mbentry) mboxlist_entry_free(&mbentry); + mboxlist_entry_free(&mbentry); free(mboxname); if (r) goto done; } @@ -4108,8 +4108,8 @@ static int jmap_calendarevent_get(struct jmap_req *req) jmap_get_fini(&get); if (db) caldav_close(db); mailbox_close(&rock.mailbox); - if (rock.mbentry) mboxlist_entry_free(&rock.mbentry); - if (rock.mbname) mbname_free(&rock.mbname); + mboxlist_entry_free(&rock.mbentry); + mbname_free(&rock.mbname); if (rock.ical) icalcomponent_free(rock.ical); if (rock.ical_instances_by_recurid.size) free_hash_table(&rock.ical_instances_by_recurid, _icalcomponent_free_cb); @@ -9556,9 +9556,7 @@ static void principal_getavailability(jmap_req_t *req, caldav_jscal_filter_fini(&jscal_filter); if (r) jmap_error(req, jmap_server_error(r)); mailbox_close(&rock.mbox); - if (rock.mbentry) { - mboxlist_entry_free(&rock.mbentry); - } + mboxlist_entry_free(&rock.mbentry); if (rock.floatingtz) { icaltimezone_free(rock.floatingtz, 1); rock.floatingtz = NULL; diff --git a/imap/jmap_contact.c b/imap/jmap_contact.c index 9cffb74b44..91c640955c 100644 --- a/imap/jmap_contact.c +++ b/imap/jmap_contact.c @@ -4838,7 +4838,7 @@ static int jmap_addressbook_get(struct jmap_req *req) } } - if (mbentry) mboxlist_entry_free(&mbentry); + mboxlist_entry_free(&mbentry); free(mboxname); if (r) goto done; } From c14cfdc10e0b43592ed2e17999c9fda3bc8b9516 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 15:48:54 +0100 Subject: [PATCH 14/38] jmap_api: remove force_openmbox_rw This makes jmap_openmbox useless, we'll remove that next --- imap/jmap_api.c | 15 ++------------- imap/jmap_api.h | 12 ------------ imap/jmap_mail.c | 6 +++--- 3 files changed, 5 insertions(+), 28 deletions(-) diff --git a/imap/jmap_api.c b/imap/jmap_api.c index f4362f74f6..1fa5bf8637 100644 --- a/imap/jmap_api.c +++ b/imap/jmap_api.c @@ -1053,21 +1053,10 @@ void jmap_add_id(jmap_req_t *req, const char *creation_id, const char *id) hash_insert(creation_id, xstrdup(id), req->created_ids); } -HIDDEN int jmap_openmbox(jmap_req_t *req, const char *name, +HIDDEN int jmap_openmbox(jmap_req_t *req __attribute__((unused)), const char *name, struct mailbox **mboxp, int rw) { - int r; - - /* Add mailbox to cache */ - if (req->force_openmbox_rw) - rw = 1; - r = rw ? mailbox_open_iwl(name, mboxp) : mailbox_open_irl(name, mboxp); - if (r) { - syslog(LOG_ERR, "jmap_openmbox(%s): %s", name, error_message(r)); - return r; - } - - return 0; + return rw ? mailbox_open_iwl(name, mboxp) : mailbox_open_irl(name, mboxp); } HIDDEN int jmap_openmbox_by_uniqueid(jmap_req_t *req, const char *id, diff --git a/imap/jmap_api.h b/imap/jmap_api.h index 1564d30f4d..5bdcc26b32 100644 --- a/imap/jmap_api.h +++ b/imap/jmap_api.h @@ -152,18 +152,6 @@ typedef struct jmap_req { double sys_start; json_t *perf_details; - /* The JMAP request keeps its own cache of opened mailboxes, - * which can be used by calling jmap_openmbox. If the - * force_openmboxrw is set, this causes all following - * mailboxes to be opened read-writeable, irrespective if - * the caller asked for a read-only lock. This allows to - * prevent lock promotion conflicts, in case a cached mailbox - * was opened read-only by a helper but it now asked to be - * locked exclusively. Since the mailbox lock does not - * support lock promition, this would currently abort with - * an error. */ - int force_openmbox_rw; - /* Internal state */ hash_table *mbstates; hash_table *created_ids; diff --git a/imap/jmap_mail.c b/imap/jmap_mail.c index 9d6804a133..23a5ec8445 100644 --- a/imap/jmap_mail.c +++ b/imap/jmap_mail.c @@ -13227,9 +13227,6 @@ static void _email_import(jmap_req_t *req, int has_attachment = 0; const char *sourcefile = NULL; - /* Force write locks on mailboxes. */ - req->force_openmbox_rw = 1; - /* Gather keywords */ strarray_t keywords = STRARRAY_INITIALIZER; const json_t *val; @@ -13362,6 +13359,9 @@ static void _email_import(jmap_req_t *req, if (!internaldate) internaldate = time(NULL); + // mailbox will be readonly, drop the lock so it can be make writable + if (mbox) mailbox_unlock_index(mbox, NULL); + /* Write the message to the file system */ _email_append(req, jmailbox_ids, &keywords, internaldate, snoozed, has_attachment, sourcefile, _email_import_cb, &content, &detail, err); From 75fdfa8c9c33aad2d451f4e9804c181f2e86ed87 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 8 Nov 2023 16:20:50 +0100 Subject: [PATCH 15/38] remove jmap_openmbox() API --- imap/jmap_api.c | 11 ++----- imap/jmap_api.h | 2 -- imap/jmap_backup.c | 19 +++++------ imap/jmap_calendar.c | 63 +++++++++++++++---------------------- imap/jmap_contact.c | 30 +++++++++--------- imap/jmap_core.c | 2 +- imap/jmap_ical.c | 5 +-- imap/jmap_mail.c | 37 +++++++++++----------- imap/jmap_mail_submission.c | 10 +++--- imap/jmap_mailbox.c | 8 ++--- imap/jmap_mdn.c | 2 +- imap/jmap_notes.c | 6 ++-- 12 files changed, 89 insertions(+), 106 deletions(-) diff --git a/imap/jmap_api.c b/imap/jmap_api.c index 1fa5bf8637..d715adf709 100644 --- a/imap/jmap_api.c +++ b/imap/jmap_api.c @@ -1053,19 +1053,14 @@ void jmap_add_id(jmap_req_t *req, const char *creation_id, const char *id) hash_insert(creation_id, xstrdup(id), req->created_ids); } -HIDDEN int jmap_openmbox(jmap_req_t *req __attribute__((unused)), const char *name, - struct mailbox **mboxp, int rw) -{ - return rw ? mailbox_open_iwl(name, mboxp) : mailbox_open_irl(name, mboxp); -} - HIDDEN int jmap_openmbox_by_uniqueid(jmap_req_t *req, const char *id, struct mailbox **mboxp, int rw) { const mbentry_t *mbentry = jmap_mbentry_by_uniqueid(req, id); if (mbentry) - return jmap_openmbox(req, mbentry->name, mboxp, rw); + return rw ? mailbox_open_iwl(mbentry->name, mboxp) + : mailbox_open_irl(mbentry->name, mboxp); else return IMAP_MAILBOX_NONEXISTENT; } @@ -1107,7 +1102,7 @@ static int findblob_cb(const conv_guidrec_t *rec, void *rock) } } - r = jmap_openmbox(req, mbentry->name, &d->mbox, 0); + r = mailbox_open_irl(mbentry->name, &d->mbox); mboxlist_entry_free(&mbentry); if (r) return r; diff --git a/imap/jmap_api.h b/imap/jmap_api.h index 5bdcc26b32..4d52489a1d 100644 --- a/imap/jmap_api.h +++ b/imap/jmap_api.h @@ -265,8 +265,6 @@ extern void jmap_admin_capabilities(json_t *account_capabilities); extern void jmap_accounts(json_t *accounts, json_t *primary_accounts); /* Request-scoped mailbox cache */ -extern int jmap_openmbox(jmap_req_t *req, const char *name, - struct mailbox **mboxp, int rw); extern int jmap_openmbox_by_uniqueid(jmap_req_t *req, const char *id, struct mailbox **mboxp, int rw); extern int jmap_mboxlist_lookup(const char *name, diff --git a/imap/jmap_backup.c b/imap/jmap_backup.c index a2ac43bd62..920156cfcf 100644 --- a/imap/jmap_backup.c +++ b/imap/jmap_backup.c @@ -375,7 +375,7 @@ static int restore_collection_cb(const mbentry_t *mbentry, void *rock) return 0; } - r = jmap_openmbox(rrock->req, mbentry->name, &rrock->mailbox, /*rw*/0); + r = mailbox_open_irl(mbentry->name, &rrock->mailbox); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s for reading", mbentry->name); @@ -498,7 +498,7 @@ static int restore_collection_cb(const mbentry_t *mbentry, void *rock) We also do this on initial entry into the loop to switch from a read lock to a write lock. */ mailbox_close(&rrock->mailbox); - r = jmap_openmbox(rrock->req, mbentry->name, &rrock->mailbox, /*rw*/1); + r = mailbox_open_iwl(mbentry->name, &rrock->mailbox); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s for writing", mbentry->name); @@ -1279,7 +1279,7 @@ static int recreate_ical_resources(const mbentry_t *mbentry, jmap_req_t *req = rrock->req; int r; - r = jmap_openmbox(req, mbentry->name, &mailbox, /*rw*/1); + r = mailbox_open_iwl(mbentry->name, &mailbox); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s", mbentry->name); return r; @@ -1309,7 +1309,7 @@ static int recreate_ical_resources(const mbentry_t *mbentry, mailbox_iter_done(&iter); mailbox_close(&mailbox); - r = jmap_openmbox(req, mbentry->name, &mailbox, /*rw*/1); + r = mailbox_open_iwl(mbentry->name, &mailbox); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s for writing", mbentry->name); @@ -1623,7 +1623,7 @@ static int restore_message_list_cb(const mbentry_t *mbentry, void *rock) isdestroyed_mbox = 1; } - r = jmap_openmbox(rrock->req, mbentry->name, &mailbox, /*rw*/0); + r = mailbox_open_irl(mbentry->name, &mailbox); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s", mbentry->name); return r; @@ -2004,8 +2004,8 @@ static void restore_mailbox_cb(const char *mboxname, void *data, void *rock) } if (!r) { - r = jmap_openmbox(req, mboxname, &mailbox, - /*rw*/newmailbox == NULL || num_unflag); + int rw = (newmailbox == NULL || num_unflag); + r = rw ? mailbox_open_iwl(mboxname, &mailbox) : mailbox_open_irl(mboxname, &mailbox); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s: %s", mboxname, error_message(r)); @@ -2055,8 +2055,9 @@ static void restore_mailbox_cb(const char *mboxname, void *data, void *rock) } } - r = jmap_openmbox(req, mboxname, &mailbox, - /*rw*/newmailbox == NULL || num_unflag); + int rw = (newmailbox == NULL || num_unflag); + r = rw ? mailbox_open_iwl(mboxname, &mailbox) + : mailbox_open_irl(mboxname, &mailbox); if (r) { syslog(LOG_ERR, "IOERROR: failed to open mailbox %s: %s", mboxname, error_message(r)); diff --git a/imap/jmap_calendar.c b/imap/jmap_calendar.c index ec3eb5a6cf..6f2d26e1b7 100644 --- a/imap/jmap_calendar.c +++ b/imap/jmap_calendar.c @@ -1767,7 +1767,7 @@ static int set_scheddefault(jmap_req_t *req, annotate_state_t *astate, const cha struct mailbox *mbox = NULL; char *mboxname = caldav_mboxname(req->accountid, colname); // Make sure it exists and is writable - r = jmap_openmbox(req, mboxname, &mbox, 1); + r = mailbox_open_iwl(mboxname, &mbox); if (!r) { if (httpd_myrights(req->authstate, mbox->mbentry) & ACL_INSERT) r = annotate_state_writemask(astate, annot, req->accountid, &buf); @@ -1878,7 +1878,7 @@ static void setcalendars_destroy(jmap_req_t *req, const char *calid, /* Update default calendar - must go last */ if (!strcmpsafe(defaultname, calid)) { - int r2 = jmap_openmbox(req, calhome_name, &calhome_mbox, 1); + int r2 = mailbox_open_iwl(calhome_name, &calhome_mbox); if (r2) { xsyslog(LOG_ERR, "can not open calendar home mailbox", "err=<%s>", error_message(r)); @@ -2132,7 +2132,7 @@ static void setcalendars_update(jmap_req_t *req, } /* Update the calendar */ - int r = jmap_openmbox(req, mboxname, &mbox, /*rw*/1); + int r = mailbox_open_iwl(mboxname, &mbox); if (!r) { r = setcalendar_writeprops(req, mbox, &props); if (r) { @@ -2408,7 +2408,8 @@ static int jmap_calendarevent_getblob(jmap_req_t *req, jmap_getblob_context_t *c } /* Open mailbox, we need it now */ - if ((r = jmap_openmbox(req, mbentry->name, &mailbox, 0))) { + r = mailbox_open_irl(mbentry->name, &mailbox); + if (r) { ctx->errstr = error_message(r); res = HTTP_SERVER_ERROR; goto done; @@ -2958,7 +2959,7 @@ static int getcalendarevents_getinstances(json_t *jsevent, /* Open calendar mailbox. */ if (!rock->mailbox || strcmp(mailbox_name(rock->mailbox), mbentry->name)) { mailbox_close(&rock->mailbox); - r = jmap_openmbox(req, mbentry->name, &rock->mailbox, 0); + r = mailbox_open_irl(mbentry->name, &rock->mailbox); if (r) goto done; } myical = caldav_record_to_ical(rock->mailbox, cdata, req->userid, NULL); @@ -5588,7 +5589,7 @@ static void setcalendarevents_update(jmap_req_t *req, goto done; } else if (r) { - syslog(LOG_ERR, "jmap_openmbox(req, %s) failed: %s", + syslog(LOG_ERR, "jmap_openmbox_by_uniqueid(req, %s) failed: %s", mbentry->name, error_message(r)); goto done; } @@ -5985,12 +5986,8 @@ static int setcalendarevents_destroy(jmap_req_t *req, } /* Open mailbox for writing */ - r = jmap_openmbox(req, mboxname, &mbox, 1); - if (r) { - syslog(LOG_ERR, "jmap_openmbox(req, %s) failed: %s", - mboxname, error_message(r)); - goto done; - } + r = mailbox_open_iwl(mboxname, &mbox); + if (r) goto done; /* Fetch index record for the resource. Need this for scheduling. */ memset(&record, 0, sizeof(struct index_record)); @@ -6200,7 +6197,7 @@ static int jmap_calendarevent_set(struct jmap_req *req) /* Open CalDAV Scheduling Inbox, but continue even on error. */ char *inboxname = caldav_mboxname(req->accountid, SCHED_INBOX); - r = jmap_openmbox(req, inboxname, &schedinbox, 1); + r = mailbox_open_iwl(inboxname, &schedinbox); if (r) { xsyslog(LOG_WARNING, "can not open CalDAV Scheduling Inbox", "accountid=%s error=%s", req->accountid, error_message(r)); @@ -6210,9 +6207,7 @@ static int jmap_calendarevent_set(struct jmap_req *req) /* Open notifications mailbox, but continue even on error. */ r = jmap_create_notify_collection(req->accountid, ¬ifmb); - if (!r) { - r = jmap_openmbox(req, notifmb->name, ¬ifmbox, 1); - } + if (!r) r = mailbox_open_iwl(notifmb->name, ¬ifmbox); if (r) { xsyslog(LOG_WARNING, "can not open jmapnotify collection", "accountid=%s error=%s", req->accountid, error_message(r)); @@ -6696,7 +6691,7 @@ static int eventquery_cb(void *vrock, struct caldav_jscal *jscal) /* Load iCalendar data for main event */ if (!rock->mailbox || strcmp(mailbox_name(rock->mailbox), mbentry->name)) { mailbox_close(&rock->mailbox); - r = jmap_openmbox(req, mbentry->name, &rock->mailbox, 0); + r = mailbox_open_irl(mbentry->name, &rock->mailbox); if (r) goto done; } match->ical = caldav_record_to_ical(rock->mailbox, &jscal->cdata, req->userid, NULL); @@ -6936,7 +6931,7 @@ static int eventquery_textsearch_run(jmap_req_t *req, if (expandrecur) { if (!mailbox || strcmp(mailbox_name(mailbox), mbentry->name)) { mailbox_close(&mailbox); - r = jmap_openmbox(req, mbentry->name, &mailbox, 0); + r = mailbox_open_irl(mbentry->name, &mailbox); if (r) goto done; } } @@ -7633,7 +7628,7 @@ static void _calendarevent_copy(jmap_req_t *req, } /* Read source event */ - r = jmap_openmbox(req, mbentry->name, &src_mbox, /*rw*/0); + r = mailbox_open_irl(mbentry->name, &src_mbox); if (r) goto done; src_ical = caldav_record_to_ical(src_mbox, cdata, req->userid, &schedule_addresses); if (!src_ical) { @@ -7711,9 +7706,7 @@ static int jmap_calendarevent_copy(struct jmap_req *req) /* Open notifications mailbox, but continue even on error. */ int r = jmap_create_notify_collection(req->accountid, ¬ifmb); - if (!r) { - r = jmap_openmbox(req, notifmb->name, ¬ifmbox, 1); - } + if (!r) r = mailbox_open_iwl(notifmb->name, ¬ifmbox); if (r) { xsyslog(LOG_WARNING, "can not open jmapnotify collection", "accountid=%s error=%s", req->accountid, error_message(r)); @@ -8015,12 +8008,8 @@ static int jmap_calendarevent_participantreply(struct jmap_req *req) } /* Open mailbox for reading */ - r = jmap_openmbox(req, mbentry->name, &mbox, 0); - if (r) { - syslog(LOG_ERR, "jmap_openmbox(req, %s) failed: %s", - mbentry->name, error_message(r)); - goto done; - } + r = mailbox_open_irl(mbentry->name, &mbox); + if (r) goto done; /* Fetch index record for the resource. */ struct index_record record = { }; @@ -8449,7 +8438,7 @@ static int principal_state_init(jmap_req_t *req, SHA1_CTX *sha1) SHA1Init(sha1); char *calhomename = caldav_mboxname(req->userid, NULL); struct mailbox *mbox = NULL; - int r = jmap_openmbox(req, calhomename, &mbox, 0); + int r = mailbox_open_irl(calhomename, &mbox); if (!r) { struct buf buf = BUF_INITIALIZER; buf_printf(&buf, "%s" MODSEQ_FMT, req->userid, mailbox_foldermodseq(mbox)); @@ -9157,7 +9146,7 @@ static int jmap_principal_set(struct jmap_req *req) if ((tz = icaltimezone_get_cyrus_timezone_from_tzid(tzid))) { char *calhomename = caldav_mboxname(req->userid, NULL); struct mailbox *mbox = NULL; - int r = jmap_openmbox(req, calhomename, &mbox, 1); + int r = mailbox_open_iwl(calhomename, &mbox); if (!r) { annotate_state_t *astate = NULL; r = mailbox_get_annotate_state(mbox, 0, &astate); @@ -9402,7 +9391,7 @@ static int principal_getavailability_cb(void *vrock, struct caldav_jscal *jscal) } buf_reset(rock->buf); } - r = jmap_openmbox(rock->req, rock->mbentry->name, &rock->mbox, 0); + r = mailbox_open_irl(rock->mbentry->name, &rock->mbox); if (r) goto done; rock->floatingtz = caldav_get_calendar_tz(rock->mbentry->name, rock->req->userid); @@ -10002,7 +9991,7 @@ static void notif_get(struct jmap_req *req, struct mailbox *notifmbox = NULL; seqset_t *seenuids = NULL; - int r = jmap_openmbox(req, notifmb->name, ¬ifmbox, 0); + int r = mailbox_open_irl(notifmb->name, ¬ifmbox); if (r) { if (r != IMAP_MAILBOX_NONEXISTENT) { *err = jmap_server_error(r); @@ -10112,7 +10101,7 @@ static void notif_set(struct jmap_req *req, if (!json_array_size(set->destroy)) goto done; - int r = jmap_openmbox(req, notifmb->name, ¬ifmbox, 1); + int r = mailbox_open_iwl(notifmb->name, ¬ifmbox); if (r) { *err = jmap_server_error(r); goto done; @@ -10232,7 +10221,7 @@ static void notif_changes(struct jmap_req *req, struct dynarray *entries = dynarray_new(sizeof(struct notifsearch_entry)); struct mailbox *notifmbox = NULL; - int r = jmap_openmbox(req, notifmboxname, ¬ifmbox, 0); + int r = mailbox_open_irl(notifmboxname, ¬ifmbox); if (r) { if (r == IMAP_MAILBOX_NONEXISTENT) { changes->new_modseq = statemodseq; @@ -10826,7 +10815,7 @@ static int jmap_sharenotification_query(struct jmap_req *req) if (!r) { static int needrights = JACL_READITEMS; if (jmap_hasrights_mbentry(req, notifmb, needrights)) { - r = jmap_openmbox(req, notifmb->name, ¬ifmbox, 0); + r = mailbox_open_irl(notifmb->name, ¬ifmbox); } else r = IMAP_PERMISSION_DENIED; } @@ -11288,7 +11277,7 @@ static int jmap_calendareventnotification_query(struct jmap_req *req) } char *notifmboxname = jmap_notifmboxname(req->accountid); - int r = jmap_openmbox(req, notifmboxname, ¬ifmbox, 0); + int r = mailbox_open_irl(notifmboxname, ¬ifmbox); free(notifmboxname); if (r == IMAP_MAILBOX_NONEXISTENT) { r = 0; @@ -11922,7 +11911,7 @@ static void calendarpreferences_set(struct jmap_req *req, goto done; } - r = jmap_openmbox(req, mbcalhome->name, &calhomembox, 1); + r = mailbox_open_iwl(mbcalhome->name, &calhomembox); if (r) { xsyslog(LOG_ERR, "can not open calendar home mailbox", "err=<%s>", error_message(r)); diff --git a/imap/jmap_contact.c b/imap/jmap_contact.c index 91c640955c..7cce1c294f 100644 --- a/imap/jmap_contact.c +++ b/imap/jmap_contact.c @@ -1282,7 +1282,7 @@ static void _contacts_set(struct jmap_req *req, unsigned kind, if (!mailbox || strcmp(mailbox_name(mailbox), mbentry->name)) { mailbox_close(&mailbox); - r = jmap_openmbox(req, mbentry->name, &mailbox, 1); + r = mailbox_open_iwl(mbentry->name, &mailbox); } mboxlist_entry_free(&mbentry); if (r) goto done; @@ -4046,7 +4046,7 @@ static int _contact_set_create(jmap_req_t *req, unsigned kind, json_t *jcard, /* we need to create and append a record */ if (!*mailbox || strcmp(mailbox_name(*mailbox), mboxname)) { mailbox_close(mailbox); - r = jmap_openmbox(req, mboxname, mailbox, 1); + r = mailbox_open_iwl(mboxname, mailbox); if (r == IMAP_MAILBOX_NONEXISTENT) { json_array_append_new(invalid, json_string("addressbookId")); r = 0; @@ -4180,7 +4180,7 @@ static int _contact_set_update(jmap_req_t *req, unsigned kind, json_array_append_new(invalid, json_string("addressbookId")); r = HTTP_FORBIDDEN; } - else if ((r = jmap_openmbox(req, mboxname, &newmailbox, 1))) { + else if ((r = mailbox_open_iwl(mboxname, &newmailbox))) { syslog(LOG_ERR, "IOERROR: failed to open %s", mboxname); } else { @@ -4203,7 +4203,7 @@ static int _contact_set_update(jmap_req_t *req, unsigned kind, if (!*mailbox || strcmp(mailbox_name(*mailbox), mbentry->name)) { mailbox_close(mailbox); - r = jmap_openmbox(req, mbentry->name, mailbox, 1); + r = mailbox_open_iwl(mbentry->name, mailbox); } if (r) { syslog(LOG_ERR, "IOERROR: failed to open %s", @@ -4448,7 +4448,7 @@ static void _contact_copy(jmap_req_t *req, } /* Read source event */ - r = jmap_openmbox(req, mbentry->name, &src_mbox, /*rw*/0); + r = mailbox_open_irl(mbentry->name, &src_mbox); if (r) goto done; struct index_record record; @@ -5081,12 +5081,8 @@ static int setaddressbook_writeprops(jmap_req_t *req, if (!jmap_hasrights(req, mboxname, JACL_READITEMS) && !ignore_acl) return IMAP_MAILBOX_NONEXISTENT; - r = jmap_openmbox(req, mboxname, &mbox, 1); - if (r) { - syslog(LOG_ERR, "jmap_openmbox(req, %s) failed: %s", - mboxname, error_message(r)); - return r; - } + r = mailbox_open_iwl(mboxname, &mbox); + if (r) return r; r = mailbox_get_annotate_state(mbox, 0, &astate); if (r) { @@ -11110,7 +11106,7 @@ static int _card_set_create(jmap_req_t *req, /* we need to create and append a record */ if (!*mailbox || strcmp(mailbox_name(*mailbox), mboxname)) { mailbox_close(mailbox); - r = jmap_openmbox(req, mboxname, mailbox, 1); + r = mailbox_open_iwl(mboxname, mailbox); if (r == IMAP_MAILBOX_NONEXISTENT) { json_array_append_new(invalid, json_string("addressbookId")); r = 0; @@ -11267,7 +11263,7 @@ static int _card_set_update(jmap_req_t *req, unsigned kind, json_array_append_new(invalid, json_string("addressBookId")); r = HTTP_FORBIDDEN; } - else if ((r = jmap_openmbox(req, mboxname, &newmailbox, 1))) { + else if ((r = mailbox_open_iwl(mboxname, &newmailbox))) { syslog(LOG_ERR, "IOERROR: failed to open %s", mboxname); } else { @@ -11290,7 +11286,7 @@ static int _card_set_update(jmap_req_t *req, unsigned kind, if (!*mailbox || strcmp(mailbox_name(*mailbox), mbentry->name)) { mailbox_close(mailbox); - r = jmap_openmbox(req, mbentry->name, mailbox, 1); + r = mailbox_open_iwl(mbentry->name, mailbox); } if (r) { syslog(LOG_ERR, "IOERROR: failed to open %s", @@ -11721,7 +11717,8 @@ static int jmap_contact_getblob(jmap_req_t *req, jmap_getblob_context_t *ctx) } /* Open mailbox, we need it now */ - if ((r = jmap_openmbox(req, mbentry->name, &mailbox, 0))) { + r = mailbox_open_irl(mbentry->name, &mailbox); + if (r) { ctx->errstr = error_message(r); res = HTTP_SERVER_ERROR; goto done; @@ -11876,7 +11873,8 @@ static int jmap_contact_getblob(jmap_req_t *req, jmap_getblob_context_t *ctx) } /* Open mailbox, we need it now */ - if ((r = jmap_openmbox(req, mbentry->name, &mailbox, 0))) { + r = mailbox_open_iwl(mbentry->name, &mailbox); + if (r) { ctx->errstr = error_message(r); res = HTTP_SERVER_ERROR; goto done; diff --git a/imap/jmap_core.c b/imap/jmap_core.c index 8e6016bece..060cef47c1 100644 --- a/imap/jmap_core.c +++ b/imap/jmap_core.c @@ -832,7 +832,7 @@ static int jmap_blob_lookup(jmap_req_t *req) r = IMAP_PERMISSION_DENIED; } else { - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); if (r) { syslog(LOG_ERR, "jmap_blob_get: can't open mailbox %s: %s", mbentry->name, error_message(r)); diff --git a/imap/jmap_ical.c b/imap/jmap_ical.c index d1b93f7fce..1b6d451653 100644 --- a/imap/jmap_ical.c +++ b/imap/jmap_ical.c @@ -440,8 +440,9 @@ HIDDEN int jmapical_context_open_attachments(struct jmapical_ctx *jmapctx) if (!jmapctx->attachments.mbox) { char *mboxname = caldav_mboxname(req->accountid, MANAGED_ATTACH); - int r = jmap_openmbox(req, mboxname, &jmapctx->attachments.mbox, - jmapctx->attachments.lock); + int rw = jmapctx->attachments.lock; + int r = rw ? mailbox_open_iwl(mboxname, &jmapctx->attachments.mbox) + : mailbox_open_irl(mboxname, &jmapctx->attachments.mbox); if (r) { xsyslog(LOG_ERR, "can't open attachments", "mboxname=<%s> err<%s>", mboxname, error_message(r)); diff --git a/imap/jmap_mail.c b/imap/jmap_mail.c index 23a5ec8445..7b37f9b1fe 100644 --- a/imap/jmap_mail.c +++ b/imap/jmap_mail.c @@ -274,7 +274,8 @@ static struct email_sortfield email_sortfields[] = { #define jmap_openmbox_by_guidrec(req, rec, mbox, rw) \ ((rec->version > CONV_GUIDREC_BYNAME_VERSION) ? \ jmap_openmbox_by_uniqueid(req, conv_guidrec_uniqueid(rec), mbox, rw) : \ - jmap_openmbox(req, conv_guidrec_mboxname(rec), mbox, rw)) + rw ? mailbox_open_iwl(conv_guidrec_mboxname(rec), mbox) \ + : mailbox_open_irl(conv_guidrec_mboxname(rec), mbox)) #define JMAP_MAIL_MAX_MAILBOXES_PER_EMAIL 20 @@ -756,7 +757,7 @@ static int _email_mailboxes_cb(const conv_guidrec_t *rec, void *rock) return 0; } - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); mboxlist_entry_free(&mbentry); if (r) goto done; @@ -1081,7 +1082,7 @@ static int _email_find_cb(const conv_guidrec_t *rec, void *rock) int r = 0; struct mailbox *mbox = NULL; - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); if (r) { // we want to keep looking and see if we can find a mailbox we can open syslog(LOG_ERR, "IOERROR: email_find_cb failed to open %s: %s", @@ -3862,10 +3863,10 @@ static int guidsearch_run_xapian(jmap_req_t *req, struct emailsearch *search, mbname_t *mbname = mbname_from_userid(req->accountid); mbname_push_boxes(mbname, config_getstring(IMAPOPT_JMAPUPLOADFOLDER)); - int r = jmap_openmbox(req, mbname_intname(mbname), &mbox, 0); + int r = mailbox_open_irl(mbname_intname(mbname), &mbox); if (r == IMAP_MAILBOX_NONEXISTENT) { free(mbname_pop_boxes(mbname)); - r = jmap_openmbox(req, mbname_intname(mbname), &mbox, 0); + r = mailbox_open_irl(mbname_intname(mbname), &mbox); } if (r) goto done; @@ -5797,7 +5798,7 @@ static int _snippet_get(jmap_req_t *req, json_t *filter, continue; } - r = jmap_openmbox(req, mboxname, &mbox, 0); + r = mailbox_open_irl(mboxname, &mbox); if (r) goto doneloop; r = msgrecord_find(mbox, uid, &mr); @@ -6430,7 +6431,7 @@ static int _email_get_keywords_cb(const conv_guidrec_t *rec, void *vrock) } /* Fetch system flags */ - int r = jmap_openmbox(req, mbentry->name, &mbox, 0); + int r = mailbox_open_irl(mbentry->name, &mbox); mboxlist_entry_free(&mbentry); if (r) return r; @@ -8148,7 +8149,7 @@ static void jmap_email_get_full(jmap_req_t *req, struct jmap_get *get, struct em uint32_t uid; int r = jmap_email_find(req, NULL, id, &mboxname, &uid); if (!r) { - r = jmap_openmbox(req, mboxname, &mbox, 0); + r = mailbox_open_irl(mboxname, &mbox); if (!r) { r = msgrecord_find(mbox, uid, &mr); if (!r) { @@ -8836,7 +8837,7 @@ static void _email_append(jmap_req_t *req, } /* Create the message in the destination mailbox */ - r = jmap_openmbox(req, mboxname, &mbox, 1); + r = mailbox_open_iwl(mboxname, &mbox); if (r) goto done; if (sourcefile) { @@ -8980,7 +8981,7 @@ static void _email_append(jmap_req_t *req, if (!strcmp(mboxname, dstname)) continue; - r = jmap_openmbox(req, dstname, &dst, 1); + r = mailbox_open_iwl(dstname, &dst); if (r) goto done; r = _copy_msgrecord(httpd_authstate, req->userid, &jmap_namespace, @@ -12400,7 +12401,7 @@ static int _email_bulkupdate_open(jmap_req_t *req, struct email_bulkupdate *bulk else if (!mbentry) { r = IMAP_MAILBOX_NONEXISTENT; } - if (!r) r = jmap_openmbox(req, mboxrec->mboxname, &mbox, /*rw*/1); + if (!r) r = mailbox_open_iwl(mboxrec->mboxname, &mbox); if (r) { int j; for (j = 0; j < ptrarray_size(&mboxrec->uidrecs); j++) { @@ -12475,7 +12476,7 @@ static int _email_bulkupdate_open(jmap_req_t *req, struct email_bulkupdate *bulk r = mboxlist_promote_intermediary(mbentry->name); mboxname_release(&namespacelock); } - if (!r) jmap_openmbox(req, mbentry->name, &mbox, /*rw*/1); + if (!r) r = mailbox_open_iwl(mbentry->name, &mbox); } if (mbox) { if (!hash_lookup(mailbox_uniqueid(mbox), &bulk->plans_by_mbox_id)) { @@ -13085,7 +13086,7 @@ static void _email_destroy_bulk(jmap_req_t *req, struct email_mboxrec *mboxrec = ptrarray_nth(mboxrecs, i); struct mailbox *mbox = NULL; int j; - int r = jmap_openmbox(req, mboxrec->mboxname, &mbox, 1); + int r = mailbox_open_iwl(mboxrec->mboxname, &mbox); if (!r) { /* Expunge messages one by one, marking any failed/expunged message */ _email_multiexpunge(req, mbox, &mboxrec->uidrecs, not_destroyed, destroyed); @@ -13727,7 +13728,6 @@ struct _email_exists_rock { static int _email_exists_cb(const conv_guidrec_t *rec, void *rock) { struct _email_exists_rock *data = (struct _email_exists_rock*) rock; - jmap_req_t *req = data->req; struct mailbox *mbox = NULL; msgrecord_t *mr = NULL; uint32_t internal_flags; @@ -13740,7 +13740,7 @@ static int _email_exists_cb(const conv_guidrec_t *rec, void *rock) goto done; } if (rec->version < 1) { - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); if (r) goto done; r = msgrecord_find(mbox, rec->uid, &mr); @@ -13789,7 +13789,7 @@ static int _email_copy_pickrecord_cb(const conv_guidrec_t *rec, void *vrock) } /* Lookup record */ - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); mboxlist_entry_free(&mbentry); if (r) goto done; r = msgrecord_find(mbox, rec->uid, &mr); @@ -13964,7 +13964,7 @@ static void _email_copy(jmap_req_t *req, json_t *copy_email, } if (!r) { struct mailbox *dst_mbox = NULL; - r = jmap_openmbox(req, dst_mboxname, &dst_mbox, /*rw*/1); + r = mailbox_open_iwl(dst_mboxname, &dst_mbox); if (!r) { r = _copy_msgrecord(httpd_authstate, req->accountid, &jmap_namespace, src_mbox, dst_mbox, src_mr); @@ -14271,7 +14271,8 @@ static int jmap_emailheader_getblob(jmap_req_t *req, jmap_getblob_context_t *ctx else if (mimetype) buf_setcstr(&ctx->content_type, mimetype); /* Open mailbox, we need it now */ - if ((r = jmap_openmbox(req, mboxname, &mailbox, 0))) { + r = mailbox_open_irl(mboxname, &mailbox); + if (r) { ctx->errstr = error_message(r); res = HTTP_SERVER_ERROR; goto done; diff --git a/imap/jmap_mail_submission.c b/imap/jmap_mail_submission.c index 8b6da09966..4f23bb6314 100644 --- a/imap/jmap_mail_submission.c +++ b/imap/jmap_mail_submission.c @@ -667,7 +667,7 @@ static void _emailsubmission_create(jmap_req_t *req, } /* Open the mailboxes */ - r = jmap_openmbox(req, mboxname, &mbox, 1); + r = mailbox_open_iwl(mboxname, &mbox); if (r) goto done; /* Load the message */ @@ -1329,7 +1329,7 @@ static int jmap_emailsubmission_get(jmap_req_t *req) goto done; } else { - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); } mboxlist_entry_free(&mbentry); if (r) goto done; @@ -1509,7 +1509,7 @@ static int jmap_emailsubmission_set(jmap_req_t *req) goto done; } - r = jmap_openmbox(req, mbentry->name, &submbox, 1); + r = mailbox_open_iwl(mbentry->name, &submbox); assert(submbox); mboxlist_entry_free(&mbentry); if (r) goto done; @@ -1682,7 +1682,7 @@ static int jmap_emailsubmission_changes(jmap_req_t *req) goto done; } - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); mboxlist_entry_free(&mbentry); if (r) goto done; @@ -2147,7 +2147,7 @@ static int jmap_emailsubmission_query(jmap_req_t *req) goto done; } - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); mboxlist_entry_free(&mbentry); if (r) goto done; diff --git a/imap/jmap_mailbox.c b/imap/jmap_mailbox.c index baf42a5993..b6bb5603a6 100644 --- a/imap/jmap_mailbox.c +++ b/imap/jmap_mailbox.c @@ -2745,7 +2745,7 @@ static void _mbox_update(jmap_req_t *req, struct mboxset_args *args, struct mailbox *mbox = NULL; uint32_t newopts; - r = jmap_openmbox(req, mboxname, &mbox, 1); + r = mailbox_open_iwl(mboxname, &mbox); if (r) goto done; if (args->shareWith) { @@ -2824,12 +2824,12 @@ static int _mbox_on_destroy_move(jmap_req_t *req, r = IMAP_NOTFOUND; goto done; } - r = jmap_openmbox(req, src_mbentry->name, &src_mbox, 0); + r = mailbox_open_irl(src_mbentry->name, &src_mbox); if (r) { syslog(LOG_ERR, "%s: can't open %s", __func__, src_mbentry->name); goto done; } - r = jmap_openmbox(req, dst_mbentry->name, &dst_mbox, 1); + r = mailbox_open_iwl(dst_mbentry->name, &dst_mbox); if (r) { syslog(LOG_ERR, "%s: can't open %s", __func__, dst_mbentry->name); goto done; @@ -2971,7 +2971,7 @@ static void _mbox_destroy(jmap_req_t *req, const char *mboxid, struct mailbox *mbox = NULL; struct mailbox_iter *iter = NULL; - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); if (r) goto done; iter = mailbox_iter_init(mbox, 0, ITER_SKIP_EXPUNGED); if (mailbox_iter_step(iter) != NULL) { diff --git a/imap/jmap_mdn.c b/imap/jmap_mdn.c index f745ec4ac7..f947be80de 100644 --- a/imap/jmap_mdn.c +++ b/imap/jmap_mdn.c @@ -275,7 +275,7 @@ static json_t *generate_mdn(struct jmap_req *req, } /* Open the mailbox */ - r = jmap_openmbox(req, mboxname, &mbox, 1); + r = mailbox_open_iwl(mboxname, &mbox); if (r) goto done; /* Load the message */ diff --git a/imap/jmap_notes.c b/imap/jmap_notes.c index 2a473305b7..40697d54ba 100644 --- a/imap/jmap_notes.c +++ b/imap/jmap_notes.c @@ -472,7 +472,7 @@ static int jmap_note_get(jmap_req_t *req) rights = jmap_myrights_mbentry(req, mbentry); - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); mboxlist_entry_free(&mbentry); if (r) goto done; @@ -867,7 +867,7 @@ static int jmap_note_set(jmap_req_t *req) rights = jmap_myrights_mbentry(req, mbentry); - r = jmap_openmbox(req, mbentry->name, &mbox, 1); + r = mailbox_open_iwl(mbentry->name, &mbox); assert(mbox); mboxlist_entry_free(&mbentry); if (r) goto done; @@ -999,7 +999,7 @@ static int jmap_note_changes(jmap_req_t *req) goto done; } - r = jmap_openmbox(req, mbentry->name, &mbox, 0); + r = mailbox_open_irl(mbentry->name, &mbox); mboxlist_entry_free(&mbentry); r = mailbox_user_flag(mbox, DFLAG_UNBIND, &userflag, 0); From d594c208fb7e6fb338ac24632942ecfd7ecd954c Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 14 Nov 2023 15:10:43 +1100 Subject: [PATCH 16/38] mailbox: use the lockname correctly to look up existing mailboxes --- imap/mailbox.c | 104 ++++++++++++++++++++++++++----------------------- 1 file changed, 55 insertions(+), 49 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index cd24ee06f8..0142db3d23 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -236,7 +236,7 @@ EXPORTED int open_mailboxes_exist() return open_mailboxes ? 1 : 0; } -static struct mailbox *create_listitem() +static struct mailbox *create_listitem(const char *lockname) { struct mailbox *item = xzmalloc(sizeof(struct mailbox)); zeromailbox(*item); @@ -245,6 +245,7 @@ static struct mailbox *create_listitem() open_mailboxes = item; item->refcount = 1; + item->lockname = xstrdup(lockname); gettimeofday(&item->starttime, 0); #if defined ENABLE_OBJECTSTORE @@ -975,19 +976,38 @@ static int mailbox_open_advanced(const char *name, const mbentry_t *mbe, struct mailbox **mailboxptr) { + int r = 0; assert(*mailboxptr == NULL); + struct mboxlock *local_namespacelock = NULL; + + // lock the user namespace FIRST before anything else + char *userid = mboxname_to_userid(name); + int haslock = user_isnamespacelocked(userid); + if (haslock) { + if (index_locktype & LOCK_EXCLUSIVE && !(haslock & LOCK_EXCLUSIVE)) + r = IMAP_MAILBOX_LOCKED; + } + else { + local_namespacelock = user_namespacelock_full(userid, index_locktype); + } + free(userid); + if (r) return r; - struct mailbox *mailbox = find_listitem(name); mbentry_t *mbentry = NULL; - int r = 0; + if (mbe) mbentry = mboxlist_entry_copy(mbe); + else r = mboxlist_lookup_allow_all(name, &mbentry, NULL); + if (r) { + mboxname_release(&local_namespacelock); + return r; + } + uint32_t legacy_dirs = (mbentry->mbtype & MBTYPE_LEGACY_DIRS); + const char *lockname = legacy_dirs ? name : mbentry->uniqueid; + struct mailbox *mailbox = find_listitem(lockname); /* already open? just use this one */ if (mailbox) { - /* can't reuse an exclusive locked mailbox */ - if (mailbox->namelock->locktype & LOCK_EXCLUSIVE) - return IMAP_MAILBOX_LOCKED; - + mboxlist_entry_free(&mbentry); /* can't promote a readonly index */ if (mailbox->index_locktype == LOCK_SHARED && locktype == LOCK_EXCLUSIVE) return IMAP_MAILBOX_LOCKED; @@ -999,21 +1019,7 @@ static int mailbox_open_advanced(const char *name, goto done; } - mailbox = create_listitem(); - - // lock the user namespace FIRST before the mailbox namespace - char *userid = mboxname_to_userid(name); - int haslock = user_isnamespacelocked(userid); - if (haslock) { - if (index_locktype & LOCK_EXCLUSIVE) assert(haslock & LOCK_EXCLUSIVE); - } - else { - mailbox->local_namespacelock = user_namespacelock_full(userid, index_locktype); - } - free(userid); - - if (mbe) mbentry = mboxlist_entry_copy(mbe); - else r = mboxlist_lookup_allow_all(name, &mbentry, NULL); + mailbox = create_listitem(lockname); /* pre-check for some conditions which mean that we don't want to go ahead and open this mailbox */ @@ -1030,7 +1036,7 @@ static int mailbox_open_advanced(const char *name, r = IMAP_MAILBOX_NONEXISTENT; if (r) { - mboxname_release(&mailbox->local_namespacelock); + mboxname_release(&local_namespacelock); mboxlist_entry_free(&mbentry); remove_listitem(mailbox); return r; @@ -1045,8 +1051,6 @@ static int mailbox_open_advanced(const char *name, "mboxname=<%s>", name); } - uint32_t legacy_dirs = (mbentry->mbtype & MBTYPE_LEGACY_DIRS); - mailbox->lockname = xstrdup(legacy_dirs ? name : mbentry->uniqueid); r = mboxname_lock(mailbox->lockname, &mailbox->namelock, locktype); if (r) { /* locked is not an error - just means we asked for NONBLOCKING */ @@ -1054,7 +1058,7 @@ static int mailbox_open_advanced(const char *name, xsyslog(LOG_ERR, "IOERROR: lock failed", "mailbox=<%s> error=<%s>", name, error_message(r)); - mboxname_release(&mailbox->local_namespacelock); + mboxname_release(&local_namespacelock); mboxlist_entry_free(&mbentry); remove_listitem(mailbox); return r; @@ -1079,6 +1083,7 @@ static int mailbox_open_advanced(const char *name, cleanup_stale_expunged(mailbox); done: + if (local_namespacelock) mailbox->local_namespacelock = local_namespacelock; if (r) mailbox_close(&mailbox); else *mailboxptr = mailbox; @@ -5795,22 +5800,19 @@ EXPORTED int mailbox_create(const char *name, /* if we already have this name open then that's an error too */ uint32_t legacy_dirs = (mbtype & MBTYPE_LEGACY_DIRS); - mailbox = find_listitem(legacy_dirs ? name : uniqueid); + const char *lockname = legacy_dirs ? name : uniqueid; + mailbox = find_listitem(lockname); if (mailbox) return IMAP_MAILBOX_LOCKED; - - mailbox = create_listitem(); + mailbox = create_listitem(lockname); /* needs to be an exclusive namelock to create a mailbox */ char *userid = mboxname_to_userid(name); int haslock = user_isnamespacelocked(userid); - syslog(LOG_ERR, "CREATING MAILBOX %s", name); assert(haslock == LOCK_EXCLUSIVE); free(userid); - mailbox->lockname = xstrdup(legacy_dirs ? name : uniqueid); r = mboxname_lock(mailbox->lockname, &mailbox->namelock, LOCK_EXCLUSIVE); if (r) { - mboxname_release(&mailbox->local_namespacelock); remove_listitem(mailbox); return r; } @@ -6971,42 +6973,46 @@ static int mailbox_reconstruct_create(const char *name, struct mailbox **mbptr) struct mailbox *mailbox = NULL; int options = config_getint(IMAPOPT_MAILBOX_DEFAULT_OPTIONS) | OPT_POP3_NEW_UIDL; + struct mboxlock *local_namespacelock = NULL; mbentry_t *mbentry = NULL; - int r; - - /* make sure it's not already open. Very odd, since we already - * discovered it's not openable! */ - mailbox = find_listitem(name); - if (mailbox) return IMAP_MAILBOX_LOCKED; - - mailbox = create_listitem(); + int r = 0; // lock the user namespace FIRST before the mailbox namespace char *userid = mboxname_to_userid(name); if (userid) { int haslock = user_isnamespacelocked(userid); - if (haslock) { - assert(haslock != LOCK_SHARED); - } + if (haslock) { + if (!(haslock & LOCK_EXCLUSIVE)) + r = IMAP_MAILBOX_LOCKED; + } else { - int locktype = LOCK_EXCLUSIVE; - mailbox->local_namespacelock = user_namespacelock_full(userid, locktype); + local_namespacelock = user_namespacelock_full(userid, LOCK_EXCLUSIVE); } - free(userid); } + free(userid); + if (r) return r; /* Start by looking up current data in mailbox list */ /* XXX - no mboxlist entry? Can we recover? */ r = mboxlist_lookup(name, &mbentry, NULL); - if (r) goto done; + if (r) return r; + + /* make sure it's not already open. Very odd, since we already + * discovered it's not openable! */ + uint32_t legacy_dirs = (mbentry->mbtype & MBTYPE_LEGACY_DIRS); + const char *lockname = legacy_dirs ? name : mbentry->uniqueid; + mailbox = find_listitem(lockname); + if (mailbox) return IMAP_MAILBOX_LOCKED; + + mailbox = create_listitem(lockname); /* if we can't get an exclusive lock first try, there's something * racy going on! */ - uint32_t legacy_dirs = (mbentry->mbtype & MBTYPE_LEGACY_DIRS); - r = mboxname_lock(legacy_dirs ? name : mbentry->uniqueid, &mailbox->namelock, LOCK_EXCLUSIVE); + r = mboxname_lock(mailbox->lockname, &mailbox->namelock, LOCK_EXCLUSIVE); if (r) goto done; mailbox->mbentry = mboxlist_entry_copy(mbentry); + if (local_namespacelock) mailbox->local_namespacelock = local_namespacelock; syslog(LOG_NOTICE, "create new mailbox %s", name); From 2ea6c88e1d171fb3a4d3f9afa1ce4e719b215ca4 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Thu, 16 Nov 2023 08:46:36 +1100 Subject: [PATCH 17/38] Cassandane - spit out exact gdb command to use if cores found --- cassandane/Cassandane/Instance.pm | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cassandane/Cassandane/Instance.pm b/cassandane/Cassandane/Instance.pm index 352eaf40e5..9dbfe21f2f 100644 --- a/cassandane/Cassandane/Instance.pm +++ b/cassandane/Cassandane/Instance.pm @@ -1448,7 +1448,11 @@ sub _check_cores my $prog = _detect_core_program($core); xlog "Found core file $core"; - xlog " from program $prog" if defined $prog; + if (defined $prog) { + xlog " from program $prog"; + my ($bin) = $prog =~ m/^(\S+)/; # binary only + xlog " debug: sudo gdb $bin $core"; + } } closedir CORES; From aff8d76d19d2e082dcf7e25e72906cb5135637a6 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Fri, 17 Nov 2023 15:44:45 +1100 Subject: [PATCH 18/38] reconstruct: fix 'setversion' mailbox leak --- imap/reconstruct.c | 1 + 1 file changed, 1 insertion(+) diff --git a/imap/reconstruct.c b/imap/reconstruct.c index ce0fd4e52a..37d464a4d1 100644 --- a/imap/reconstruct.c +++ b/imap/reconstruct.c @@ -540,6 +540,7 @@ static int do_reconstruct(struct findall_data *data, void *rock) } free(extname); } + mailbox_close(&mailbox); mboxname_release(&namespacelock); return 0; } From 17b83c5bec81f4b93e2a81349b5e800b42a3a2c8 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Thu, 16 Nov 2023 08:47:22 +1100 Subject: [PATCH 19/38] Reconstruct: add a version downgrade test --- cassandane/Cassandane/Cyrus/Reconstruct.pm | 51 ++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/cassandane/Cassandane/Cyrus/Reconstruct.pm b/cassandane/Cassandane/Cyrus/Reconstruct.pm index a065cce54a..abc26bd30c 100644 --- a/cassandane/Cassandane/Cyrus/Reconstruct.pm +++ b/cassandane/Cassandane/Cyrus/Reconstruct.pm @@ -572,4 +572,55 @@ sub test_reconstruct_uniqueid_from_header_uuidmb $self->assert_str_equals("user\x1fcassandane", $hash->{N}); } +sub test_downgrade_upgrade +{ + my ($self) = @_; + + my $talk = $self->{store}->get_client(); + $self->{store}->_select(); + $self->assert_num_equals(1, $talk->uid()); + $self->{store}->set_fetch_attributes(qw(uid flags)); + + xlog $self, "Add two messages"; + my %msg; + $msg{A} = $self->make_message('Message A'); + $msg{A}->set_attributes(id => 1, + uid => 1, + flags => []); + $msg{B} = $self->make_message('Message B'); + $msg{B}->set_attributes(id => 2, + uid => 2, + flags => []); + $self->check_messages(\%msg); + + xlog $self, "Set \\Seen on message A"; + my $res = $talk->store('1', '+flags', '(\\Seen)'); + $self->assert_deep_equals({ '1' => { 'flags' => [ '\\Seen' ] }}, $res); + $msg{A}->set_attribute(flags => ['\\Seen']); + $self->check_messages(\%msg); + + xlog $self, "Clear \\Seen on message A"; + $res = $talk->store('1', '-flags', '(\\Seen)'); + $self->assert_deep_equals({ '1' => { 'flags' => [] }}, $res); + $msg{A}->set_attribute(flags => []); + $self->check_messages(\%msg); + + xlog $self, "Set \\Seen on message A again"; + $res = $talk->store('1', '+flags', '(\\Seen)'); + $self->assert_deep_equals({ '1' => { 'flags' => [ '\\Seen' ] }}, $res); + $msg{A}->set_attribute(flags => ['\\Seen']); + $self->check_messages(\%msg); + + for my $version (12, 14, 16, 'max') { + xlog $self, "Set to version $version"; + eval { $self->{instance}->run_command({ cyrus => 1 }, 'reconstruct', '-V', $version) }; + + xlog $self, "Reconnect, \\Seen should still be on message A"; + $self->{store}->disconnect(); + $self->{store}->connect(); + $self->{store}->_select(); + $self->check_messages(\%msg); + } +} + 1; From 02358868361a9cebe5956c24328048590242f51f Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Fri, 17 Nov 2023 15:55:25 +1100 Subject: [PATCH 20/38] mailbox: turn IOERROR into xsyslog (ellie review) --- imap/mailbox.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 0142db3d23..228addcacf 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -2651,8 +2651,9 @@ EXPORTED void mailbox_unlock_index(struct mailbox *mailbox, struct statusdata *s if (mailbox->local_cstate) { int r = conversations_commit(&mailbox->local_cstate); if (r) { - syslog(LOG_ERR, "IOERROR: Error committing to conversations database for mailbox %s: %s", - mailbox_name(mailbox), error_message(r)); + xsyslog(LOG_ERR, "IOERROR: Error committing to conversations database" + "mailbox=<%s> error=<%s>", + mailbox_name(mailbox), error_message(r)); } } From 30c3f0d27a73243f7586aa40c1b58067a67b550e Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 15 Nov 2023 10:17:58 +1100 Subject: [PATCH 21/38] mailbox: use is_readonly to track writeable status of index_fd --- imap/mailbox.c | 77 +++++++++++++++++++++++--------------------------- imap/mailbox.h | 2 +- 2 files changed, 36 insertions(+), 43 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 228addcacf..77935cfbae 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -905,6 +905,7 @@ static void mailbox_release_resources(struct mailbox *mailbox) map_free(&mailbox->index_base, &mailbox->index_len); xclose(mailbox->index_fd); mailbox->index_locktype = 0; /* lock was released by closing fd */ + mailbox->is_readonly = 0; /* no longer have a readonly fd */ /* release caches */ for (i = 0; i < mailbox->caches.count; i++) { @@ -919,18 +920,16 @@ static void mailbox_release_resources(struct mailbox *mailbox) */ static int mailbox_open_index(struct mailbox *mailbox, int index_locktype) { - const char *fname; - int openflags = index_locktype == LOCK_SHARED ? O_RDONLY : O_RDWR; - assert(!mailbox->index_locktype); mailbox_release_resources(mailbox); /* open and map the index file */ - fname = mailbox_meta_fname(mailbox, META_INDEX); + const char *fname = mailbox_meta_fname(mailbox, META_INDEX); if (!fname) return IMAP_MAILBOX_BADNAME; - mailbox->index_fd = open(fname, openflags, 0); + mailbox->is_readonly = (index_locktype == LOCK_SHARED) ? 1 : 0; + mailbox->index_fd = open(fname, mailbox->is_readonly ? O_RDONLY : O_RDWR, 0); if (mailbox->index_fd == -1) return IMAP_IOERROR; @@ -996,7 +995,29 @@ static int mailbox_open_advanced(const char *name, mbentry_t *mbentry = NULL; if (mbe) mbentry = mboxlist_entry_copy(mbe); else r = mboxlist_lookup_allow_all(name, &mbentry, NULL); + + /* pre-check for some conditions which mean that we don't want + to go ahead and open this mailbox */ + if (!r && mbentry->mbtype & MBTYPE_DELETED) + r = IMAP_MAILBOX_NONEXISTENT; + if (!r && mbentry->mbtype & MBTYPE_MOVING) + r = IMAP_MAILBOX_MOVED; + if (!r && mbentry->mbtype & MBTYPE_INTERMEDIATE) + r = IMAP_MAILBOX_NONEXISTENT; + if (!r && !mbentry->partition) + r = IMAP_MAILBOX_NONEXISTENT; + + /* XXX can we even get here for remote mbentries? */ + if (!r && !mbentry->uniqueid && mbentry_is_local_mailbox(mbentry)) { + /* Theoretically it shouldn't be possible for an mbentry to not + * have a uniqueid... so if it happens, complain loudly. + */ + xsyslog(LOG_ERR, "mbentry has no uniqueid, needs reconstruct", + "mboxname=<%s>", name); + } + if (r) { + mboxlist_entry_free(&mbentry); mboxname_release(&local_namespacelock); return r; } @@ -1007,11 +1028,18 @@ static int mailbox_open_advanced(const char *name, /* already open? just use this one */ if (mailbox) { + if (local_namespacelock) mailbox->local_namespacelock = local_namespacelock; mboxlist_entry_free(&mbentry); /* can't promote a readonly index */ - if (mailbox->index_locktype == LOCK_SHARED && locktype == LOCK_EXCLUSIVE) + if (mailbox->index_locktype == LOCK_SHARED && index_locktype != LOCK_SHARED) return IMAP_MAILBOX_LOCKED; + /* if we have a readonly FD, we need to reopen */ + if (mailbox->is_readonly && index_locktype != LOCK_SHARED) { + r = mailbox_open_index(mailbox, index_locktype); + if (r) return r; + } + mailbox->refcount++; if (!mailbox->index_locktype) goto lockindex; @@ -1021,36 +1049,6 @@ static int mailbox_open_advanced(const char *name, mailbox = create_listitem(lockname); - /* pre-check for some conditions which mean that we don't want - to go ahead and open this mailbox */ - if (!r && mbentry->mbtype & MBTYPE_DELETED) - r = IMAP_MAILBOX_NONEXISTENT; - - if (!r && mbentry->mbtype & MBTYPE_MOVING) - r = IMAP_MAILBOX_MOVED; - - if (!r && mbentry->mbtype & MBTYPE_INTERMEDIATE) - r = IMAP_MAILBOX_NONEXISTENT; - - if (!r && !mbentry->partition) - r = IMAP_MAILBOX_NONEXISTENT; - - if (r) { - mboxname_release(&local_namespacelock); - mboxlist_entry_free(&mbentry); - remove_listitem(mailbox); - return r; - } - - /* XXX can we even get here for remote mbentries? */ - if (!mbentry->uniqueid && mbentry_is_local_mailbox(mbentry)) { - /* Theoretically it shouldn't be possible for an mbentry to not - * have a uniqueid... so if it happens, complain loudly. - */ - xsyslog(LOG_ERR, "mbentry has no uniqueid, needs reconstruct", - "mboxname=<%s>", name); - } - r = mboxname_lock(mailbox->lockname, &mailbox->namelock, locktype); if (r) { /* locked is not an error - just means we asked for NONBLOCKING */ @@ -1063,6 +1061,7 @@ static int mailbox_open_advanced(const char *name, remove_listitem(mailbox); return r; } + if (local_namespacelock) mailbox->local_namespacelock = local_namespacelock; if (!mbentry->name) mbentry->name = xstrdup(name); mailbox->mbentry = mbentry; @@ -1083,7 +1082,6 @@ static int mailbox_open_advanced(const char *name, cleanup_stale_expunged(mailbox); done: - if (local_namespacelock) mailbox->local_namespacelock = local_namespacelock; if (r) mailbox_close(&mailbox); else *mailboxptr = mailbox; @@ -2484,9 +2482,6 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) } if (locktype == LOCK_EXCLUSIVE) { - /* we can't upgrade a readonly mailbox */ - if (mailbox->is_readonly) - return IMAP_MAILBOX_LOCKED; r = lock_blocking(mailbox->index_fd, index_fname); } else if (locktype == LOCK_SHARED) { @@ -2523,7 +2518,6 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) } mailbox->index_locktype = locktype; - mailbox->is_readonly = (locktype == LOCK_SHARED) ? 1 : 0; gettimeofday(&mailbox->starttime, 0); r = stat(header_fname, &sbuf); @@ -2638,7 +2632,6 @@ EXPORTED void mailbox_unlock_index(struct mailbox *mailbox, struct statusdata *s "mailbox=<%s>", mailbox_name(mailbox)); mailbox->index_locktype = 0; - mailbox->is_readonly = 0; gettimeofday(&endtime, 0); timediff = timesub(&mailbox->starttime, &endtime); diff --git a/imap/mailbox.h b/imap/mailbox.h index 5f6128ac3f..578dc39926 100644 --- a/imap/mailbox.h +++ b/imap/mailbox.h @@ -257,7 +257,7 @@ struct mailbox { size_t index_len; /* mapped size */ int index_locktype; /* 0 = none, 1 = shared, 2 = exclusive */ - int is_readonly; /* this matches locktype but COULD be true even if locktype == exclusive */ + int is_readonly; /* tells us whether the index_fd is opened RW or RO */ ino_t header_file_ino; bit32 header_file_crc; From 51c975b09e068a355ba9ee8b6c70be937ae31000 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 15 Nov 2023 11:29:42 +1100 Subject: [PATCH 22/38] mailbox: we're is_readonly if we're locked readonly as WELL as if we're opened readonly --- imap/mailbox.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 77935cfbae..4aa656ff75 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -648,7 +648,8 @@ static struct mappedfile *mailbox_cachefile(struct mailbox *mailbox, else fname = mailbox_meta_fname(mailbox, META_CACHE); - return cache_getfile(&mailbox->caches, fname, mailbox->is_readonly, mailbox->i.generation_no); + int is_readonly = mailbox->is_readonly || mailbox->index_locktype == LOCK_SHARED; + return cache_getfile(&mailbox->caches, fname, is_readonly, mailbox->i.generation_no); } static struct mappedfile *repack_cachefile(struct mailbox_repack *repack, @@ -4318,7 +4319,8 @@ EXPORTED struct conversations_state *mailbox_get_cstate_full(struct mailbox *mai /* open the conversations DB - don't bother checking return code since it'll * only be set if it opens successfully, and we can only return NULL or an * object */ - conversations_open_mbox(mailbox_name(mailbox), mailbox->is_readonly, &mailbox->local_cstate); + int is_readonly = mailbox->is_readonly || mailbox->index_locktype == LOCK_SHARED; + conversations_open_mbox(mailbox_name(mailbox), is_readonly, &mailbox->local_cstate); return mailbox->local_cstate; } From 413433e30db95c00fb5e6b4fddc2dbeab8a96f21 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 15 Nov 2023 11:30:02 +1100 Subject: [PATCH 23/38] mailbox: use more bitwise checks on locks --- imap/mailbox.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 4aa656ff75..61b0291514 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -580,8 +580,8 @@ EXPORTED char *mailbox_cache_get_env(struct mailbox *mailbox, EXPORTED int mailbox_index_islocked(struct mailbox *mailbox, int write) { - if (mailbox->index_locktype == LOCK_EXCLUSIVE) return 1; - if (mailbox->index_locktype == LOCK_SHARED && !write) return 1; + if (mailbox->index_locktype & LOCK_EXCLUSIVE) return 1; + if (mailbox->index_locktype & LOCK_SHARED && !write) return 1; return 0; } @@ -929,7 +929,7 @@ static int mailbox_open_index(struct mailbox *mailbox, int index_locktype) if (!fname) return IMAP_MAILBOX_BADNAME; - mailbox->is_readonly = (index_locktype == LOCK_SHARED) ? 1 : 0; + mailbox->is_readonly = (index_locktype & LOCK_SHARED) ? 1 : 0; mailbox->index_fd = open(fname, mailbox->is_readonly ? O_RDONLY : O_RDWR, 0); if (mailbox->index_fd == -1) return IMAP_IOERROR; @@ -954,14 +954,15 @@ static int mailbox_relock(struct mailbox *mailbox, int locktype, int index_lockt if (userid) { int haslock = user_isnamespacelocked(userid); if (haslock) { - if (haslock == LOCK_SHARED) - assert(index_locktype == LOCK_SHARED); + if ((haslock & LOCK_SHARED) && (index_locktype & LOCK_EXCLUSIVE)) + r = IMAP_MAILBOX_LOCKED; } else { mailbox->local_namespacelock = user_namespacelock_full(userid, index_locktype); } free(userid); } + if (r) return r; r = mailbox_lock_index_internal(mailbox, index_locktype); return r; } @@ -984,7 +985,7 @@ static int mailbox_open_advanced(const char *name, char *userid = mboxname_to_userid(name); int haslock = user_isnamespacelocked(userid); if (haslock) { - if (index_locktype & LOCK_EXCLUSIVE && !(haslock & LOCK_EXCLUSIVE)) + if ((haslock & LOCK_SHARED) && (index_locktype & LOCK_EXCLUSIVE)) r = IMAP_MAILBOX_LOCKED; } else { @@ -1032,7 +1033,7 @@ static int mailbox_open_advanced(const char *name, if (local_namespacelock) mailbox->local_namespacelock = local_namespacelock; mboxlist_entry_free(&mbentry); /* can't promote a readonly index */ - if (mailbox->index_locktype == LOCK_SHARED && index_locktype != LOCK_SHARED) + if ((mailbox->index_locktype & LOCK_SHARED) && (index_locktype & LOCK_EXCLUSIVE)) return IMAP_MAILBOX_LOCKED; /* if we have a readonly FD, we need to reopen */ @@ -2457,7 +2458,7 @@ EXPORTED int mailbox_find_index_record(struct mailbox *mailbox, uint32_t uid, /* * Lock the index file for 'mailbox'. Reread index file header if necessary. */ -static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) +static int mailbox_lock_index_internal(struct mailbox *mailbox, int index_locktype) { struct stat sbuf; int r = 0; @@ -2468,7 +2469,7 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) // if we're already locked, don't need to lock again! if (mailbox->index_locktype) { - if (mailbox->index_locktype == LOCK_SHARED && locktype != LOCK_SHARED) + if ((mailbox->index_locktype & LOCK_SHARED) && (index_locktype & LOCK_EXCLUSIVE)) return IMAP_MAILBOX_LOCKED; return 0; } @@ -2477,15 +2478,15 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) if (userid) { if (!user_isnamespacelocked(userid)) { free(userid); - return mailbox_relock(mailbox, LOCK_SHARED, locktype); + return mailbox_relock(mailbox, LOCK_SHARED, index_locktype); } free(userid); } - if (locktype == LOCK_EXCLUSIVE) { + if (index_locktype == LOCK_EXCLUSIVE) { r = lock_blocking(mailbox->index_fd, index_fname); } - else if (locktype == LOCK_SHARED) { + else if (index_locktype == LOCK_SHARED) { r = lock_shared(mailbox->index_fd, index_fname); } else { @@ -2518,7 +2519,7 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) return IMAP_IOERROR; } - mailbox->index_locktype = locktype; + mailbox->index_locktype = index_locktype; gettimeofday(&mailbox->starttime, 0); r = stat(header_fname, &sbuf); @@ -2565,11 +2566,11 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int locktype) return 0; } -EXPORTED int mailbox_lock_index(struct mailbox *mailbox, int locktype) +EXPORTED int mailbox_lock_index(struct mailbox *mailbox, int index_locktype) { int r = 0; - r = mailbox_lock_index_internal(mailbox, locktype); + r = mailbox_lock_index_internal(mailbox, index_locktype); if (r) return r; /* otherwise, sanity checks for regular use, but not for internal From 4d70a2fe1667807dbc4cb26127ae8f92774d9760 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 15 Nov 2023 11:36:21 +1100 Subject: [PATCH 24/38] jmap_contact: readlock for getblob --- imap/jmap_contact.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap/jmap_contact.c b/imap/jmap_contact.c index 7cce1c294f..77dfe41ffe 100644 --- a/imap/jmap_contact.c +++ b/imap/jmap_contact.c @@ -11873,7 +11873,7 @@ static int jmap_contact_getblob(jmap_req_t *req, jmap_getblob_context_t *ctx) } /* Open mailbox, we need it now */ - r = mailbox_open_iwl(mbentry->name, &mailbox); + r = mailbox_open_irl(mbentry->name, &mailbox); if (r) { ctx->errstr = error_message(r); res = HTTP_SERVER_ERROR; From 0dbc622d403858a20d1deea241fd5513c03c5922 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 21 Nov 2023 10:00:25 +1100 Subject: [PATCH 25/38] create_notification_collection: fix logic to not leak a mailbox --- imap/http_dav_sharing.c | 41 ++++++++++++++++------------------------- 1 file changed, 16 insertions(+), 25 deletions(-) diff --git a/imap/http_dav_sharing.c b/imap/http_dav_sharing.c index d5f4c6b413..68d6bf1474 100644 --- a/imap/http_dav_sharing.c +++ b/imap/http_dav_sharing.c @@ -365,15 +365,15 @@ int dav_lookup_notify_collection(const char *userid, mbentry_t **mbentry) return r; } -static int _create_notify_collection(const char *userid, struct mailbox **mailbox) +static int _create_notify_collection(const char *userid, mbentry_t **mbentryp) { /* lock the namespace lock and try again */ struct mboxlock *namespacelock = user_namespacelock(userid); - mbentry_t *mbentry = NULL; - int r = dav_lookup_notify_collection(userid, &mbentry); + int r = dav_lookup_notify_collection(userid, mbentryp); if (r == IMAP_MAILBOX_NONEXISTENT) { + mbentry_t *mbentry = *mbentryp; if (!mbentry) goto done; else if (mbentry->server) { proxy_findserver(mbentry->server, &http_protocol, httpd_userid, @@ -383,44 +383,35 @@ static int _create_notify_collection(const char *userid, struct mailbox **mailbo r = mboxlist_createmailbox(mbentry, 0/*options*/, 0/*highestmodseq*/, 1/*isadmin*/, userid, NULL/*authstate*/, - 0/*flags*/, mailbox); - /* we lost the race, that's OK */ - if (r == IMAP_MAILBOX_LOCKED) r = 0; - if (r) syslog(LOG_ERR, "IOERROR: failed to create %s (%s)", + 0/*flags*/, NULL); + + if (r) xsyslog(LOG_ERR, "IOERROR: failed to create notify collection" + "mailbox=<%s> error=<%s>", mbentry->name, error_message(r)); } - else if (!r && mailbox) { - /* Open mailbox for writing */ - r = mailbox_open_iwl(mbentry->name, mailbox); - if (r) { - syslog(LOG_ERR, "mailbox_open_iwl(%s) failed: %s", - mbentry->name, error_message(r)); - } - } done: mboxname_release(&namespacelock); - mboxlist_entry_free(&mbentry); return r; } -static int create_notify_collection(const char *userid, struct mailbox **mailbox) +static int create_notify_collection(const char *userid, struct mailbox **mailboxp) { /* notifications collection */ mbentry_t *mbentry = NULL; int r = dav_lookup_notify_collection(userid, &mbentry); - if (r) { + + if (r == IMAP_MAILBOX_NONEXISTENT) { mboxlist_entry_free(&mbentry); - return _create_notify_collection(userid, mailbox); + r = _create_notify_collection(userid, &mbentry); } - if (mailbox) { + if (!r && mailboxp) { /* Open mailbox for writing */ - r = mailbox_open_iwl(mbentry->name, mailbox); - if (r) { - syslog(LOG_ERR, "mailbox_open_iwl(%s) failed: %s", - mbentry->name, error_message(r)); - } + r = mailbox_open_iwl(mbentry->name, mailboxp); + if (r) xsyslog(LOG_ERR, "IOERROR: failed to open notify collection", + "mailbox=<%s> error=<%s>", + mbentry->name, error_message(r)); } mboxlist_entry_free(&mbentry); From 7e57e00f22355b1eb643c74070c4c6fb50a13f13 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Mon, 27 Nov 2023 10:20:17 +1100 Subject: [PATCH 26/38] mailbox: avoid infinite loop on bad locks --- imap/mailbox.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 61b0291514..719c98966d 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -2467,22 +2467,6 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int index_lockty assert(mailbox->index_fd != -1); - // if we're already locked, don't need to lock again! - if (mailbox->index_locktype) { - if ((mailbox->index_locktype & LOCK_SHARED) && (index_locktype & LOCK_EXCLUSIVE)) - return IMAP_MAILBOX_LOCKED; - return 0; - } - - char *userid = mboxname_to_userid(mailbox_name(mailbox)); - if (userid) { - if (!user_isnamespacelocked(userid)) { - free(userid); - return mailbox_relock(mailbox, LOCK_SHARED, index_locktype); - } - free(userid); - } - if (index_locktype == LOCK_EXCLUSIVE) { r = lock_blocking(mailbox->index_fd, index_fname); } @@ -2569,6 +2553,26 @@ static int mailbox_lock_index_internal(struct mailbox *mailbox, int index_lockty EXPORTED int mailbox_lock_index(struct mailbox *mailbox, int index_locktype) { int r = 0; + int need_relock = 0; + + if (mailbox->index_locktype) { + if (mailbox->index_locktype & LOCK_EXCLUSIVE) + return 0; // exclusive lock is good for anything + if (index_locktype & LOCK_SHARED) + return 0; // shared lock is OK if that's all we need + // we're going to need to re-lock + need_relock = 1; + } + else { + // if the user isn't locked, we always need to relock + char *userid = mboxname_to_userid(mailbox_name(mailbox)); + if (!user_isnamespacelocked(userid)) + need_relock = 1; + free(userid); + } + + if (need_relock) + return mailbox_relock(mailbox, LOCK_SHARED, index_locktype); r = mailbox_lock_index_internal(mailbox, index_locktype); if (r) return r; From af93f2aa9163477d0981cbd48644b9a984954c68 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Mon, 27 Nov 2023 10:25:26 +1100 Subject: [PATCH 27/38] mailbox: lock global as well --- imap/mailbox.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index 719c98966d..d125477e60 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -951,17 +951,15 @@ static int mailbox_relock(struct mailbox *mailbox, int locktype, int index_lockt r = mailbox_open_index(mailbox, index_locktype); if (r) return r; char *userid = mboxname_to_userid(mailbox_name(mailbox)); - if (userid) { - int haslock = user_isnamespacelocked(userid); - if (haslock) { - if ((haslock & LOCK_SHARED) && (index_locktype & LOCK_EXCLUSIVE)) - r = IMAP_MAILBOX_LOCKED; - } - else { - mailbox->local_namespacelock = user_namespacelock_full(userid, index_locktype); - } - free(userid); + int haslock = user_isnamespacelocked(userid); + if (haslock) { + if ((haslock & LOCK_SHARED) && (index_locktype & LOCK_EXCLUSIVE)) + r = IMAP_MAILBOX_LOCKED; + } + else { + mailbox->local_namespacelock = user_namespacelock_full(userid, index_locktype); } + free(userid); if (r) return r; r = mailbox_lock_index_internal(mailbox, index_locktype); return r; @@ -6980,11 +6978,10 @@ static int mailbox_reconstruct_create(const char *name, struct mailbox **mbptr) // lock the user namespace FIRST before the mailbox namespace char *userid = mboxname_to_userid(name); - if (userid) { - int haslock = user_isnamespacelocked(userid); - if (haslock) { - if (!(haslock & LOCK_EXCLUSIVE)) - r = IMAP_MAILBOX_LOCKED; + int haslock = user_isnamespacelocked(userid); + if (haslock) { + if (!(haslock & LOCK_EXCLUSIVE)) { + r = IMAP_MAILBOX_LOCKED; } else { local_namespacelock = user_namespacelock_full(userid, LOCK_EXCLUSIVE); From fc63163e929d328b88b0d3c2925d6c1674fa644d Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Mon, 27 Nov 2023 21:59:25 +1100 Subject: [PATCH 28/38] dav_lookup_notify_collection: rename mbentryp --- imap/http_dav_sharing.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/imap/http_dav_sharing.c b/imap/http_dav_sharing.c index 68d6bf1474..f48dad3de6 100644 --- a/imap/http_dav_sharing.c +++ b/imap/http_dav_sharing.c @@ -318,7 +318,7 @@ static void my_dav_init(struct buf *serverinfo __attribute__((unused))) } -int dav_lookup_notify_collection(const char *userid, mbentry_t **mbentry) +int dav_lookup_notify_collection(const char *userid, mbentry_t **mbentryp) { mbname_t *mbname; const char *notifyname; @@ -341,22 +341,22 @@ int dav_lookup_notify_collection(const char *userid, mbentry_t **mbentry) /* Locate the mailbox */ notifyname = mbname_intname(mbname); - r = proxy_mlookup(notifyname, mbentry, NULL, NULL); + r = proxy_mlookup(notifyname, mbentryp, NULL, NULL); if (r == IMAP_MAILBOX_NONEXISTENT) { /* Find location of INBOX */ char *inboxname = mboxname_user_mbox(userid, NULL); - int r1 = proxy_mlookup(inboxname, mbentry, NULL, NULL); + int r1 = proxy_mlookup(inboxname, mbentryp, NULL, NULL); free(inboxname); if (r1 == IMAP_MAILBOX_NONEXISTENT) { r = IMAP_INVALID_USER; goto done; } - mboxlist_entry_free(mbentry); - *mbentry = mboxlist_entry_create(); - (*mbentry)->name = xstrdup(notifyname); - (*mbentry)->mbtype = MBTYPE_COLLECTION; + mboxlist_entry_free(mbentryp); + *mbentryp = mboxlist_entry_create(); + (*mbentryp)->name = xstrdup(notifyname); + (*mbentryp)->mbtype = MBTYPE_COLLECTION; } done: From 94af25ba2b65484611ce8cf14407a94649f61439 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Mon, 27 Nov 2023 22:18:44 +1100 Subject: [PATCH 29/38] JMAPCalendars sharewith-acl: test alphabetically BEFORE and AFTER --- .../JMAPCalendars/calendar-set-sharewith-acl | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/cassandane/tiny-tests/JMAPCalendars/calendar-set-sharewith-acl b/cassandane/tiny-tests/JMAPCalendars/calendar-set-sharewith-acl index 860fad8c33..d54b5a1b97 100644 --- a/cassandane/tiny-tests/JMAPCalendars/calendar-set-sharewith-acl +++ b/cassandane/tiny-tests/JMAPCalendars/calendar-set-sharewith-acl @@ -8,7 +8,8 @@ sub test_calendar_set_sharewith_acl my $jmap = $self->{jmap}; my $admin = $self->{adminstore}->get_client(); - $admin->create("user.test1"); + $admin->create("user.aatester"); + $admin->create("user.zztester"); my $res = $jmap->CallMethods([ ['Calendar/set', { @@ -79,7 +80,8 @@ sub test_calendar_set_sharewith_acl update => { $calendarId => { shareWith => { - test1 => $_->{rights}, + aatester => $_->{rights}, + zztester => $_->{rights}, }, }, }, @@ -104,8 +106,11 @@ sub test_calendar_set_sharewith_acl ), %{$_->{wantRights}}); $self->assert_deep_equals(\%mergedrights, - $res->[1][1]{list}[0]{shareWith}{test1}); + $res->[1][1]{list}[0]{shareWith}{aatester}); + $self->assert_deep_equals(\%mergedrights, + $res->[1][1]{list}[0]{shareWith}{zztester}); my %acl = @{$admin->getacl("user.cassandane.#calendars.$calendarId")}; - $self->assert_str_equals($_->{acl}, $acl{test1}); + $self->assert_str_equals($_->{acl}, $acl{aatester}); + $self->assert_str_equals($_->{acl}, $acl{zztester}); } } From 7f640cf47fe9164dc01531311e0f92cafc76e9ec Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Mon, 27 Nov 2023 22:45:13 +1100 Subject: [PATCH 30/38] mboxname: disabled code for checking *U* key ordering --- imap/imapd.c | 4 +++- imap/mboxname.c | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/imap/imapd.c b/imap/imapd.c index 28f570080c..677f1bc2ac 100644 --- a/imap/imapd.c +++ b/imap/imapd.c @@ -8781,7 +8781,6 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, /* take care of deleting old quotas */ if (!r && rename_user) { - user_sharee_renameacls(&imapd_namespace, olduser, newuser); user_deletequotaroots(olduser); sync_log_unuser(olduser); } @@ -8826,6 +8825,9 @@ static void cmd_rename(char *tag, char *oldname, char *newname, char *location, done: mboxname_release(&oldnamespacelock); mboxname_release(&newnamespacelock); + // rename acls after the lock is dropped + if (!r && rename_user) + user_sharee_renameacls(&imapd_namespace, olduser, newuser); mboxlist_entry_free(&mbentry); if (oldname != orig_oldname) free(oldname); if (newname != orig_newname) free(newname); diff --git a/imap/mboxname.c b/imap/mboxname.c index 3a66fbfa8b..703d26f026 100644 --- a/imap/mboxname.c +++ b/imap/mboxname.c @@ -131,6 +131,19 @@ EXPORTED int open_mboxlocks_exist(void) static struct mboxlocklist *create_lockitem(const char *name) { +#if 0 + // disable this log tracking! + if (!strncmp(name, "*U*", 3)) { + // LOCK ORDERING! + struct mboxlocklist *item; + for (item = open_mboxlocks; item; item = item->next) { + if (strncmp(item->l.name, "*U*", 3)) continue; + if (strcmp(name, item->l.name) < 0) { + syslog(LOG_ERR, "IOERROR: namelock ordering wrong %s < %s", name, item->l.name); + } + } + } +#endif struct mboxlocklist *item = xmalloc(sizeof(struct mboxlocklist)); item->next = open_mboxlocks; open_mboxlocks = item; From dab1fbbf33c1792afcc2cf5d46a9ffe6cec98a85 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 28 Nov 2023 00:32:09 +1100 Subject: [PATCH 31/38] dlist: add dlist_copy API This just serialises and unserialises, which is pretty inefficient but easy to write --- imap/dlist.c | 10 ++++++++++ imap/dlist.h | 2 ++ 2 files changed, 12 insertions(+) diff --git a/imap/dlist.c b/imap/dlist.c index 6e9f2f6524..4f28ddf0fe 100644 --- a/imap/dlist.c +++ b/imap/dlist.c @@ -1722,3 +1722,13 @@ EXPORTED void dlist_rename(struct dlist *dl, const char *name) free(dl->name); dl->name = xstrdup(name); } + +EXPORTED struct dlist *dlist_copy(const struct dlist *dl) +{ + struct buf buf = BUF_INITIALIZER; + struct dlist *new = NULL; + dlist_printbuf(dl, 1, &buf); + dlist_parsemap(&new, 1, 0, buf_base(&buf), buf_len(&buf)); + buf_free(&buf); + return new; +} diff --git a/imap/dlist.h b/imap/dlist.h index 4c8fc6cd7c..80437a4503 100644 --- a/imap/dlist.h +++ b/imap/dlist.h @@ -247,6 +247,8 @@ struct dlist *dlist_getkvchild_bykey(struct dlist *dl, void dlist_rename(struct dlist *dl, const char *name); +struct dlist *dlist_copy(const struct dlist *dl); + const char *dlist_lastkey(void); /* print a dlist iteratively rather than recursively */ From 7a528153566e0dbf08131494fb05aa9e5efeac29 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 28 Nov 2023 00:53:25 +1100 Subject: [PATCH 32/38] dav_send_notification - delay until after locks are released! --- imap/dlist.c | 1 + imap/http_dav_sharing.c | 45 ++++++++++++++++++++++++++++++++++++++--- imap/http_dav_sharing.h | 3 ++- imap/jmap_api.c | 4 ++-- imap/jmap_calendar.c | 1 + imap/jmap_contact.c | 2 ++ imap/jmap_mailbox.c | 3 +++ 7 files changed, 53 insertions(+), 6 deletions(-) diff --git a/imap/dlist.c b/imap/dlist.c index 4f28ddf0fe..ce5ae9d59f 100644 --- a/imap/dlist.c +++ b/imap/dlist.c @@ -1725,6 +1725,7 @@ EXPORTED void dlist_rename(struct dlist *dl, const char *name) EXPORTED struct dlist *dlist_copy(const struct dlist *dl) { + if (!dl) return NULL; struct buf buf = BUF_INITIALIZER; struct dlist *new = NULL; dlist_printbuf(dl, 1, &buf); diff --git a/imap/http_dav_sharing.c b/imap/http_dav_sharing.c index f48dad3de6..f773d45779 100644 --- a/imap/http_dav_sharing.c +++ b/imap/http_dav_sharing.c @@ -879,7 +879,7 @@ static int dav_store_notification(struct transaction_t *txn, return r; } -HIDDEN int dav_send_notification(xmlDocPtr doc, struct dlist *extradata, +static int dav_send_notification(xmlDocPtr doc, struct dlist *extradata, const char *userid, const char *resource) { struct mailbox *mailbox = NULL; @@ -951,6 +951,44 @@ HIDDEN int dav_send_notification(xmlDocPtr doc, struct dlist *extradata, return r; } +struct scheduled_notification { + xmlDocPtr doc; + struct dlist *extradata; + char *userid; + char *resource; + struct scheduled_notification *next; +}; + +static struct scheduled_notification *scheduled_notifications = NULL; + +HIDDEN int dav_schedule_notification(xmlDocPtr doc, struct dlist *extradata, + const char *userid, const char *resource) +{ + struct scheduled_notification *new = xzmalloc(sizeof(struct scheduled_notification)); + new->doc = xmlCopyDoc(doc, 1); + new->extradata = extradata; // not a copy, eaten by dav_send_notification + new->userid = xstrdupnull(userid); + new->resource = xstrdupnull(resource); + new->next = scheduled_notifications; + scheduled_notifications = new; + return 0; +} + +HIDDEN void dav_run_notifications() +{ + struct scheduled_notification *next = NULL; + struct scheduled_notification *item; + for (item = scheduled_notifications; item; item = next) { + next = item->next; + dav_send_notification(item->doc, item->extradata, item->userid, item->resource); + xmlFreeDoc(item->doc); + free(item->userid); + free(item->resource); + free(item); + } + scheduled_notifications = NULL; +} + /* Perform a POST request on a WebDAV notification resource */ HIDDEN int notify_post(struct transaction_t *txn) @@ -2105,8 +2143,8 @@ HIDDEN int dav_post_share(struct transaction_t *txn, struct meth_params *pparams strhash(txn->req_tgt.mbentry->name), strhash(userid)); - r = dav_send_notification(notify->doc, extradata, - userid, buf_cstring(&resource)); + r = dav_schedule_notification(notify->doc, extradata, + userid, buf_cstring(&resource)); } free(userid); @@ -2124,6 +2162,7 @@ HIDDEN int dav_post_share(struct transaction_t *txn, struct meth_params *pparams if (notify) xmlFreeDoc(notify->doc); buf_free(&resource); mboxname_release(&namespacelock); + dav_run_notifications(); return ret; } diff --git a/imap/http_dav_sharing.h b/imap/http_dav_sharing.h index 58c8d0f486..3c9776bbe9 100644 --- a/imap/http_dav_sharing.h +++ b/imap/http_dav_sharing.h @@ -93,8 +93,9 @@ int dav_create_invite(xmlNodePtr *notify, xmlNsPtr *ns, struct request_target_t *tgt, const struct prop_entry *live_props, const char *sharee, int access, xmlChar *content); -int dav_send_notification(xmlDocPtr doc, struct dlist *extradata, +int dav_schedule_notification(xmlDocPtr doc, struct dlist *extradata, const char *userid, const char *resource); +void dav_run_notifications(); int dav_lookup_notify_collection(const char *userid, mbentry_t **mbentry); diff --git a/imap/jmap_api.c b/imap/jmap_api.c index d715adf709..a0b2ba58ce 100644 --- a/imap/jmap_api.c +++ b/imap/jmap_api.c @@ -2740,8 +2740,8 @@ static void send_dav_invite(const char *userid, void *val, void *rock) dlist_setatom(extradata, "OLD", rights); cyrus_acl_masktostr(change->newrights, rights); dlist_setatom(extradata, "NEW", rights); - r = dav_send_notification(irock->notify->doc, extradata, - userid, buf_cstring(&irock->resource)); + r = dav_schedule_notification(irock->notify->doc, extradata, + userid, buf_cstring(&irock->resource)); } } } diff --git a/imap/jmap_calendar.c b/imap/jmap_calendar.c index 6f2d26e1b7..6c15e9de79 100644 --- a/imap/jmap_calendar.c +++ b/imap/jmap_calendar.c @@ -2308,6 +2308,7 @@ static int jmap_calendar_set(struct jmap_req *req) done: mboxname_release(&namespacelock); + dav_run_notifications(); // after releasing lock jmap_parser_fini(&argparser); jmap_set_fini(&set); return r; diff --git a/imap/jmap_contact.c b/imap/jmap_contact.c index 77dfe41ffe..449a6aee59 100644 --- a/imap/jmap_contact.c +++ b/imap/jmap_contact.c @@ -59,6 +59,7 @@ #include "hash.h" #include "http_carddav.h" #include "http_dav.h" +#include "http_dav_sharing.h" #include "http_jmap.h" #include "json_support.h" #include "mailbox.h" @@ -5546,6 +5547,7 @@ static int jmap_addressbook_set(struct jmap_req *req) done: mboxname_release(&namespacelock); + dav_run_notifications(); jmap_parser_fini(&argparser); jmap_set_fini(&set); return r; diff --git a/imap/jmap_mailbox.c b/imap/jmap_mailbox.c index b6bb5603a6..cf6b82bd92 100644 --- a/imap/jmap_mailbox.c +++ b/imap/jmap_mailbox.c @@ -57,6 +57,8 @@ #include "bsearch.h" #include "cyr_qsort_r.h" #include "http_jmap.h" +#include "http_dav.h" +#include "http_dav_sharing.h" #include "jmap_mail.h" #include "json_support.h" #include "mailbox.h" @@ -4122,6 +4124,7 @@ static int jmap_mailbox_set(jmap_req_t *req) struct mboxlock *namespacelock = user_namespacelock(req->accountid); _mboxset(req, &set); mboxname_release(&namespacelock); + dav_run_notifications(); jmap_ok(req, jmap_set_reply(&set.super)); done: From 48c803e9c750a4bd7ba435b752e84bd7f551ca13 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 28 Nov 2023 06:09:29 +1100 Subject: [PATCH 33/38] user: make lock name the inbox folder name This means they sort alphabetically the same as elsewhere --- imap/user.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/imap/user.c b/imap/user.c index ae4c9d8d41..496fb505d5 100644 --- a/imap/user.c +++ b/imap/user.c @@ -693,21 +693,13 @@ EXPORTED char *user_hash_xapian_byid(const char *mboxid, const char *root) static const char *_namelock_name_from_userid(const char *userid) { - const char *p; static struct buf buf = BUF_INITIALIZER; - if (!userid) userid = ""; // no userid == global lock buf_setcstr(&buf, "*U*"); - - for (p = userid; *p; p++) { - switch(*p) { - case '.': - buf_putc(&buf, '^'); - break; - default: - buf_putc(&buf, *p); - break; - } + if (userid) { + char *inbox = mboxname_user_mbox(userid, NULL); + buf_appendcstr(&buf, inbox); + free(inbox); } return buf_cstring(&buf); From 49c0b747e7bfd089118264b77ece5cbdf50c71b9 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 28 Nov 2023 09:31:26 +1100 Subject: [PATCH 34/38] jmap: take namespacelocks in order for /copy commands This requires opening conversations late as well, but means our lock ordering is always valid --- imap/jmap_calendar.c | 30 ++++++++++++++++++++++++++++-- imap/jmap_contact.c | 30 ++++++++++++++++++++++++++++-- imap/jmap_core.c | 30 ++++++++++++++++++++++++++++-- imap/jmap_mail.c | 36 +++++++++++++++++++++++++++++++++--- 4 files changed, 117 insertions(+), 9 deletions(-) diff --git a/imap/jmap_calendar.c b/imap/jmap_calendar.c index 6c15e9de79..882c943d82 100644 --- a/imap/jmap_calendar.c +++ b/imap/jmap_calendar.c @@ -176,7 +176,7 @@ static jmap_method_t jmap_calendar_methods_standard[] = { "CalendarEvent/copy", JMAP_URN_CALENDARS, &jmap_calendarevent_copy, - JMAP_NEED_CSTATE | JMAP_READ_WRITE + JMAP_READ_WRITE // can't open conversations until we have locks ordered }, { "CalendarEvent/parse", @@ -7686,6 +7686,8 @@ static int jmap_calendarevent_copy(struct jmap_req *req) json_t *destroy_events = json_array(); struct mailbox *notifmbox = NULL; mbentry_t *notifmb = NULL; + struct mboxlock *srcnamespacelock = NULL; + struct mboxlock *dstnamespacelock = NULL; /* Parse request */ jmap_copy_parse(req, &parser, NULL, NULL, ©, &err); @@ -7694,6 +7696,28 @@ static int jmap_calendarevent_copy(struct jmap_req *req) goto done; } + char *srcinbox = mboxname_user_mbox(copy.from_account_id, NULL); + char *dstinbox = mboxname_user_mbox(req->accountid, NULL); + if (strcmp(srcinbox, dstinbox) < 0) { + srcnamespacelock = mboxname_usernamespacelock(srcinbox); + dstnamespacelock = mboxname_usernamespacelock(dstinbox); + } + else { + dstnamespacelock = mboxname_usernamespacelock(dstinbox); + srcnamespacelock = mboxname_usernamespacelock(srcinbox); + } + free(srcinbox); + free(dstinbox); + + // now we can open the cstate + int r = conversations_open_user(req->accountid, 0, &req->cstate); + if (r) { + syslog(LOG_ERR, "jmap_email_copy: can't open converstaions: %s", + error_message(r)); + jmap_error(req, jmap_server_error(r)); + goto done; + } + src_db = caldav_open_userid(copy.from_account_id); if (!src_db) { jmap_error(req, json_pack("{s:s}", "type", "fromAccountNotFound")); @@ -7706,7 +7730,7 @@ static int jmap_calendarevent_copy(struct jmap_req *req) } /* Open notifications mailbox, but continue even on error. */ - int r = jmap_create_notify_collection(req->accountid, ¬ifmb); + r = jmap_create_notify_collection(req->accountid, ¬ifmb); if (!r) r = mailbox_open_iwl(notifmb->name, ¬ifmbox); if (r) { xsyslog(LOG_WARNING, "can not open jmapnotify collection", @@ -7754,6 +7778,8 @@ static int jmap_calendarevent_copy(struct jmap_req *req) json_decref(destroy_events); if (src_db) caldav_close(src_db); if (dst_db) caldav_close(dst_db); + mboxname_release(&srcnamespacelock); + mboxname_release(&dstnamespacelock); jmap_parser_fini(&parser); jmap_copy_fini(©); return 0; diff --git a/imap/jmap_contact.c b/imap/jmap_contact.c index 449a6aee59..349b73227b 100644 --- a/imap/jmap_contact.c +++ b/imap/jmap_contact.c @@ -180,7 +180,7 @@ static jmap_method_t jmap_contact_methods_standard[] = { "ContactCard/copy", JMAP_URN_CONTACTS, &jmap_card_copy, - JMAP_NEED_CSTATE | JMAP_READ_WRITE + JMAP_READ_WRITE // need to do lock ordering before opening conversations }, { "ContactCard/parse", @@ -245,7 +245,7 @@ static jmap_method_t jmap_contact_methods_nonstandard[] = { "Contact/copy", JMAP_CONTACTS_EXTENSION, &jmap_contact_copy, - JMAP_NEED_CSTATE | JMAP_READ_WRITE + JMAP_READ_WRITE // can't open conversations until we get locks ordered }, { NULL, NULL, NULL, 0} }; @@ -4523,6 +4523,8 @@ static int _contacts_copy(struct jmap_req *req, json_t *err = NULL; struct carddav_db *src_db = NULL; json_t *destroy_cards = json_array(); + struct mboxlock *srcnamespacelock = NULL; + struct mboxlock *dstnamespacelock = NULL; /* Parse request */ jmap_copy_parse(req, &parser, NULL, NULL, ©, &err); @@ -4531,6 +4533,28 @@ static int _contacts_copy(struct jmap_req *req, goto done; } + char *srcinbox = mboxname_user_mbox(copy.from_account_id, NULL); + char *dstinbox = mboxname_user_mbox(req->accountid, NULL); + if (strcmp(srcinbox, dstinbox) < 0) { + srcnamespacelock = mboxname_usernamespacelock(srcinbox); + dstnamespacelock = mboxname_usernamespacelock(dstinbox); + } + else { + dstnamespacelock = mboxname_usernamespacelock(dstinbox); + srcnamespacelock = mboxname_usernamespacelock(srcinbox); + } + free(srcinbox); + free(dstinbox); + + // now we can open the cstate + int r = conversations_open_user(req->accountid, 0, &req->cstate); + if (r) { + syslog(LOG_ERR, "jmap_email_copy: can't open converstaions: %s", + error_message(r)); + jmap_error(req, jmap_server_error(r)); + goto done; + } + src_db = carddav_open_userid(copy.from_account_id); if (!src_db) { jmap_error(req, json_pack("{s:s}", "type", "fromAccountNotFound")); @@ -4577,6 +4601,8 @@ static int _contacts_copy(struct jmap_req *req, done: json_decref(destroy_cards); if (src_db) carddav_close(src_db); + mboxname_release(&srcnamespacelock); + mboxname_release(&dstnamespacelock); jmap_parser_fini(&parser); jmap_copy_fini(©); return 0; diff --git a/imap/jmap_core.c b/imap/jmap_core.c index 060cef47c1..873e302fc9 100644 --- a/imap/jmap_core.c +++ b/imap/jmap_core.c @@ -77,7 +77,7 @@ static jmap_method_t jmap_core_methods_standard[] = { "Blob/copy", JMAP_URN_CORE, &jmap_blob_copy, - JMAP_NEED_CSTATE | JMAP_READ_WRITE + JMAP_READ_WRITE // no conversations, we need to get lock ordering first }, { "Core/echo", @@ -357,6 +357,8 @@ static int jmap_blob_copy(jmap_req_t *req) size_t i = 0; int r = 0; struct mailbox *to_mbox = NULL; + struct mboxlock *srcnamespacelock = NULL; + struct mboxlock *dstnamespacelock = NULL; /* Parse request */ jmap_copy_parse(req, &parser, NULL, NULL, ©, &err); @@ -365,6 +367,28 @@ static int jmap_blob_copy(jmap_req_t *req) goto cleanup; } + char *srcinbox = mboxname_user_mbox(copy.from_account_id, NULL); + char *dstinbox = mboxname_user_mbox(req->accountid, NULL); + if (strcmp(srcinbox, dstinbox) < 0) { + srcnamespacelock = mboxname_usernamespacelock(srcinbox); + dstnamespacelock = mboxname_usernamespacelock(dstinbox); + } + else { + dstnamespacelock = mboxname_usernamespacelock(dstinbox); + srcnamespacelock = mboxname_usernamespacelock(srcinbox); + } + free(srcinbox); + free(dstinbox); + + // now we can open the cstate + r = conversations_open_user(req->accountid, 0, &req->cstate); + if (r) { + syslog(LOG_ERR, "jmap_email_copy: can't open converstaions: %s", + error_message(r)); + jmap_error(req, jmap_server_error(r)); + goto cleanup; + } + /* Check if we can upload to toAccountId */ r = jmap_open_upload_collection(req->accountid, &to_mbox); if (r == IMAP_PERMISSION_DENIED) { @@ -397,9 +421,11 @@ static int jmap_blob_copy(jmap_req_t *req) r = 0; cleanup: + mailbox_close(&to_mbox); + mboxname_release(&srcnamespacelock); + mboxname_release(&dstnamespacelock); jmap_parser_fini(&parser); jmap_copy_fini(©); - mailbox_close(&to_mbox); return r; } diff --git a/imap/jmap_mail.c b/imap/jmap_mail.c index 7b37f9b1fe..889996500c 100644 --- a/imap/jmap_mail.c +++ b/imap/jmap_mail.c @@ -169,7 +169,7 @@ static jmap_method_t jmap_mail_methods_standard[] = { "Email/copy", JMAP_URN_MAIL, &jmap_email_copy, - JMAP_NEED_CSTATE | JMAP_READ_WRITE + JMAP_READ_WRITE // don't want cstate, we need to take locks in order }, { "SearchSnippet/get", @@ -9774,7 +9774,7 @@ static struct emailpart *_emailpart_parse(jmap_req_t *req, /* type */ json_t *jtype = json_object_get(jpart, "type"); if (JNOTNULL(jtype) && json_is_string(jtype) && !have_type_header) { - const char *type = json_string_value(jtype); + const char *type = json_string_value(jtype); struct param *type_params = NULL; /* Validate type value */ message_parse_type(type, &part->type, &part->subtype, &type_params); @@ -14093,6 +14093,8 @@ static int jmap_email_copy(jmap_req_t *req) json_t *destroy_emails = json_array(); const char *scheduled_uniqueid = NULL; const mbentry_t *scheduled_mbe = NULL; + struct mboxlock *srcnamespacelock = NULL; + struct mboxlock *dstnamespacelock = NULL; /* Parse request */ jmap_copy_parse(req, &parser, NULL, NULL, ©, &err); @@ -14101,7 +14103,33 @@ static int jmap_email_copy(jmap_req_t *req) goto done; } - int r = seen_open(req->userid, SEEN_CREATE, &seendb); + if (open_mboxlocks_exist()) { + abort(); + } + + char *srcinbox = mboxname_user_mbox(copy.from_account_id, NULL); + char *dstinbox = mboxname_user_mbox(req->accountid, NULL); + if (strcmp(srcinbox, dstinbox) < 0) { + srcnamespacelock = mboxname_usernamespacelock(srcinbox); + dstnamespacelock = mboxname_usernamespacelock(dstinbox); + } + else { + dstnamespacelock = mboxname_usernamespacelock(dstinbox); + srcnamespacelock = mboxname_usernamespacelock(srcinbox); + } + free(srcinbox); + free(dstinbox); + + // now we can open the cstate + int r = conversations_open_user(req->accountid, 0, &req->cstate); + if (r) { + syslog(LOG_ERR, "jmap_email_copy: can't open converstaions: %s", + error_message(r)); + jmap_error(req, jmap_server_error(r)); + goto done; + } + + r = seen_open(req->userid, SEEN_CREATE, &seendb); if (r) { syslog(LOG_ERR, "jmap_email_copy: can't open seen.db: %s", error_message(r)); @@ -14157,6 +14185,8 @@ static int jmap_email_copy(jmap_req_t *req) } done: + mboxname_release(&srcnamespacelock); + mboxname_release(&dstnamespacelock); json_decref(destroy_emails); jmap_parser_fini(&parser); jmap_copy_fini(©); From 83e722184f4e7685487756a4ab9d262b5038125a Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 28 Nov 2023 09:44:05 +1100 Subject: [PATCH 35/38] dav_run_notifications: move to after the conversations lock is over --- imap/jmap_api.c | 3 +++ imap/jmap_calendar.c | 1 - imap/jmap_contact.c | 1 - imap/jmap_mailbox.c | 1 - 4 files changed, 3 insertions(+), 3 deletions(-) diff --git a/imap/jmap_api.c b/imap/jmap_api.c index a0b2ba58ce..ee2469bcb5 100644 --- a/imap/jmap_api.c +++ b/imap/jmap_api.c @@ -839,6 +839,9 @@ HIDDEN int jmap_api(struct transaction_t *txn, } conversations_commit(&req.cstate); + // run any notification updates after conversations are released + dav_run_notifications(); + json_decref(args); } diff --git a/imap/jmap_calendar.c b/imap/jmap_calendar.c index 882c943d82..5d7773a1f8 100644 --- a/imap/jmap_calendar.c +++ b/imap/jmap_calendar.c @@ -2308,7 +2308,6 @@ static int jmap_calendar_set(struct jmap_req *req) done: mboxname_release(&namespacelock); - dav_run_notifications(); // after releasing lock jmap_parser_fini(&argparser); jmap_set_fini(&set); return r; diff --git a/imap/jmap_contact.c b/imap/jmap_contact.c index 349b73227b..2985e0ee22 100644 --- a/imap/jmap_contact.c +++ b/imap/jmap_contact.c @@ -5573,7 +5573,6 @@ static int jmap_addressbook_set(struct jmap_req *req) done: mboxname_release(&namespacelock); - dav_run_notifications(); jmap_parser_fini(&argparser); jmap_set_fini(&set); return r; diff --git a/imap/jmap_mailbox.c b/imap/jmap_mailbox.c index cf6b82bd92..4774a2f9d6 100644 --- a/imap/jmap_mailbox.c +++ b/imap/jmap_mailbox.c @@ -4124,7 +4124,6 @@ static int jmap_mailbox_set(jmap_req_t *req) struct mboxlock *namespacelock = user_namespacelock(req->accountid); _mboxset(req, &set); mboxname_release(&namespacelock); - dav_run_notifications(); jmap_ok(req, jmap_set_reply(&set.super)); done: From 211b0832e011adfc0c1190f65e86a39b9a666c26 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 12 Dec 2023 23:35:03 +1100 Subject: [PATCH 36/38] Update imap/mailbox.c Co-authored-by: elliefm --- imap/mailbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap/mailbox.c b/imap/mailbox.c index d125477e60..e820a5aa72 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -2648,7 +2648,7 @@ EXPORTED void mailbox_unlock_index(struct mailbox *mailbox, struct statusdata *s if (mailbox->local_cstate) { int r = conversations_commit(&mailbox->local_cstate); if (r) { - xsyslog(LOG_ERR, "IOERROR: Error committing to conversations database" + xsyslog(LOG_ERR, "IOERROR: Error committing to conversations database", "mailbox=<%s> error=<%s>", mailbox_name(mailbox), error_message(r)); } From e6eed671fb29007e5b8d086d1449c9db20fcc55c Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Tue, 12 Dec 2023 23:35:30 +1100 Subject: [PATCH 37/38] Update imap/http_dav_sharing.c Co-authored-by: elliefm --- imap/http_dav_sharing.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap/http_dav_sharing.c b/imap/http_dav_sharing.c index f773d45779..13b9a11f89 100644 --- a/imap/http_dav_sharing.c +++ b/imap/http_dav_sharing.c @@ -385,7 +385,7 @@ static int _create_notify_collection(const char *userid, mbentry_t **mbentryp) 1/*isadmin*/, userid, NULL/*authstate*/, 0/*flags*/, NULL); - if (r) xsyslog(LOG_ERR, "IOERROR: failed to create notify collection" + if (r) xsyslog(LOG_ERR, "IOERROR: failed to create notify collection", "mailbox=<%s> error=<%s>", mbentry->name, error_message(r)); } From 9255220cbc49cfe6225ddaf8803aef772551b3d2 Mon Sep 17 00:00:00 2001 From: Bron Gondwana Date: Wed, 13 Dec 2023 09:37:31 +1100 Subject: [PATCH 38/38] Reconstruct: remove eval (elliefm review) --- cassandane/Cassandane/Cyrus/Reconstruct.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cassandane/Cassandane/Cyrus/Reconstruct.pm b/cassandane/Cassandane/Cyrus/Reconstruct.pm index abc26bd30c..d4232f7e4b 100644 --- a/cassandane/Cassandane/Cyrus/Reconstruct.pm +++ b/cassandane/Cassandane/Cyrus/Reconstruct.pm @@ -613,7 +613,7 @@ sub test_downgrade_upgrade for my $version (12, 14, 16, 'max') { xlog $self, "Set to version $version"; - eval { $self->{instance}->run_command({ cyrus => 1 }, 'reconstruct', '-V', $version) }; + $self->{instance}->run_command({ cyrus => 1 }, 'reconstruct', '-V', $version); xlog $self, "Reconnect, \\Seen should still be on message A"; $self->{store}->disconnect();