[RFC PATCH] Fix index lock breakage in mailbox_release_resources()

[Bron Gondwana]

On Sun, Jul 19, 2015, at 07:06 AM, Thomas Jarosch wrote:
> mailbox_release_resources() closes the file descriptor
> to the index. This implicitly releases any lock on the file.
> 
> Unfortunately the function does not reset the index_locktype state.
> Therefore we didn't notice that a mailbox rename
> operated on an unlocked index of the new mailbox. Yikes.

How did you discover this?  I'm surprised we haven't run into this already.

> Note: mailbox_index_lock() implies a mailbox_read_index_header().
> But it also does more work than just read the index header,
> so there might be side effects.

I changed to mailbox_index_lock_internal() since we don't want to abort
on a DELETED mailbox.

> @@ -4370,7 +4371,7 @@ static int mailbox_reconstruct_create(const char *name, struct mailbox **mbptr)
>   
>      /* Attempt to open index */
>      r = mailbox_open_index(mailbox);
> -    if (!r) r = mailbox_read_index_header(mailbox);
> +    if (!r) r = mailbox_lock_index(mailbox, LOCK_EXCLUSIVE);
>      if (r) {
>  	printf("%s: failed to read index header\n", mailbox->name);
>  	syslog(LOG_ERR, "failed to read index header for %s", mailbox->name);

I'm not convinced by this one - the whole point with reconstruct is that you're working with a potentially broken mailbox, and you want to check each step more simply to see if it works.  I've left this one in place.

(we do need some more tests of reconstruct on mailboxes with errors in them to make sure it corrects them - anyone feel like doing some Cassandane work? :) )

Bron.



-- 
  Bron Gondwana
  brong at fastmail.fm