Race conditions when accessing mailbox properties

[kristofferingemansson]

The current client.locker is not enough to protect from race conditions when accessing mailbox properties, either via client.Mailbox() or via <-updates.

The code below will only protect the client.mailbox pointer. After the function returns, reads on the returned object will "collide" with writes by the handleUnilateral-function. Since the client.locker mutex is unexported, it is also impossible for any caller of client.Mailbox() or consumer of <-updates to manually lock before accessing data. https://github.com/emersion/go-imap/blob/cfc6c581cf592705a35ccc6caf0c7bf73e4f5804/client/client.go#L300-L307

I suggest maybe renaming the mailbox.ItemsLocker to mailbox.Locker and using it to safeguard all access to any property of the mailbox instance. client.locker can continue to safeguard the pointer on the client object. https://github.com/emersion/go-imap/blob/cfc6c581cf592705a35ccc6caf0c7bf73e4f5804/client/client.go#L398-L406

[emersion]

We should probably return a copy of the mailbox status.

[fastred]

@emersion I'd like to tackle this issue. I'm wondering though if copying a mailbox status before returning it won't introduce any regressions.

Do you think it's possible that some user or extension code relied on being able to modify Client's internal mailbox status from the outside (given it's returned by reference) without using SetState?

[emersion]

Do you think it's possible that some user or extension code relied on being able to modify Client's internal mailbox status from the outside (given it's returned by reference) without using SetState?

Possible, but I wouldn't worry about breaking such broken users.

[emersion]

This was fixed in v2.