go-imap
go-imap copied to clipboard
Race conditions when accessing mailbox properties
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
We should probably return a copy of the mailbox status.
@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
?
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.
This was fixed in v2.