[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:

assert.commandFailed(db.runCommand({geoNear: coll.getName(),
                                    near: [0,0], spherical: true, num: NaN}));

Digging shows this to be because of this code in geo_near_cmd.cpp:

            if (!eNumWanted.eoo()) {
                uassert(17303, "limit must be number", eNumWanted.isNumber());
                numWanted = eNumWanted.numberInt();
                uassert(17302, "limit must be >=0", numWanted >= 0);
            }

.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: SERVER-15392 safely parse geoNear command limit as long value
Branch: master
https://github.com/mongodb/mongo/commit/434f8fc1cfc364d40e2bb7df742412a58c5907ae

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.

Generated at Thu Feb 08 03:37:53 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.