[SERVER-21087] Some uses of assert.throws() always pass Created: 23/Oct/15  Updated: 31/Mar/17  Resolved: 27/Oct/16

Status: Closed
Project: Core Server
Component/s: Testing Infrastructure
Affects Version/s: None
Fix Version/s: 3.4.0-rc2

Type: Bug Priority: Major - P3
Reporter: Kevin Pulo Assignee: Robert Guo (Inactive)
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on SERVER-26503 Fix incorrect uses of assert.throws() Closed
Related
related to SERVER-12476 assert() calls that can't fail becaus... Closed
is related to SERVER-21089 assert.throws() doesn't check 2nd arg... Closed
is related to SERVER-28567 assert.throws should validate that th... Backlog
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: TIG 2016-10-10, TIG 2016-10-31
Participants:

 Description   

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:

  1. Make assert.throws accept a this parameter which gets passed on to func.apply.
  2. 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.
  3. 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
    

  4. 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.



 Comments   
Comment by Githook User [ 27/Oct/16 ]

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

Comment by Max Hirschhorn [ 05/Oct/16 ]

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.

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