[SERVER-15392] geo_validate.js depends on undefined behaviour (and fails on arm64) Created: 25/Sep/14 Updated: 11/Jul/16 Resolved: 08/Oct/14 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Geo |
| Affects Version/s: | None |
| Fix Version/s: | 2.7.8 |
| Type: | Bug | Priority: | Minor - P4 |
| Reporter: | Michael Hudson-Doyle | Assignee: | Greg Studer |
| Resolution: | Done | Votes: | 0 |
| Labels: | arm64 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Operating System: | ALL |
| Participants: |
| Description |
|
This bit of geo_validate.js fails on arm64:
Digging shows this to be because of this code in geo_near_cmd.cpp:
.numberInt() on a double just casts the double value to an int. Unfortunately, I think casting a NaN to an int is undefined behaviour. On intel you get INT_MIN but on arm64 (and probably other platforms from what I read) you get 0, a perfectly valid value in this context, and so the test fails. |
| Comments |
| Comment by Greg Studer [ 08/Oct/14 ] |
|
The more-appropriate safeNumberLong() is now used to parse geoNear limits. |
| Comment by Githook User [ 06/Oct/14 ] |
|
Author: {u'username': u'gregstuder', u'name': u'Greg Studer', u'email': u'greg@10gen.com'}Message: |
| Comment by Michael Hudson-Doyle [ 25/Sep/14 ] |
|
FWIW, I patched numberInt to raise an exception when called on a NaN and ran the tests ... some failed because my VM didn't have enough disk (sigh) but most passed. So the test suite, at least, doesn't depend much on this behaviour. I suppose isNumber returning false on NaN would be more honest somehow (but then what about infinities? Or just doubles too large to fit in an integer? Should numberInt return INT_MAX or fail in these cases?) The most backwards-compatible-yet-avoiding-undefined-behaviour fix would be "if std::isnan(v) return INT_MIN" but that seems, well, a bit lame. |
| Comment by Greg Studer [ 25/Sep/14 ] |
|
Agree that our test suite at least should not rely on this behavior. In general we may want isNumber() to not return true when the value is NaN, but this will require a bunch of auditing to determine the callers who actually want to process NaN. |
| Comment by Michael Hudson-Doyle [ 25/Sep/14 ] |
|
I don't really know what a good fix for this is. I guess mostly one wants .numberInt() to fail when called on a NaN value – I guess it could raise an exception but I don't know enough about how mongod is put together to know if that's the Right Thing. |