diff options
4 files changed, 58 insertions, 7 deletions
diff --git a/jstests/core/collmod_bad_spec.js b/jstests/core/collmod_bad_spec.js new file mode 100644 index 00000000000..43d7c43882d --- /dev/null +++ b/jstests/core/collmod_bad_spec.js @@ -0,0 +1,23 @@ +// This is a regression test for SERVER-21545. +// +// Tests that a collMod with a bad specification does not cause any changes, and does not crash the +// server. +(function() { + "use strict"; + + var collName = "collModBadSpec"; + var coll = db.getCollection(collName); + + coll.drop(); + assert.commandWorked(db.createCollection(collName)); + + // Get the original collection options for the collection. + var originalResult = db.getCollectionInfos({name: collName}); + + // Issue an invalid command. + assert.commandFailed(coll.runCommand("collMod", {usePowerOf2Sizes: true, unknownField: "x"})); + + // Make sure the options are unchanged. + var newResult = db.getCollectionInfos({name: collName}); + assert.eq(originalResult, newResult); +})(); diff --git a/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.cpp b/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.cpp index 0c70f9959ea..d8b32fbe47c 100644 --- a/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.cpp +++ b/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.cpp @@ -39,6 +39,7 @@ #include "mongo/db/storage/mmap_v1/catalog/namespace_details_rsv1_metadata.h" #include "mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.h" #include "mongo/db/storage/record_store.h" +#include "mongo/db/storage/recovery_unit.h" #include "mongo/util/log.h" #include "mongo/util/startup_test.h" @@ -58,7 +59,7 @@ NamespaceDetailsCollectionCatalogEntry::NamespaceDetailsCollectionCatalogEntry( _namespacesRecordStore(namespacesRecordStore), _indexRecordStore(indexRecordStore), _db(db) { - setNamespacesRecordId(namespacesRecordId); + setNamespacesRecordId(nullptr, namespacesRecordId); } CollectionOptions NamespaceDetailsCollectionCatalogEntry::getCollectionOptions( @@ -363,7 +364,7 @@ void NamespaceDetailsCollectionCatalogEntry::_updateSystemNamespaces(OperationCo StatusWith<RecordId> result = _namespacesRecordStore->updateRecord( txn, _namespacesRecordId, newEntry.objdata(), newEntry.objsize(), false, NULL); fassert(17486, result.getStatus()); - _namespacesRecordId = result.getValue(); + setNamespacesRecordId(txn, result.getValue()); } void NamespaceDetailsCollectionCatalogEntry::updateFlags(OperationContext* txn, int newValue) { @@ -372,13 +373,37 @@ void NamespaceDetailsCollectionCatalogEntry::updateFlags(OperationContext* txn, _updateSystemNamespaces(txn, BSON("$set" << BSON("options.flags" << newValue))); } -void NamespaceDetailsCollectionCatalogEntry::setNamespacesRecordId(RecordId newId) { +namespace { +class NamespacesRecordIdChange : public RecoveryUnit::Change { +public: + NamespacesRecordIdChange(RecordId* namespacesRecordId, const RecordId& newId) + : _namespacesRecordId(namespacesRecordId), _oldId(*namespacesRecordId) {} + + void commit() override {} + void rollback() override { + *_namespacesRecordId = _oldId; + } + +private: + RecordId* _namespacesRecordId; + const RecordId _oldId; +}; +} + +void NamespaceDetailsCollectionCatalogEntry::setNamespacesRecordId(OperationContext* txn, + RecordId newId) { if (newId.isNull()) { invariant(ns().coll() == "system.namespaces" || ns().coll() == "system.indexes"); } else { - // We don't need an OperationContext in MMAP, so 'nullptr' is OK. - auto namespaceEntry = _namespacesRecordStore->dataFor(nullptr, newId).releaseToBson(); + // 'txn' is allowed to be null, but we don't need an OperationContext in MMAP, so that's OK. + auto namespaceEntry = _namespacesRecordStore->dataFor(txn, newId).releaseToBson(); invariant(namespaceEntry["name"].String() == ns().ns()); + + // Register RecordId change for rollback if we're not initializing. + if (txn && !_namespacesRecordId.isNull()) { + txn->recoveryUnit()->registerChange( + new NamespacesRecordIdChange(&_namespacesRecordId, newId)); + } _namespacesRecordId = newId; } } diff --git a/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.h b/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.h index 2128fda14cd..44b7bfb6051 100644 --- a/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.h +++ b/src/mongo/db/storage/mmap_v1/catalog/namespace_details_collection_entry.h @@ -103,7 +103,10 @@ public: return _namespacesRecordId; } - void setNamespacesRecordId(RecordId newId); + /** + * 'txn' is only allowed to be null when called from the constructor. + */ + void setNamespacesRecordId(OperationContext* txn, RecordId newId); private: NamespaceDetails* _details; diff --git a/src/mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.cpp b/src/mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.cpp index 3a5ada58046..d7c0c4fe8e9 100644 --- a/src/mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.cpp +++ b/src/mongo/db/storage/mmap_v1/mmap_v1_database_catalog_entry.cpp @@ -574,7 +574,7 @@ void MMAPV1DatabaseCatalogEntry::_init(OperationContext* txn) { // because they don't have indexes on them anyway. if (entry) { if (entry->catalogEntry->getNamespacesRecordId().isNull()) { - entry->catalogEntry->setNamespacesRecordId(rid); + entry->catalogEntry->setNamespacesRecordId(txn, rid); } else { invariant(entry->catalogEntry->getNamespacesRecordId() == rid); } |