Unsafe async behavior in DefaultShapeTableManager
#16445
Comments
|
Please see this @sebastienros @MikeAlhayek. |
True, that lock is absolutely useless. |
|
Exactly. We need to use a Semaphore here instead of a lock. And maybe to a pass on all the locks we use to ensure we don't repro the same pattern (non-async code that return tasks). |
|
We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues). This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here. |
Describe the bug
In cases when
DefaultShapeTableManager.GetShapeTableAsync()is called from multiple requests in a quick succession before the cache is built, it causes a database deadlock. This can break the site although only in a short time frame after startup so the threat impact is limited. Still, it should be fixed asap.Orchard Core version
Latest main, all previews since #15651.
To Reproduce
I wasn't able to create an easy repro, but I've experienced this in OrchardCore.Commerce (OrchardCMS/OrchardCore.Commerce#454) when trying to run dynamic security scan using Zed Attack Proxy security tool.
Expected behavior
It should not be possible to cause such a deadlock. I see two options:
When the shape table cache has to be built, it should actually lock. Note that right now it basically does this:
lock (_lock) { return task; }. This is not an effective way of locking, because the lock may be released before the non-awaited task is finished.SemaphoreSlimcould be used instead oflock.Alternatively,
IShapeTableManager.GetShapeTableAsync()should be called as soon as possible to prevent this scenario. This means calling it on the current main theme immediately after startup, and calling it before/during theme changed. This approach works for the specific scenario where I encountered the problem as mentioned above. I created an MVC action (see here) that callsIShapeTableManager.GetShapeTableAsync(), and then navigated to that action at the start of the security scanning UI test. This is a bit janky, but I feel it's a good proof of concept.Logs and screenshots
An example of the errors encountered while security testing OrchardCore.Commerce can be seen below. Note that nothing from OCC is called above the middleware line, which is proof that the problem is internal to OrchardCore.
The text was updated successfully, but these errors were encountered: