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 [email protected]
