From cab92db33858bf6092deb30a910692c95541e00b Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Tue, 18 Sep 2018 02:14:24 -0700 Subject: [PATCH] ICU-20050 Fixing memory leaks in move and copy assignment in Number*Formatter. --- icu4c/source/i18n/number_fluent.cpp | 17 +++++--- icu4c/source/i18n/numrange_fluent.cpp | 14 ++++-- icu4c/source/i18n/unicode/numberformatter.h | 2 + .../i18n/unicode/numberrangeformatter.h | 2 + icu4c/source/test/intltest/numbertest.h | 1 + icu4c/source/test/intltest/numbertest_api.cpp | 7 +++ .../source/test/intltest/numbertest_range.cpp | 43 +++++++++++++++++++ 7 files changed, 78 insertions(+), 8 deletions(-) diff --git a/icu4c/source/i18n/number_fluent.cpp b/icu4c/source/i18n/number_fluent.cpp index e0e489a3573..a66e3bd0f23 100644 --- a/icu4c/source/i18n/number_fluent.cpp +++ b/icu4c/source/i18n/number_fluent.cpp @@ -407,7 +407,8 @@ LocalizedNumberFormatter::LocalizedNumberFormatter(NFS&& src) U_NOEXCEPT LocalizedNumberFormatter& LocalizedNumberFormatter::operator=(const LNF& other) { NFS::operator=(static_cast&>(other)); - // No additional fields to assign (let call count and compiled formatter reset to defaults) + // Reset to default values. + clear(); return *this; } @@ -419,20 +420,26 @@ LocalizedNumberFormatter& LocalizedNumberFormatter::operator=(LNF&& src) U_NOEXC // Formatter is compiled lnfMoveHelper(static_cast(src)); } else { - // Reset to default values. - auto* callCount = reinterpret_cast(fUnsafeCallCount); - umtx_storeRelease(*callCount, 0); - fCompiled = nullptr; + clear(); } return *this; } +void LocalizedNumberFormatter::clear() { + // Reset to default values. + auto* callCount = reinterpret_cast(fUnsafeCallCount); + umtx_storeRelease(*callCount, 0); + delete fCompiled; + fCompiled = nullptr; +} + void LocalizedNumberFormatter::lnfMoveHelper(LNF&& src) { // Copy over the compiled formatter and set call count to INT32_MIN as in computeCompiled(). // Don't copy the call count directly because doing so requires a loadAcquire/storeRelease. // The bits themselves appear to be platform-dependent, so copying them might not be safe. auto* callCount = reinterpret_cast(fUnsafeCallCount); umtx_storeRelease(*callCount, INT32_MIN); + delete fCompiled; fCompiled = src.fCompiled; // Reset the source object to leave it in a safe state. auto* srcCallCount = reinterpret_cast(src.fUnsafeCallCount); diff --git a/icu4c/source/i18n/numrange_fluent.cpp b/icu4c/source/i18n/numrange_fluent.cpp index aee9518fe09..4a812fd485e 100644 --- a/icu4c/source/i18n/numrange_fluent.cpp +++ b/icu4c/source/i18n/numrange_fluent.cpp @@ -227,18 +227,26 @@ LocalizedNumberRangeFormatter::LocalizedNumberRangeFormatter(LocalizedNumberRang LocalizedNumberRangeFormatter::LocalizedNumberRangeFormatter(NFS&& src) U_NOEXCEPT : NFS(std::move(src)) { - // No additional fields to assign + // Steal the compiled formatter + LNF&& _src = static_cast(src); + fImpl = _src.fImpl; + _src.fImpl = nullptr; } LocalizedNumberRangeFormatter& LocalizedNumberRangeFormatter::operator=(const LNF& other) { NFS::operator=(static_cast&>(other)); - // No additional fields to assign + // Do not steal; just clear + delete fImpl; + fImpl = nullptr; return *this; } LocalizedNumberRangeFormatter& LocalizedNumberRangeFormatter::operator=(LNF&& src) U_NOEXCEPT { NFS::operator=(static_cast&&>(src)); - // No additional fields to assign + // Steal the compiled formatter + delete fImpl; + fImpl = src.fImpl; + src.fImpl = nullptr; return *this; } diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index b75d8e6432f..61ef9840773 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -2358,6 +2358,8 @@ class U_I18N_API LocalizedNumberFormatter LocalizedNumberFormatter(impl::MacroProps &¯os, const Locale &locale); + void clear(); + void lnfMoveHelper(LocalizedNumberFormatter&& src); /** diff --git a/icu4c/source/i18n/unicode/numberrangeformatter.h b/icu4c/source/i18n/unicode/numberrangeformatter.h index 5b9f264f9bf..6e50daae15a 100644 --- a/icu4c/source/i18n/unicode/numberrangeformatter.h +++ b/icu4c/source/i18n/unicode/numberrangeformatter.h @@ -625,6 +625,8 @@ class U_I18N_API LocalizedNumberRangeFormatter LocalizedNumberRangeFormatter(impl::RangeMacroProps &¯os, const Locale &locale); + void clear(); + // To give the fluent setters access to this class's constructor: friend class NumberRangeFormatterSettings; friend class NumberRangeFormatterSettings; diff --git a/icu4c/source/test/intltest/numbertest.h b/icu4c/source/test/intltest/numbertest.h index 0d0f7dec815..49c2d4411a9 100644 --- a/icu4c/source/test/intltest/numbertest.h +++ b/icu4c/source/test/intltest/numbertest.h @@ -257,6 +257,7 @@ class NumberRangeFormatterTest : public IntlTest { void testIdentity(); void testDifferentFormatters(); void testPlurals(); + void testCopyMove(); void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = 0); diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index 0604a9d9db1..ec8da929244 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -2372,8 +2372,15 @@ void NumberFormatterApiTest::copyMove() { assertTrue("[constructor] Source should be reset after move", l1.getCompiled() == nullptr); // Reset l1 and l2 to check for macro-props copying for behavior testing + // Make the test more interesting: also warm them up with a compiled formatter. l1 = NumberFormatter::withLocale("en"); + l1.formatInt(1, status); + l1.formatInt(1, status); + l1.formatInt(1, status); l2 = NumberFormatter::withLocale("en"); + l2.formatInt(1, status); + l2.formatInt(1, status); + l2.formatInt(1, status); // Copy assignment l1 = l3; diff --git a/icu4c/source/test/intltest/numbertest_range.cpp b/icu4c/source/test/intltest/numbertest_range.cpp index 1199f886545..30f033d059c 100644 --- a/icu4c/source/test/intltest/numbertest_range.cpp +++ b/icu4c/source/test/intltest/numbertest_range.cpp @@ -48,6 +48,7 @@ void NumberRangeFormatterTest::runIndexedTest(int32_t index, UBool exec, const c TESTCASE_AUTO(testIdentity); TESTCASE_AUTO(testDifferentFormatters); TESTCASE_AUTO(testPlurals); + TESTCASE_AUTO(testCopyMove); TESTCASE_AUTO_END; } @@ -706,6 +707,48 @@ void NumberRangeFormatterTest::testPlurals() { } } +void NumberRangeFormatterTest::testCopyMove() { + IcuTestErrorCode status(*this, "testCopyMove"); + + // Default constructors + LocalizedNumberRangeFormatter l1; + assertEquals("Initial behavior", u"1–5", l1.formatFormattableRange(1, 5, status).toString(status)); + if (status.errDataIfFailureAndReset()) { return; } + + // Setup + l1 = NumberRangeFormatter::withLocale("fr-FR") + .numberFormatterBoth(NumberFormatter::with().unit(USD)); + assertEquals("Currency behavior", u"1,00–5,00 $US", l1.formatFormattableRange(1, 5, status).toString(status)); + + // Copy constructor + LocalizedNumberRangeFormatter l2 = l1; + assertEquals("Copy constructor", u"1,00–5,00 $US", l2.formatFormattableRange(1, 5, status).toString(status)); + + // Move constructor + LocalizedNumberRangeFormatter l3 = std::move(l1); + assertEquals("Move constructor", u"1,00–5,00 $US", l3.formatFormattableRange(1, 5, status).toString(status)); + + // Reset objects for assignment tests + l1 = NumberRangeFormatter::withLocale("en-us"); + l2 = NumberRangeFormatter::withLocale("en-us"); + assertEquals("Rest behavior, l1", u"1–5", l1.formatFormattableRange(1, 5, status).toString(status)); + assertEquals("Rest behavior, l2", u"1–5", l2.formatFormattableRange(1, 5, status).toString(status)); + + // Copy assignment + l1 = l3; + assertEquals("Copy constructor", u"1,00–5,00 $US", l1.formatFormattableRange(1, 5, status).toString(status)); + + // Move assignment + l2 = std::move(l3); + assertEquals("Copy constructor", u"1,00–5,00 $US", l2.formatFormattableRange(1, 5, status).toString(status)); + + // FormattedNumberRange + FormattedNumberRange result = l1.formatFormattableRange(1, 5, status); + assertEquals("FormattedNumberRange move constructor", u"1,00–5,00 $US", result.toString(status)); + result = l1.formatFormattableRange(3, 6, status); + assertEquals("FormattedNumberRange move assignment", u"3,00–6,00 $US", result.toString(status)); +} + void NumberRangeFormatterTest::assertFormatRange( const char16_t* message, const UnlocalizedNumberRangeFormatter& f,