refactor: neomutt_mailboxlist_get_all()

This function used to return a STAILQ_HEAD, but that hid a subtle bug when the STAILQ was returned empty. When the STAILQ is initialised, `stqh_last` is set to the address of `stqh_first` which equates to the address of the STAILQ itself. The STAILQ is on the stack. If the function returned without adding anything to the STAILQ, the `stqh_last` pointer would now be invalid, pointing to the now-dead stack.
dariocorreal · Jun 3, 2020 · 6ab8294 · 6ab8294
1 parent ff987e9
commit 6ab8294
Show file tree

Hide file tree

Showing 11 changed files with 43 additions and 24 deletions.
diff --git a/browser.c b/browser.c
@@ -775,7 +775,8 @@ static int examine_directory(struct Menu *menu, struct BrowserState *state,
 
     init_state(state, menu);
 
-    struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+    struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+    neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
     while ((de = readdir(dp)))
     {
       if (mutt_str_strcmp(de->d_name, ".") == 0)
@@ -866,7 +867,8 @@ static int examine_mailboxes(struct Menu *menu, struct BrowserState *state)
     md = mutt_buffer_pool_get();
     mutt_mailbox_check(Context ? Context->mailbox : NULL, 0);
 
-    struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+    struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+    neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
     struct MailboxNode *np = NULL;
     STAILQ_FOREACH(np, &ml, entries)
     {

diff --git a/command_parse.c b/command_parse.c
@@ -1921,7 +1921,8 @@ enum CommandResult parse_unmailboxes(struct Buffer *buf, struct Buffer *s,
       tmp_valid = true;
     }
 
-    struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+    struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+    neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
     struct MailboxNode *np = NULL;
     struct MailboxNode *nptmp = NULL;
     STAILQ_FOREACH_SAFE(np, &ml, entries, nptmp)

diff --git a/core/mailbox.c b/core/mailbox.c
@@ -96,7 +96,8 @@ struct Mailbox *mailbox_find(const char *path)
   if (stat(path, &sb) != 0)
     return NULL;
 
-  struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+  neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
   struct MailboxNode *np = NULL;
   struct Mailbox *m = NULL;
   STAILQ_FOREACH(np, &ml, entries)
@@ -126,7 +127,8 @@ struct Mailbox *mailbox_find_name(const char *name)
   if (!name)
     return NULL;
 
-  struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+  neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
   struct MailboxNode *np = NULL;
   struct Mailbox *m = NULL;
   STAILQ_FOREACH(np, &ml, entries)

diff --git a/core/neomutt.c b/core/neomutt.c
@@ -148,18 +148,20 @@ void neomutt_mailboxlist_clear(struct MailboxList *ml)
 
 /**
  * neomutt_mailboxlist_get_all - Get a List of all Mailboxes
+ * @param head List to store the Mailboxes
  * @param n    NeoMutt
  * @param type Type of Account to match, see #MailboxType
- * @retval obj List of Mailboxes
+ * @retval num Number of Mailboxes in the List
  *
  * @note If type is #MUTT_MAILBOX_ANY then all Mailbox types will be matched
  */
-struct MailboxList neomutt_mailboxlist_get_all(struct NeoMutt *n, enum MailboxType type)
+size_t neomutt_mailboxlist_get_all(struct MailboxList *head, struct NeoMutt *n,
+                                   enum MailboxType type)
 {
-  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
   if (!n)
-    return ml;
+    return 0;
 
+  size_t count = 0;
   struct Account *a = NULL;
   struct MailboxNode *mn = NULL;
 
@@ -172,9 +174,10 @@ struct MailboxList neomutt_mailboxlist_get_all(struct NeoMutt *n, enum MailboxTy
     {
       struct MailboxNode *mn2 = mutt_mem_calloc(1, sizeof(*mn2));
       mn2->mailbox = mn->mailbox;
-      STAILQ_INSERT_TAIL(&ml, mn2, entries);
+      STAILQ_INSERT_TAIL(head, mn2, entries);
+      count++;
     }
   }
 
-  return ml;
+  return count;
 }
diff --git a/core/neomutt.h b/core/neomutt.h
@@ -56,7 +56,7 @@ bool            neomutt_account_remove(struct NeoMutt *n, struct Account *a);
 void            neomutt_free          (struct NeoMutt **ptr);
 struct NeoMutt *neomutt_new           (struct ConfigSet *cs);
 
-void               neomutt_mailboxlist_clear  (struct MailboxList *ml);
-struct MailboxList neomutt_mailboxlist_get_all(struct NeoMutt *n, enum MailboxType type);
+void   neomutt_mailboxlist_clear  (struct MailboxList *ml);
+size_t neomutt_mailboxlist_get_all(struct MailboxList *head, struct NeoMutt *n, enum MailboxType type);
 
 #endif /* MUTT_CORE_NEOMUTT_H */
diff --git a/imap/browse.c b/imap/browse.c
@@ -112,7 +112,8 @@ static void add_folder(char delim, char *folder, bool noselect, bool noinferiors
   (state->entry)[state->entrylen].selectable = !noselect;
   (state->entry)[state->entrylen].inferiors = !noinferiors;
 
-  struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+  neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
   struct MailboxNode *np = NULL;
   STAILQ_FOREACH(np, &ml, entries)
   {
@@ -206,7 +207,8 @@ int imap_browse(const char *path, struct BrowserState *state)
   C_ImapCheckSubscribed = false;
 
   // Pick first mailbox connected to the same server
-  struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_IMAP);
+  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+  neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_IMAP);
   struct MailboxNode *np = NULL;
   STAILQ_FOREACH(np, &ml, entries)
   {

diff --git a/imap/imap.c b/imap/imap.c
@@ -372,7 +372,8 @@ static int complete_hosts(char *buf, size_t buflen)
   size_t matchlen;
 
   matchlen = mutt_str_strlen(buf);
-  struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+  neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
   struct MailboxNode *np = NULL;
   STAILQ_FOREACH(np, &ml, entries)
   {

diff --git a/init.c b/init.c
@@ -952,7 +952,8 @@ int mutt_init(struct ConfigSet *cs, bool skip_sys_rc, struct ListHead *commands)
   if (C_VirtualSpoolfile)
   {
     /* Find the first virtual folder and open it */
-    struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_NOTMUCH);
+    struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+    neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_NOTMUCH);
     struct MailboxNode *mp = STAILQ_FIRST(&ml);
     if (mp)
       cs_str_string_set(cs, "spoolfile", mailbox_path(mp->mailbox), NULL);

diff --git a/mutt_mailbox.c b/mutt_mailbox.c
@@ -193,7 +193,8 @@ int mutt_mailbox_check(struct Mailbox *m_cur, int force)
     contex_sb.st_ino = 0;
   }
 
-  struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+  neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
   struct MailboxNode *np = NULL;
   STAILQ_FOREACH(np, &ml, entries)
   {
@@ -239,7 +240,8 @@ bool mutt_mailbox_list(void)
 
   mailboxlist[0] = '\0';
   pos += strlen(strncat(mailboxlist, _("New mail in "), sizeof(mailboxlist) - 1 - pos));
-  struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+  neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
   struct MailboxNode *np = NULL;
   STAILQ_FOREACH(np, &ml, entries)
   {
@@ -326,7 +328,8 @@ struct Mailbox *mutt_mailbox_next(struct Mailbox *m_cur, struct Buffer *s)
     bool found = false;
     for (int pass = 0; pass < 2; pass++)
     {
-      struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+      struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+      neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
       struct MailboxNode *np = NULL;
       STAILQ_FOREACH(np, &ml, entries)
       {

diff --git a/sidebar.c b/sidebar.c
@@ -445,7 +445,8 @@ static void unsort_entries(void)
 {
   int i = 0;
 
-  struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+  struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+  neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
   struct MailboxNode *np = NULL;
   STAILQ_FOREACH(np, &ml, entries)
   {
@@ -1170,7 +1171,8 @@ void sb_draw(struct MuttWindow *win)
 
   if (!Entries)
   {
-    struct MailboxList ml = neomutt_mailboxlist_get_all(NeoMutt, MUTT_MAILBOX_ANY);
+    struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+    neomutt_mailboxlist_get_all(&ml, NeoMutt, MUTT_MAILBOX_ANY);
     struct MailboxNode *np = NULL;
     STAILQ_FOREACH(np, &ml, entries)
     {

diff --git a/test/neo/neomutt_mailboxlist_get_all.c b/test/neo/neomutt_mailboxlist_get_all.c
@@ -28,10 +28,12 @@
 
 void test_neomutt_mailboxlist_get_all(void)
 {
-  // struct MailboxList neomutt_mailboxlist_get_all(struct NeoMutt *n, enum MailboxType magic);
+  // size_t neomutt_mailboxlist_get_all(struct MailboxList *head, struct NeoMutt *n, enum MailboxType magic);
 
   {
-    struct MailboxList ml = neomutt_mailboxlist_get_all(NULL, MUTT_MAILDIR);
+    struct MailboxList ml = STAILQ_HEAD_INITIALIZER(ml);
+    size_t count = neomutt_mailboxlist_get_all(&ml, NULL, MUTT_MAILDIR);
+    TEST_CHECK(count == 0);
     TEST_CHECK(STAILQ_EMPTY(&ml) == true);
   }
 }