[SERVER-69009] Narrowing conversion of timestamp components in mozjs valuewriter Created: 19/Aug/22 Updated: 29/Oct/23 Resolved: 14/Dec/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 6.3.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Dan Larkin-York | Assignee: | Justin Seyster |
| 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 | ||||||||||||||||
| Sprint: | QE 2022-09-19, QE 2022-10-03, QE 2022-10-17, QE 2022-10-31, QE 2022-11-14, QE 2022-11-28, QE 2022-12-12, QE 2022-12-26 | ||||||||||||||||
| Participants: | |||||||||||||||||
| Linked BF Score: | 59 | ||||||||||||||||
| Description |
|
We could experience some unexpected, implementation-defined behavior if the components of the timestamp are doubles with values outside the allowed integer range. src/mongo/scripting/mozjs/valuewriter.cpp:302:22: warning: narrowing conversion from 'double' to 'unsigned int' [bugprone-narrowing-conversions] |
| Comments |
| Comment by Githook User [ 14/Dec/22 ] | ||||||||||||||||
|
Author: {'name': 'Justin Seyster', 'email': 'justin.seyster@mongodb.com', 'username': 'jseyster'}Message: | ||||||||||||||||
| Comment by Dan Larkin-York [ 08/Sep/22 ] | ||||||||||||||||
|
justin.seyster@mongodb.com Thanks for taking a look! I also agree the usassert approach sounds quite reasonable. If we backport the changes, there's some small chance this will cause errors in existing scripts/applications, but I think that would be preferable to silently doing something potentially wrong due to undefined behavior. And we can always just not backport it, since it seems a low-risk bug anyway. But I'll leave that decision up to your judgment. | ||||||||||||||||
| Comment by Mohammad Dashti (Inactive) [ 08/Sep/22 ] | ||||||||||||||||
|
justin.seyster@mongodb.com, thanks for taking a look into it. We've already discussed this on this Slack thread: https://mongodb.slack.com/archives/C03STQ6G8RL/p1660831758698009 I agree with justin.seyster@mongodb.com's suggestion, as a similar change is done for this kind of issue (e.g., here):
| ||||||||||||||||
| Comment by Justin Seyster [ 07/Sep/22 ] | ||||||||||||||||
|
There is a hidden bug here, but it's minor. These conversions should be correct, because the constructor for JavaScript Timestamp objects validates that both t and i values are within the bounds of a uint32_t. However, it is possible to bypass validation by assigning to an already constructed Timestamp:
I don't think this will affect customers, though, and we probably don't want to spend a lot of time on it. My suggestion is to add uassert}}s at the two locations referenced by this ticket to ensure that {{t and o are still in uint32_t range. Users will still be able to pass around invalid Timestamp objects, but any time they cross the barrier from JavaScript to mongod, the ranges will be revalidated. The bulletproof solution would be modifying the C++ wrapper that specifies the `Timestamp` object so that t and {i}} are getter/setter properties, giving us a hook we can use to validate new values. I don't know if it's possible to add a setter-style function to the kind of object `Timestamp` is, however, and JavaScript may still provide a way to make an unchecked modification to the underlying fields anyway. | ||||||||||||||||
| Comment by Kyle Suarez [ 30/Aug/22 ] | ||||||||||||||||
|
arun.banala@mongodb.com / steve.tarzia@mongodb.com, does either Justin or Mohammad have bandwidth to take a look at this? |