Build failed in Jenkins: cyrus-imapd-master #674

[Bron Gondwana]

on phone

Not just your bad. Naming of dlist pointers is shockingly bad. We need a scheme which makes ownership clearer!

Bron.

On Thu, Jun 28, 2012, at 10:24 AM, Greg Banks wrote:
> 
> 
> On Wed, Jun 27, 2012, at 05:08 PM, Jenkins wrote:
> > See <http://ci.cyrusimap.org/job/cyrus-imapd-master/674/>
> > 
> > --build-url=http://ci.cyrusimap.org/job/cyrus-imapd-master/674/
> > Test failures and errors summary
> > ================================
> > 
> > Cassandane::Cyrus::Metadata.specialuse
> >     http://ci.cyrusimap.org/job/cyrus-imapd-master/674//testReport/%28root%29/Cassandane__Cyrus__Metadata/test_specialuse/
> 
> Lots of tests were failing with "Some process is already listening on
> 127.0.0.1:9100", the highly annoying Cassandane cascading failure mode.
> 
> > Cassandane::Cyrus::Metadata.msg_replication_mod_bot_msl
> >     http://ci.cyrusimap.org/job/cyrus-imapd-master/674//testReport/%28root%29/Cassandane__Cyrus__Metadata/test_msg_replication_mod_bot_msl/
> 
> This was an actual bug
> 
> Error Message
> Perl exception: Core files found in /var/tmp/cass/21032384/conf/cores    
> 
> Core was generated by
> `/var/lib/jenkins/jobs/cyrus-imapd-master/workspace/inst/usr/cyrus/bin/sync_clie'.
> Program terminated with signal 6, Aborted.
> #0  0x0000003665830265 in raise () from /lib64/libc.so.6
> (gdb) bt
> #0  0x0000003665830265 in raise () from /lib64/libc.so.6
> #1  0x0000003665831d10 in abort () from /lib64/libc.so.6
> #2  0x000000366586a84b in __libc_message () from /lib64/libc.so.6
> #3  0x000000366587230f in _int_free () from /lib64/libc.so.6
> #4  0x000000366587276b in free () from /lib64/libc.so.6
> #5  0x00002abf9a6df754 in dlist_free (dlp=0x7fff951d71d8) at
> imap/dlist.c:650
> #6  0x000000000040a20c in mailbox_full_update (mboxname=0x18d164c0
> "user.cassandane") at imap/sync_client.c:1366
> #7  0x000000000040ac77 in update_mailbox (local=0x18d15f10,
> remote=0x18d16b50, reserve_guids=0x18d16760)
>     at imap/sync_client.c:1502
> #8  0x000000000040c139 in do_folders (mboxname_list=0x18d14ab0,
> replica_folders=0x18d14a90, delete_remote=1)
>     at imap/sync_client.c:1825
> #9  0x000000000040cc70 in do_user_main (user=0x7fff951d9b0f
> "cassandane", replica_folders=0x18d14a90, 
>     replica_quota=0x18d15a70) at imap/sync_client.c:2000
> #10 0x000000000040d9bf in do_user (userid=0x7fff951d9b0f "cassandane")
> at imap/sync_client.c:2204
> #11 0x0000000000412588 in main (argc=9, argv=0x7fff951d8768) at
> imap/sync_client.c:3246
> 
> Hmm
> 
> 646     void dlist_free(struct dlist **dlp)
> 647     {
> 648         if (!*dlp) return;
> 649         _dlist_clean(*dlp);
> 650         free((*dlp)->name);   <----
> 651         free(*dlp);
> 652         *dlp = NULL;
> 653     }
> 
> 1361        mailbox_close(&mailbox);
> 1362    
> 1363        dlist_free(&kin);
> 1364        dlist_free(&kaction);
> 1365        dlist_free(&kexpunge);
> 1366        dlist_free(&kuids); <----
> 1367    
> 1368        return r;
> 1369    }
> 
> Ah, that line was added recently by Bron on the basis of an analysis I
> made on an internal Fastmail mailing list.  Let's look at the code a bit
> harder.
> 
> 1217 static int mailbox_full_update(const char *mboxname)
> 1218 {
> …
> 1225     struct dlist *kuids = NULL;
> …
> 1321     kexpunge = dlist_newkvlist(NULL, "EXPUNGE");
> 1322     dlist_setatom(kexpunge, "MBOXNAME", mailbox->name);
> 1323     dlist_setatom(kexpunge, "UNIQUEID", mailbox->uniqueid); /* just
> for safety */
> 1324     kuids = dlist_newlist(kexpunge, "UID");
> 1325     for (ka = kaction->head; ka; ka = ka->next) {
> 1326         if (!strcmp(ka->name, "EXPUNGE")) {
> 1327             dlist_setnum32(kuids, "UID", dlist_num(ka));
> 1328         }
> 1329         else if (!strcmp(ka->name, "COPYBACK")) {
> 1330             r = copy_remote(mailbox, dlist_num(ka), kr);
> 1331             if (r) goto cleanup;
> 1332             dlist_setnum32(kuids, "UID", dlist_num(ka));
> 1333         }
> 1334         else if (!strcmp(ka->name, "RENUMBER")) {
> 1335             r = copy_local(mailbox, dlist_num(ka));
> 1336             if (r) goto cleanup;
> 1337         }
> 1338     }
> …
> 1365     dlist_free(&kexpunge);
> 1366     dlist_free(&kuids);
> 
> Whoops, I was completely wrong about leaking kuids.  Now we are
> double-freeing it.  The 'kuids' pointer actually points into the dlist
> tree whose root is pointed to by 'kexpunge'.  So we're freeing the kuids
> dlists at line 1365, then once more at the new line 1366.  My bad :(
> 
> Fixed in commit
> http://git.cyrusimap.org/cyrus-imapd/commit/?id=6f10cc844e297d31d2c3ed8ec83a818533cdbf90
> 
> -- 
> Greg.
-- 
  Bron Gondwana
  brong at fastmail.fm