[SERVER-32007] ExpressionDateFromString::evaluate() is not thread safe Created: 16/Nov/17 Updated: 30/Oct/23 Resolved: 17/Nov/17 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Aggregation Framework |
| Affects Version/s: | 3.6.0-rc4 |
| Fix Version/s: | 3.6.0-rc5, 3.7.1 |
| Type: | Bug | Priority: | Critical - P2 |
| Reporter: | David Storch | Assignee: | David Storch |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Operating System: | ALL | ||||||||
| Backport Requested: |
v3.6
|
||||||||
| Sprint: | Query 2017-12-04 | ||||||||
| Participants: | |||||||||
| Linked BF Score: | 0 | ||||||||
| Description |
|
In general, aggregation expressions are owned by a single thread responsible for executing the aggregate command. However, this is not the case for a document validator which uses $expr. A thread running a create or collMod commands sets up the validator. Subsequent writes to the collection can concurrently evaluate the aggregation expression, as part of $expr. Note that this is the only the case for storage engines which support document-level concurrency; on MMAPv1, writes will take an exclusive collection lock and thus will not evaluate the $expr concurrently. Expression::evaluate() is marked as const and therefore is expected to be thread-safe, but ExpressionDateFromString's evaluate implementation has a subtle data race. This expression holds a subexpression by boost::intrusive_ptr: It passes this intrusive_ptr by value to the makeTimeZone() helper: This causes the intrusive_ptr refcount to be incremented when this function is called, and decremented when the function returns. This increment/decrement pair amounts to mutating the object inside a const method, and it results in a data race. The Expression class inherits from IntrusiveCounterUnsigned in order to interface with instrusive_ptr. IntrusiveCounterUnsigned writes directly to an unsigned integer on increment and decrement: This is in contrast to our RefCountable class, whose refcount is an AtomicUInt32. |
| Comments |
| Comment by Githook User [ 17/Nov/17 ] |
|
Author: {'name': 'David Storch', 'username': 'dstorch', 'email': 'david.storch@10gen.com'}Message: (cherry picked from commit 8e06e18365f4efe772c924fa377eaaa0f1ddd219) |
| Comment by Githook User [ 17/Nov/17 ] |
|
Author: {'name': 'David Storch', 'username': 'dstorch', 'email': 'david.storch@10gen.com'}Message: (cherry picked from commit 411853747b6c5ee4e6d0f5d8ef063514e54d7ee0) |
| Comment by Githook User [ 17/Nov/17 ] |
|
Author: {'name': 'David Storch', 'username': 'dstorch', 'email': 'david.storch@10gen.com'}Message: |
| Comment by Githook User [ 17/Nov/17 ] |
|
Author: {'name': 'David Storch', 'username': 'dstorch', 'email': 'david.storch@10gen.com'}Message: |
| Comment by David Storch [ 16/Nov/17 ] |
|
This bug has been observed to cause a crash for ExpressionDateFromString in our continuous integration system. It affects all of the 3.6 release candidates, though since $expr and ExpressionDateFromString are new, it does not affect old branches. The crash is likely a result of the data race causing the Expression to be de-allocated when there is still an outstanding reference. It appears from the code that this affects other expressions, not just ExpressionDateFromString. At the very least, all of the evaluate() methods that call makeTimeZone() are likely to be affected. It's possible that there are other Expression::evaluate() implementations that are subtly non-const due to writes to the intrusive_ptr refcount. Any such expression falls under the purview of this ticket as well. Finally, we should investigate whether there is a similar problem affecting any of the features which can persist query expressions:
|