From 16aff18cbb6b993ac325a50a59b1898d35cf08c3 Mon Sep 17 00:00:00 2001 From: "A. Jesse Jiryu Davis" Date: Mon, 15 Feb 2021 07:34:37 -0500 Subject: SERVER-54482 Fix crash if collation.backwards param is undefined/null --- jstests/core/collation.js | 15 +++++++++++++++ src/mongo/db/query/canonical_query_encoder.cpp | 2 +- src/mongo/db/query/collation/collator_factory_icu.cpp | 6 +++--- .../db/query/collation/collator_factory_icu_test.cpp | 8 ++++---- src/mongo/idl/basic_types.idl | 6 +++--- 5 files changed, 26 insertions(+), 11 deletions(-) diff --git a/jstests/core/collation.js b/jstests/core/collation.js index 15142a7659f..ff856ba8ea8 100644 --- a/jstests/core/collation.js +++ b/jstests/core/collation.js @@ -1676,6 +1676,21 @@ if (db.getMongo().writeMode() === "commands") { }); } +// updateOne with undefined/null collation.backwards parameter (SERVER-54482). +if (db.getMongo().writeMode() === "commands") { + for (let backwards of [undefined, null]) { + assert.throws(function() { + coll.bulkWrite([{ + updateOne: { + filter: {str: 'foo'}, + update: {$set: {str: 'bar'}}, + collation: {locale: 'en_US', backwards: backwards} + } + }]); + }); + } +} + // updateMany with bulkWrite(). coll.drop(); assert.commandWorked(coll.insert({_id: 1, str: "foo"})); diff --git a/src/mongo/db/query/canonical_query_encoder.cpp b/src/mongo/db/query/canonical_query_encoder.cpp index f5c610004d0..e8ef4308158 100644 --- a/src/mongo/db/query/canonical_query_encoder.cpp +++ b/src/mongo/db/query/canonical_query_encoder.cpp @@ -367,7 +367,7 @@ void encodeCollation(const CollatorInterface* collation, StringBuilder* keyBuild *keyBuilder << encodeEnum(spec.getAlternate()); *keyBuilder << encodeEnum(spec.getMaxVariable()); *keyBuilder << spec.getNormalization(); - *keyBuilder << spec.getBackwards().value_or(false); + *keyBuilder << spec.getBackwards(); // We do not encode 'spec.version' because query shape strings are never persisted, and need // not be stable between versions. diff --git a/src/mongo/db/query/collation/collator_factory_icu.cpp b/src/mongo/db/query/collation/collator_factory_icu.cpp index b2d90d1b3d0..499f19384d3 100644 --- a/src/mongo/db/query/collation/collator_factory_icu.cpp +++ b/src/mongo/db/query/collation/collator_factory_icu.cpp @@ -396,9 +396,9 @@ Status updateCollationSpecFromICUCollator(const BSONObj& spec, } else { UErrorCode status = U_ZERO_ERROR; // collation->getBackwards should be engaged if spec has a "backwards" field. - invariant(collation->getBackwards().is_initialized()); + invariant(collation->getBackwards().has_value()); icuCollator->setAttribute( - UCOL_FRENCH_COLLATION, boolToAttribute(*collation->getBackwards()), status); + UCOL_FRENCH_COLLATION, boolToAttribute(collation->getBackwards()), status); if (U_FAILURE(status)) { icu::ErrorCode icuError; icuError.set(status); @@ -466,7 +466,7 @@ Status validateLocaleID(const BSONObj& spec, StringData originalID, const icu::C Status validateCollationSpec(const Collation& collation, const BSONObj& spec) { // The backwards option specifically means backwards secondary weighting, and therefore only // affects the secondary comparison level. It has no effect at strength 1. - if (collation.getBackwards().value_or(false) && + if (collation.getBackwards() && static_cast(collation.getStrength()) == CollationStrength::kPrimary) { return {ErrorCodes::BadValue, str::stream() << "'" << Collation::kBackwardsFieldName << "' is invalid with '" diff --git a/src/mongo/db/query/collation/collator_factory_icu_test.cpp b/src/mongo/db/query/collation/collator_factory_icu_test.cpp index f0a08fdf97a..bb62deaf241 100644 --- a/src/mongo/db/query/collation/collator_factory_icu_test.cpp +++ b/src/mongo/db/query/collation/collator_factory_icu_test.cpp @@ -418,7 +418,7 @@ TEST(CollatorFactoryICUTest, DefaultsSetSuccessfully) { ASSERT_EQ(static_cast(CollationMaxVariableEnum::kPunct), static_cast(collator.getValue()->getSpec().getMaxVariable())); ASSERT_FALSE(collator.getValue()->getSpec().getNormalization()); - ASSERT_FALSE(*collator.getValue()->getSpec().getBackwards()); + ASSERT_FALSE(collator.getValue()->getSpec().getBackwards()); } TEST(CollatorFactoryICUTest, LanguageDependentDefaultsSetSuccessfully) { @@ -426,7 +426,7 @@ TEST(CollatorFactoryICUTest, LanguageDependentDefaultsSetSuccessfully) { auto collator = factory.makeFromBSON(BSON("locale" << "fr_CA")); ASSERT_OK(collator.getStatus()); - ASSERT_TRUE(*collator.getValue()->getSpec().getBackwards()); + ASSERT_TRUE(collator.getValue()->getSpec().getBackwards()); } TEST(CollatorFactoryICUTest, CaseLevelFalseParsesSuccessfully) { @@ -616,7 +616,7 @@ TEST(CollatorFactoryICUTest, BackwardsFalseParsesSuccessfully) { << "en_US" << "backwards" << false)); ASSERT_OK(collator.getStatus()); - ASSERT_FALSE(*collator.getValue()->getSpec().getBackwards()); + ASSERT_FALSE(collator.getValue()->getSpec().getBackwards()); } TEST(CollatorFactoryICUTest, BackwardsTrueParsesSuccessfully) { @@ -625,7 +625,7 @@ TEST(CollatorFactoryICUTest, BackwardsTrueParsesSuccessfully) { << "en_US" << "backwards" << true)); ASSERT_OK(collator.getStatus()); - ASSERT_TRUE(*collator.getValue()->getSpec().getBackwards()); + ASSERT_TRUE(collator.getValue()->getSpec().getBackwards()); } TEST(CollatorFactoryICUTest, LongStrengthFieldParsesSuccessfully) { diff --git a/src/mongo/idl/basic_types.idl b/src/mongo/idl/basic_types.idl index c82abccf4f7..1bac48374f5 100644 --- a/src/mongo/idl/basic_types.idl +++ b/src/mongo/idl/basic_types.idl @@ -384,10 +384,10 @@ structs: type: bool default: false # Causes accent differences to be considered in reverse order, as it is done in the - # French language. + # French language. Note: the optionalBool parser rejects null/undefined inputs, which + # preserves an invariant in updateCollationSpecFromICUCollator. backwards: - type: bool - optional: true + type: optionalBool # Indicates the version of the collator. It is used to ensure that we do not mix # versions by, for example, constructing an index with one version of ICU and then # attempting to use this index with a server that is built against a newer ICU version. -- cgit v1.2.1