[SERVER-37183] BSONElement::safeNumberLong is not safe Created: 18/Sep/18  Updated: 29/Oct/23  Resolved: 13/Dec/18

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: 4.0.7, 4.1.7

Type: Bug Priority: Major - P3
Reporter: Martin Neupauer Assignee: Backlog - Query Team (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Backports
Depends
Related
related to SERVER-48286 Handle accuracy issues of casting lon... Closed
is related to SERVER-38623 Make safeNumberLongForHash consistent... Closed
Assigned Teams:
Query
Backwards Compatibility: Fully Compatible
Operating System: ALL
Backport Requested:
v4.0
Participants:
Linked BF Score: 61

 Description   

BSONElement::safeNumberLong is not as safe as it may seem. Consider the following simple program:

#include <limits>
#include <iostream>
int main()
{
    //9223372036854775807
    //0x7fffffffffffffff
    long long l1 = std::numeric_limits<long long>::max();
    std::cout << l1 << "\n";
 
    // convert the long long to double
    // The 0x7fffffffffffffff is not representable exactly as double
    // and the final result is greater than the original value.
    // It means that we will hit UB when going back to long long.
    double d = (double)l1;
 
    // In our case d and long long max are EQUAL so we will NOT enter
    // the branch.
    if (d > (double)std::numeric_limits<long long>::max()) {
        std::cout << "safe\n";
    }
 
    if (d < std::numeric_limits<long long>::min()) {
        std::cout << "safe\n";
    }
 
    // And hence BOOM! We hit the UB.
    //-9223372036854775808
    //0x8000000000000000
    long long l2 = (long long)d;
     std::cout << l2 << "\n";
 
    return 0;
}

 I.e. there exist a double value that is greater than the long long max and yet is fails to be clamped by safeNumberLong.

 

Please note that safeNumberLong is called as part of the hash index processing. It means that we may have computed and stored wrong hash values to disk. If safeNumberLong gets fixed it may start generating different hash values leading to incorrect query results.



 Comments   
Comment by Githook User [ 20/Feb/19 ]

Author:

{'name': 'Justin Seyster', 'username': 'jseyster', 'email': 'justin.seyster@mongodb.com'}

Message: SERVER-37183 Safer bound for safeNumberLong()

(cherry picked from commit 1582fb6cce63c8e5691a14f8de2db4b3fbe42873)
Branch: v4.0
https://github.com/mongodb/mongo/commit/8d330f31c562e46302558c8c3f2dd1e82048dbe0

Comment by Githook User [ 07/Dec/18 ]

Author:

{'name': 'Justin Seyster', 'email': 'justin.seyster@mongodb.com', 'username': 'jseyster'}

Message: SERVER-37183 Safer bound for safeNumberLong()
Branch: master
https://github.com/mongodb/mongo/commit/1582fb6cce63c8e5691a14f8de2db4b3fbe42873

Comment by Justin Seyster [ 03/Nov/18 ]

I've made progress on this, but it needs some more time before it's ready for code review. I can continue work next BF Friday. I plan to use the kLongLongMaxPlusOneAsDouble bound for the positive overflow check, which is what $convert does.

Comment by Ian Whalen (Inactive) [ 11/Oct/18 ]

Assigning to Dave to figure out what is the impact of the linked test failure.

Generated at Thu Feb 08 04:45:16 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.