[SERVER-51231] suspicious long long to double conversions in bounds checks Created: 30/Sep/20  Updated: 27/Oct/23  Resolved: 23/May/22

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Billy Donahue Assignee: Billy Donahue
Resolution: Gone away Votes: 0
Labels: internal-sa-move
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Problem/Incident
is caused by SERVER-46071 Server parameters with AtomicWord<int... Closed
Related
is related to SERVER-51084 MongoDB fails to compile on Mac with ... Closed
Operating System: ALL
Steps To Reproduce:

buildscripts/scons.py --ssl --variables-files=etc/scons/xcode_macosx.vars --libc++ --detect-odr-violations --dbg=on --opt=off --ninja --build-tools=next VARIANT_DIR=ninja generate-ninja

Emits warnings treated as errors.
Reviewing these shows what look like incorrect range checks.

Sprint: Service Arch 2022-2-21, Service Arch 2022-03-07, Service Arch 2022-03-21, Service Arch 2022-04-04, Service Arch 2022-04-18, Service Arch 2022-05-02, Service Arch 2022-05-16, Service Arch 2022-05-30
Participants:
Story Points: 2

 Description   

(found by XCode12 clang warnings which appear to be true positives)

We have several scenarios where double values are compared to LONG_LONG_MAX to determine whether they are representable. The problem is LONG_LONG_MAX (2^63 - 1) doesn't cleanly map to any double, and it could be rounded up to the double representation of LONG_LONG_MAX+1, (2^63), before comparison.
So a double having the value LONG_LONG_MAX+1, or perhaps even LONG_LONG_MAX+1000 will pass a range check because the 1001 of error is still beyond a double's 53 bit precision.

There could be valuable bugs here, as double values near LONG_LONG_MAX will appear unrepresentable to coerce and formatting functions when they actually are fine, or vice versa, depending on the code and relops in question. We have unit tests with the same bug, including safe_num_test, so it may look like these converters are coming up with the right answer and passing unit tests when they aren't actually correct.

summation_test.cpp etc don't cover the highest values of the long long range very well and have the same casting errors.

It would be nice to see if we could craft some custom-built tests that fail because of these casts. I haven't tried that.

Not for review yet, but this CR shows where the warnings are. A few other minor tweaks are in the CR. Ignore those.
https://mongodbcr.appspot.com/663460026/

AC: Investigate within the time-boxed story point time.



 Comments   
Comment by Billy Donahue [ 23/May/22 ]

XCode12 is the production tool now, so these issues were fixed while rolling it out.

Comment by Billy Donahue [ 30/Sep/20 ]

The problem illustrated.

https://gcc.godbolt.org/z/fv3369

#include <iostream>
#include <iomanip>
#include <limits>
int main() {
  auto d = 0x1p63;
  auto m = std::numeric_limits<long long>::max();
  std::cout << std::setprecision(100) << std::boolalpha
            << d << " > " << m << " = " << (d > m) << "\n";
  return 0;
}
 
9223372036854775808 > 9223372036854775807 = false

Comment by Billy Donahue [ 30/Sep/20 ]

SERVER-51084 is about the XCode warnings that pointed out this issue to us

Generated at Thu Feb 08 05:24:52 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.