Community
Participate
Working Groups
Steps: 1. Trigger something that initializes secure store (e.g. CVS synchronization) 2. Trigger password prompt that writes to secure store (e.g. JIRA) "main" prio=10 tid=0x0000000041b37800 nid=0x1ebf in Object.wait() [0x00007fc92dd23000] java.lang.Thread.State: TIMED_WAITING (on object monitor) at java.lang.Object.wait(Native Method) - waiting on <0x00000000ede64800> (a org.eclipse.core.internal.jobs.Semaphore) at org.eclipse.core.internal.jobs.Semaphore.acquire(Semaphore.java:39) - locked <0x00000000ede64800> (a org.eclipse.core.internal.jobs.Semaphore) at org.eclipse.core.internal.jobs.OrderedLock.doAcquire(OrderedLock.java:176) at org.eclipse.core.internal.jobs.OrderedLock.acquire(OrderedLock.java:110) at org.eclipse.core.internal.jobs.OrderedLock.acquire(OrderedLock.java:84) at org.eclipse.equinox.internal.security.storage.SecurePreferencesRoot.getModulePassword(SecurePreferencesRoot.java:239) at org.eclipse.equinox.internal.security.storage.SecurePreferencesRoot.getPassword(SecurePreferencesRoot.java:224) at org.eclipse.equinox.internal.security.storage.SecurePreferences.put(SecurePreferences.java:224) at org.eclipse.equinox.internal.security.storage.SecurePreferencesWrapper.put(SecurePreferencesWrapper.java:110) at org.eclipse.mylyn.tasks.core.TaskRepository.addAuthInfo(TaskRepository.java:264) at org.eclipse.mylyn.tasks.core.TaskRepository.setCredentials(TaskRepository.java:755) - locked <0x00000000c5e442c8> (a org.eclipse.mylyn.tasks.core.TaskRepository) at org.eclipse.mylyn.internal.tasks.ui.TaskRepositoryLocationUi$PasswordRunner.apply(TaskRepositoryLocationUi.java:159) at org.eclipse.mylyn.internal.tasks.ui.TaskRepositoryLocationUi$PasswordRunner.run(TaskRepositoryLocationUi.java:116) at org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:164) at org.eclipse.ui.internal.UISynchronizer$3.run(UISynchronizer.java:158) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135) - locked <0x00000000fff53ce8> (a org.eclipse.swt.widgets.RunnableLock) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3563) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3212) at org.eclipse.jface.window.Window.runEventLoop(Window.java:825) at org.eclipse.jface.window.Window.open(Window.java:801) at org.eclipse.equinox.internal.security.ui.storage.DefaultPasswordProvider$1.run(DefaultPasswordProvider.java:49) at org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:164) at org.eclipse.ui.internal.UISynchronizer$3.run(UISynchronizer.java:158) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135) - locked <0x00000000fff53d10> (a org.eclipse.swt.widgets.RunnableLock) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3563) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3212) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2696) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2660)
It seems that the only way to prevent this is to never read or write encrypted key store values from the UI thread. That ensures that the locking order is always keystore -> UI.
Steffen, I've pushed a start on this to 9579: 357278: deadlock when opening secure storage [I4f404896] https://git.eclipse.org/r/#/c/9579/
Since opening the secure store can trigger a password prompt, we should create some way for clients to test whether it has already been opened so that they can attempt to delay opening it until it is really needed.
I am able to reproduce the deadlock consistently and so I've verified that this prevents it. However, this solution can actually cause a different deadlock. If you are holding onto any lock when you try to access the secure store, the modal context may spawn a thread that tries to access the same lock. This can happen when calling the synchronized method TaskRepository.getCredentials() which accesses the secure store. I don't see any way to prevent this (unless we can initialize the store on startup before anything else tries to access it, but this would cause an annoying prompt), but we'll need to fix this specific case and add documentation that the store should never be accessed while holding a lock.
I've created another review to address this issue for TaskRepository: 9685: potential deadlock when TaskRepository accesses secure storage [I3b454195] https://git.eclipse.org/r/#/c/9685/
(In reply to comment #3) > Since opening the secure store can trigger a password prompt, we should create > some way for clients to test whether it has already been opened so that they can > attempt to delay opening it until it is really needed. 9693: 357278: allow clients to determine if the secure store has been opened [I3945a620] https://git.eclipse.org/r/#/c/9693/ I don't exactly love this, but I'm not sure how else to do it.
Hey, I was trying to solve this issue, but then I have noticed that the stacktrace seems to have no deadlock - just look at the locks ids they're different. What's more, I have found this gem: "Another main feature of ILock is the interaction with Display#syncExec(). If the UI thread is blocked on an ILock#acquire(), it will still execute syncExecs from other threads that own locks." Since TaskRepositoryLocationUi$PasswordRunner.run is invoked by Display.syncExec, I'd not expect a lock here. Was there another thread involved in this lock?
Krzysztof, thanks for the tip (and sorry for the slow response). After looking into this again, I don't think the deadlock has anything to do with accessing the secure store on the UI thread. The deadlock happens when a non-UI-thread calls a synchronized method such as TaskRepository.getCredentials which accesses the secure store. If this is the first access to the secure store, it may (depending on the system and configuration) do a syncExec to prompt the user for the master password. If the UI thread is blocked waiting for the same synchronized method, there is a deadlock. Steps to reproduce: * start in a clean workspace * disable all master password providers other than the UI Prompt * add creds to eclipse.org Bugzilla repository * create a query * restart in debug mode with breakpoint at DefaultPasswordProvider.getPassword where it does the syncExec * sync the query to hit the breakpoint * try to open the repository properties * resume the paused thread Steffen, I can't see how to produce a deadlock on the SecurePreferencesRoot lock. If I open up properties for a different repository than the one whose query is being synced, the main thread tries to acquire the lock and processes the syncExec from the other thread (prompting for the master password) as Krzysztof described. The deadlock I _can_ reproduce is easily fixed by using an ILock for synchronization in TaskRepository instead of @synchronized@ methods.
Ok, now I understand Steffen's deadlock, which is different than the one I have reproduced. It happens when the prompt for the master password blocks processing an event which tries to access the secure storage. Krzysztof, the problem isn't that the syncExec can't run because the main thread is trying to acquire the lock (as you pointed out), it's that the syncExec runs the event loop, which _then_ tries to acquire the lock. So, the master password prompt opens, but then the UI freezes because an event from another dialog tries to access the secure store. Because the UI is frozen, the user can't close the password prompt so the lock is never released.
Actually, it's not that the UI freezes (because the event loop keeps running while waiting for the semaphore), it's that the call to open() on the secure store password prompt doesn't return because it's processing an event which is waiting for the lock. At least in theory. But I can't reproduce it. What happens in practice is that even though another thread is holding the lock, the main thread is somehow able to acquire it. I'm not sure if this is due to deadlock detection or a bug.
Created attachment 238639 [details] threads Here's a screenshot. It looks like there's about to be a deadlock but there isn't.
(In reply to comment #8) > The deadlock I _can_ reproduce is easily fixed by using an ILock for > synchronization in TaskRepository instead of @synchronized@ methods. 20251: 357278: prevent deadlock on TaskRepository synchronized methods [Ia73cb74d] https://git.eclipse.org/r/#/c/20251/
(In reply to comment #6) > (In reply to comment #3) > > Since opening the secure store can trigger a password prompt, we should create > > some way for clients to test whether it has already been opened so that they > can > > attempt to delay opening it until it is really needed. > > 9693: 357278: allow clients to determine if the secure store has been opened > [I3945a620] > https://git.eclipse.org/r/#/c/9693/ > > I don't exactly love this, but I'm not sure how else to do it. I'm abandonning this as I don't think it's useful. Clients that need to access the secure store will just have to put up with the prompt. It's a shortcoming of the secure store implementation and I don't think we can reasonably work around it.
To sumarize, we have the following 2 open reviews: 9579: 357278: deadlock when opening secure storage [I4f404896] https://git.eclipse.org/r/#/c/9579/ 20251: 357278: prevent deadlock on TaskRepository synchronized methods [Ia73cb74d] https://git.eclipse.org/r/#/c/20251/ There are 3 distinct deadlocks that were raised on this bug: h3. 1. Deadlock opening secure storage on UI thread This was the described in comment 0 and is fixed by review 9579. A year ago I wrote some code that allowed me to reproduce it and I verified the fix. Unfortunately I no longer have that code and can't reproduce it now. h3. 2. Deadlock on TaskRepository synchronized methods This was described in comment 8 and is fixed by review 20251, using @ILock@ instead of the @synchronized@ keyword. h3. 3. Deadlock accessing TaskRepository credentials in nested modal operation This was described in comment 4 and is *not* fixed. It's a direct result of the fix for the first deadlock. I don't see any way to fix it (unless we can remove synchronization from TaskRepository entirely) but fortunately it doesn't seem like it could happen in practice. Steps to reproduce: # start eclipse with a breakpoint in the ModalContext in UiSecureCredentialsStore.getSecurePreferences() # open the properties for a repository, the breakpoint will be hit # open new query dialog on the same repository and click refresh config button # resume the paused thread from step 2
It occurs to me that we could significantly reduce the (already very small) chance of hitting deadlock #3 by doing something like this: 22132: 357278: open secure store before acquiring lock [I1b50b07a] https://git.eclipse.org/r/#/c/22132/ But I'm not sure what other implications that could have so I won't pursue it any further now.
The changes have been merged.
There is still a chance for dead locks when accessing credentials. I'm proposing that we remove locking all together assuming that accessing the secure store is thread safe: https://git.eclipse.org/r/#/c/22789/1 .
Thanks Steffen. I think this could cause issues, e.g. getCredentials could return a username and password that don't match if called at the same time as setCredentials, or the password could be removed from both the secure store and the transientProperties if setCredentials(..., true) and setCredentials(..., false) are called at the same time. However, these issues seem less severe than a deadlock, so I am leaning towards removing the locking anyway. But it's worth noting that this deadlock has been possible for a long time for repositories using the secure storage but was never reported as far as I know (until comment 8), and the issues that could result from removing the locking may come up a lot more than the deadlock did, and would be hard to diagnose.
I did some testing and the fix hasn't caused any problems so far. Looking at the change, it seems unlikely to be a problem in practice. I have merged the fix and cherry-picked it for 3.11.
