Info

When assert.throws() calls the given function, this is not set. This means functions that rely on this being set (eg. most member functions of DB or DBCollection) are highly likely to throw an exception when this turns out to be null. This makes the assert.throws() test succeed, but not because the intended assertion was thrown. For example, consider from jstests/sharding/authmr.js:69: assert.throws(test2DB.foo.count); However, the count function has calls to this.find and this.getName, which cause an exception: > db.foo.count() 8 > if (assert.throws(db.foo.count)) print("success") success > assert.throws(db.foo.count) TypeError: this.find is not a function There are up to 625 uses of assert.throws, of which up to 56 may be affected: $ grep -nr 'assert\.throws' jstests | grep -v 'assert\.throws\(\.automsg\)\?\s*(\s*function\s*(' | grep 'assert\.throws' jstests/dur/diskfull.js:105: assert.throws( work, null, "no exception thrown when exceeding disk capacity" ); jstests/ssl/libs/ssl_helpers.js:44: assert.throws(load,[replSetTestFile], jstests/sharding/replmonitor_bad_seed.js:46:assert.throws(verifyInsert); jstests/sharding/version2.js:52:assert.throws( simpleFindOne , [] , "should complain about not in sharded mode 1" ); jstests/sharding/authmr.js:69: assert.throws(test2DB.foo.count); jstests/sharding/authwhere.js:66: assert.throws(test2DB.foo.count); jstests/sharding/authwhere.js:69: assert.throws(test1DB.foo.count, ["db.getSiblingDB('test2').foo.count() == 1"]); jstests/sharding/authwhere.js:72: assert.throws(test1DB.foo.count, ["db.foo.insert({b: 1})"]); jstests/core/counta.js:11:assert.throws( jstests/core/geo_s2disjoint_holes.js:35:print(assert.throws( jstests/core/geo_s2disjoint_holes.js:42:print(assert.throws( jstests/core/distinct4.js:18: assert.throws( t.distinct, [{a:1}] ); jstests/core/distinct4.js:28: assert.throws( t.distinct, ['a', '1'] ); jstests/core/basic3.js:21:assert.throws(doBadSave, [{"a.b":5}], ". in names aren't allowed doesn't work"); jstests/core/basic3.js:23:assert.throws(doBadSave, jstests/core/basic3.js:30:assert.throws(doBadUpdate, [{a:0}, { "b.b" : 1 }], jstests/core/basic3.js:34:assert.throws(doBadUpdate, [{a:10}, { c: {"b.b" : 1 }}], jstests/core/basic3.js:42:assert.throws(doBadUpdate, [{a:0}, { "":{"b.b" : 1} }], jstests/core/indexes_on_indexes.js:16: assert.throws(t.system.indexes.createIndex({_id:1})); jstests/core/indexes_on_indexes.js:21: assert.throws(t.system.indexes.insert({ v:1, jstests/core/basic9.js:16:assert.throws(doBadSave, [{$foo:5}], "key names aren't allowed to start with $ doesn't work"); jstests/core/basic9.js:17:assert.throws(doBadSave, jstests/core/shellstartparallel.js:4:assert.throws(f); jstests/core/error2.js:9:assert.throws( jstests/core/error2.js:16:assert.throws( jstests/core/fts_partition1.js:13:assert.throws(t.find( { "$text": { "$search" : "foo" } } )); jstests/core/cappeda.js:23://assert.throws( q , [] , "A1" ); jstests/core/cappeda.js:24://assert.throws( u , [] , "B1" ); jstests/core/fts_mix.js:72:assert.throws(tc.find( { "$text": { "$search": "member", $language: "spanglish" } } )); jstests/core/geo_2d_with_geojson_point.js:14:print(assert.throws( jstests/core/push_sort.js:65:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [ 2 ], $slice:-2, $sort:{a:1} } } } ) ) jstests/core/push_sort.js:68:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2},1], $slice:-2, $sort:{a:1} } } })) jstests/core/push_sort.js:71:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort:{} } } } ) ) jstests/core/push_sort.js:74:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:2, $sort: {a:1} } } })) jstests/core/push_sort.js:77:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2.1, $sort: {a:1} } }})) jstests/core/push_sort.js:80:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort: {a:-2} } } })) jstests/core/push_sort.js:84:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort: 1 } } } ) ) jstests/core/push_sort.js:87:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort: {'a.':1} }}})) jstests/core/push_sort.js:90:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort: {'':1} } } })) jstests/core/push_sort.js:94:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $xxx: {s:1} } } } ) ) jstests/core/push_sort.js:101:assert.throws(t.update( jstests/core/ord.js:34:assert.throws( c.next() ); jstests/core/invalid_db_name.js:13: assert.throws(doWrite); jstests/concurrency/fsm_selftests.js:19: // when assert.throws executes jstests/replsets/maintenance.js:74:var ex = assert.throws( jstests/replsets/server8070.js:104:// the sleep 30sec is a hold over from the unsafe assert.throws(assert.soon()) jstests/replsets/stepdown3.js:39: throw new Error("satisfy assert.throws()"); jstests/replsets/stepdown3.js:44:var result = assert.throws(gleFunction); jstests/multiVersion/libs/verify_collection_data.js:145: assert.throws(myValidator.validateCollectionData, [collection], "Validation function should have thrown since we modified the collection"); jstests/multiVersion/libs/verify_collection_data.js:153: assert.throws(myValidator.validateCollectionData, [collection], "Validation function should have thrown since we modified the collection"); jstests/multiVersion/libs/verify_collection_data.js:167: assert.throws(myValidator.validateCollectionData, [collection], "Validation function should have thrown since we modified the collection"); jstests/multiVersion/libs/verify_collection_data.js:175: assert.throws(myValidator.validateCollectionData, [collection], "Validation function should have thrown since we modified the collection"); jstests/noPassthroughWithMongod/log_component_helpers.js:35: assert.throws(mongo.setLogLevel, [2, 24]); jstests/noPassthroughWithMongod/log_component_helpers.js:36: assert.throws(db.setLogLevel, [2, ["array", "not.allowed"]]); jstests/noPassthroughWithMongod/no_balance_collection.js:6:assert.throws(sh.disableBalancing, [], "sh.disableBalancing requires a collection"); jstests/noPassthroughWithMongod/no_balance_collection.js:7:assert.throws(sh.enableBalancing, [], "sh.enableBalancing requires a collection"); Possible solutions: Make assert.throws accept a this parameter which gets passed on to func.apply. Make callers of assert.throws declare upfront a substring that the assertion should contain. This can be achieved manually by using String.prototype.includes on the result of assert.throws, but there's no way to enforce that callers of assert.throws do this (and it's also complicated because assert.throws can return undefined or a non-string). By contrast, assert.throws(func, params, requiredAssertion, msg) would only pass if func(params) throws an exception and the exception contains requiredAssertion as a substring. Instead of passing member functions directly to assert.throws, wrap them in an anonymous function (variables/functions will still be available via a closure) which calls the member normally, thereby ensuring that this is set as expected. > if (assert.throws(function () { db.foo.count(); })) print("success") assert: did not throw exception: undefined doassert@src/mongo/shell/assert.js:15:14 assert.throws@src/mongo/shell/assert.js:229:5 @(shell):1:5 2015-10-23T15:15:48.446+1100 E QUERY [thread1] Error: did not throw exception: undefined : doassert@src/mongo/shell/assert.js:15:14 assert.throws@src/mongo/shell/assert.js:229:5 @(shell):1:5 Notice (somehow) if the assertion fails due to a problem with this, and do not consider that a valid assertion (for the purposes of assert.throws passing). Eg. if it matches ^TypeError: this\.[^ ]\+ is not a function$ #1 and #2 are probably better for preventing re-occurrences of the problem being introduced in the future, but will involve widespread changes, while #3 is likely to be much quicker/easier. #4 makes me nervous about falsely flagging assertions that happen to legitimately involve this, as well as needing to enumerate all the ways that this misuse of assert.throws can manifest.

Top User Comments

xgen-internal-githook commented on Thu, 27 Oct 2016 15:36:14 +0000: Author: {u'username': u'guoyr', u'name': u'Robert Guo', u'email': u'robert.guo@10gen.com'} Message: SERVER-21089 fix assert.throw/doesNotThrow() argument validation SERVER-21087 prevent use of "this" in the function passed to assert.throw/doesNotThrow() Branch: master https://github.com/mongodb/mongo/commit/5b93d767f48469ac7a9212b2013f8349a30e1f70 max.hirschhorn@10gen.com commented on Wed, 5 Oct 2016 20:38:44 +0000: A more robust way to satisfy option #4 would be to invoke the specified function with a Proxy object as the this value that sets some internal state to true if any handlers on the Proxy object are invoked. assert.throws = function(func, params, msg) { ... let thisWasUsed = false; const thisSpy = new Proxy({}, { has: function(target, property) { thisWasUsed = true; return false; }, get: function get(target, property, receiver) { thisWasUsed = true; return target[property]; }, set: function set(target, property, value, receiver) { thisWasUsed = true; return false; }, }); let error; try { func.apply(thisSpy, params); } catch (e) { error = e; } if (thisWasUsed) { doassert("Attempted to access 'this' during function call: " + tojson(error)); } else if (error === undefined) { doassert("did not throw exception: " + msg); } return error; }; We should see if any other handlers should be trapped by the Proxy object in addition to the has, get, and set handlers above.

Steps to Reproduce