Skip to content

Commit

Permalink
ICU-21383 Fix memory problem in FormattedStringBuilder
Browse files Browse the repository at this point in the history
  • Loading branch information
@sffc
sffc committed Nov 11, 2020
1 parent 9169326 commit e7f6673
Show file tree
Hide file tree
Showing 9 changed files with 182 additions and 14 deletions.
4 changes: 2 additions & 2 deletions icu4c/source/i18n/formattedval_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,8 @@ class U_I18N_API FormattedValueStringBuilderImpl : public UMemory, public Format
* spanValue: the index of the list item, for example.
* length: the length of the span, used to split adjacent fields.
*/
void appendSpanInfo(int32_t spanValue, int32_t length);
void prependSpanInfo(int32_t spanValue, int32_t length);
void appendSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status);
void prependSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status);

private:
FormattedStringBuilder fString;
Expand Down
28 changes: 22 additions & 6 deletions icu4c/source/i18n/formattedval_sbimpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,19 +219,35 @@ bool FormattedValueStringBuilderImpl::nextPositionImpl(ConstrainedFieldPosition&
}

U_ASSERT(currField == kUndefinedField);
// Always set the position to the end so that we don't revisit previous sections
cfpos.setState(
cfpos.getCategory(),
cfpos.getField(),
fString.fLength,
fString.fLength);
return false;
}

void FormattedValueStringBuilderImpl::appendSpanInfo(int32_t spanValue, int32_t length) {
if (spanIndices.getCapacity() <= spanValue) {
spanIndices.resize(spanValue * 2);
void FormattedValueStringBuilderImpl::appendSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status) {
if (U_FAILURE(status)) { return; }
U_ASSERT(spanIndices.getCapacity() >= spanValue);
if (spanIndices.getCapacity() == spanValue) {
if (!spanIndices.resize(spanValue * 2, spanValue)) {
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
}
spanIndices[spanValue] = {spanValue, length};
}

void FormattedValueStringBuilderImpl::prependSpanInfo(int32_t spanValue, int32_t length) {
if (spanIndices.getCapacity() <= spanValue) {
spanIndices.resize(spanValue * 2);
void FormattedValueStringBuilderImpl::prependSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status) {
if (U_FAILURE(status)) { return; }
U_ASSERT(spanIndices.getCapacity() >= spanValue);
if (spanIndices.getCapacity() == spanValue) {
if (!spanIndices.resize(spanValue * 2, spanValue)) {
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
}
for (int32_t i = spanValue - 1; i >= 0; i--) {
spanIndices[i+1] = spanIndices[i];
Expand Down
6 changes: 3 additions & 3 deletions icu4c/source/i18n/listformatter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ class FormattedListBuilder {
start,
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
status);
data->appendSpanInfo(0, start.length());
data->appendSpanInfo(0, start.length(), status);
}
}

Expand Down Expand Up @@ -603,7 +603,7 @@ class FormattedListBuilder {
next,
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
status);
data->appendSpanInfo(position, next.length());
data->appendSpanInfo(position, next.length(), status);
data->getStringRef().append(
temp.tempSubString(offsets[1]),
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD},
Expand All @@ -622,7 +622,7 @@ class FormattedListBuilder {
next,
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
status);
data->prependSpanInfo(position, next.length());
data->prependSpanInfo(position, next.length(), status);
data->getStringRef().insert(
0,
temp.tempSubStringBetween(0, offsets[1]),
Expand Down
6 changes: 6 additions & 0 deletions icu4c/source/test/intltest/formattedvaluetest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
}
UBool afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"A after loop: " + CFPosToUnicodeString(cfpos), afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"A after loop again: " + CFPosToUnicodeString(cfpos), afterLoopResult);

// Check nextPosition constrained over each category one at a time
for (int32_t category=0; category<UFIELD_CATEGORY_COUNT+1; category++) {
Expand Down Expand Up @@ -266,6 +268,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
}
UBool afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"B after loop @ " + CFPosToUnicodeString(cfpos), afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"B after loop again @ " + CFPosToUnicodeString(cfpos), afterLoopResult);
}

// Check nextPosition constrained over each field one at a time
Expand Down Expand Up @@ -300,6 +304,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
}
UBool afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"C after loop: " + CFPosToUnicodeString(cfpos), afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"C after loop again: " + CFPosToUnicodeString(cfpos), afterLoopResult);
}
}

Expand Down
54 changes: 54 additions & 0 deletions icu4c/source/test/intltest/listformattertest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,60 @@ void ListFormatterTest::TestFormattedValue() {
expectedFieldPositions,
UPRV_LENGTHOF(expectedFieldPositions));
}

{
LocalPointer<ListFormatter> fmt(ListFormatter::createInstance("en", ULISTFMT_TYPE_UNITS, ULISTFMT_WIDTH_SHORT, status));
if (status.errIfFailureAndReset()) { return; }
const char16_t* message = u"ICU-21383 Long list";
const char16_t* expectedString = u"a, b, c, d, e, f, g, h, i";
const UnicodeString inputs[] = {
u"a",
u"b",
u"c",
u"d",
u"e",
u"f",
u"g",
u"h",
u"i",
};
FormattedList result = fmt->formatStringsToValue(inputs, UPRV_LENGTHOF(inputs), status);
static const UFieldPositionWithCategory expectedFieldPositions[] = {
// field, begin index, end index
{UFIELD_CATEGORY_LIST_SPAN, 0, 0, 1},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 0, 1},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 1, 3},
{UFIELD_CATEGORY_LIST_SPAN, 1, 3, 4},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 3, 4},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 4, 6},
{UFIELD_CATEGORY_LIST_SPAN, 2, 6, 7},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 6, 7},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 7, 9},
{UFIELD_CATEGORY_LIST_SPAN, 3, 9, 10},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 9, 10},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 10, 12},
{UFIELD_CATEGORY_LIST_SPAN, 4, 12, 13},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 12, 13},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 13, 15},
{UFIELD_CATEGORY_LIST_SPAN, 5, 15, 16},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 15, 16},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 16, 18},
{UFIELD_CATEGORY_LIST_SPAN, 6, 18, 19},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 18, 19},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 19, 21},
{UFIELD_CATEGORY_LIST_SPAN, 7, 21, 22},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 21, 22},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 22, 24},
{UFIELD_CATEGORY_LIST_SPAN, 8, 24, 25},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 24, 25},
};
checkMixedFormattedValue(
message,
result,
expectedString,
expectedFieldPositions,
UPRV_LENGTHOF(expectedFieldPositions));
}
}

void ListFormatterTest::DoTheRealListStyleTesting(Locale locale,
Expand Down
6 changes: 6 additions & 0 deletions icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,12 @@ else if (cfpos.matchesField((Field) _field, null)) {
}

assert currField == null;
// Always set the position to the end so that we don't revisit previous sections
cfpos.setState(
cfpos.getField(),
cfpos.getFieldValue(),
self.length,
self.length);
return false;
}

Expand Down
4 changes: 3 additions & 1 deletion icu4j/main/classes/core/src/com/ibm/icu/text/ConstrainedFieldPosition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,9 @@ public void setInt64IterationContext(long context) {
*/
public void setState(Field field, Object value, int start, int limit) {
// Check matchesField only as an assertion (debug build)
assert matchesField(field, value);
if (field != null) {
assert matchesField(field, value);
}

fField = field;
fValue = value;
Expand Down
6 changes: 6 additions & 0 deletions icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ public static void checkFormattedValue(String message, FormattedValue fv, String
}
boolean afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);

// Check nextPosition constrained over each class one at a time
for (Class<?> classConstraint : uniqueFieldClasses) {
Expand All @@ -238,6 +240,8 @@ public static void checkFormattedValue(String message, FormattedValue fv, String
}
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
}

// Check nextPosition constrained over an unrelated class
Expand Down Expand Up @@ -267,6 +271,8 @@ public static void checkFormattedValue(String message, FormattedValue fv, String
}
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
}
}

Expand Down
82 changes: 80 additions & 2 deletions icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,8 @@ private boolean isDefaultLocaleEnglishLike() {

@Test
public void TestFormattedValue() {
ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH);

{
ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH);
String message = "Field position test 1";
String expectedString = "hello, wonderful, and world";
String[] inputs = {
Expand All @@ -244,6 +243,85 @@ public void TestFormattedValue() {
expectedString,
expectedFieldPositions);
}

{
ListFormatter fmt = ListFormatter.getInstance(ULocale.CHINESE, Type.UNITS, Width.SHORT);
String message = "Field position test 2 (ICU-21340)";
String expectedString = "aabbbbbbbccc";
String inputs[] = {
"aa",
"bbbbbbb",
"ccc"
};
FormattedList result = fmt.formatToValue(Arrays.asList(inputs));
Object[][] expectedFieldPositions = {
// field, begin index, end index
{ListFormatter.SpanField.LIST_SPAN, 0, 2, 0},
{ListFormatter.Field.ELEMENT, 0, 2},
{ListFormatter.SpanField.LIST_SPAN, 2, 9, 1},
{ListFormatter.Field.ELEMENT, 2, 9},
{ListFormatter.SpanField.LIST_SPAN, 9, 12, 2},
{ListFormatter.Field.ELEMENT, 9, 12}};
if (!logKnownIssue("21351", "Java still coalesces adjacent elements")) {
FormattedValueTest.checkFormattedValue(
message,
result,
expectedString,
expectedFieldPositions);
}
}

{
ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH, Type.UNITS, Width.SHORT);
String message = "ICU-21383 Long list";
String expectedString = "a, b, c, d, e, f, g, h, i";
String inputs[] = {
"a",
"b",
"c",
"d",
"e",
"f",
"g",
"h",
"i",
};
FormattedList result = fmt.formatToValue(Arrays.asList(inputs));
Object[][] expectedFieldPositions = {
// field, begin index, end index
{ListFormatter.SpanField.LIST_SPAN, 0, 1, 0},
{ListFormatter.Field.ELEMENT, 0, 1},
{ListFormatter.Field.LITERAL, 1, 3},
{ListFormatter.SpanField.LIST_SPAN, 3, 4, 1},
{ListFormatter.Field.ELEMENT, 3, 4},
{ListFormatter.Field.LITERAL, 4, 6},
{ListFormatter.SpanField.LIST_SPAN, 6, 7, 2},
{ListFormatter.Field.ELEMENT, 6, 7},
{ListFormatter.Field.LITERAL, 7, 9},
{ListFormatter.SpanField.LIST_SPAN, 9, 10, 3},
{ListFormatter.Field.ELEMENT, 9, 10},
{ListFormatter.Field.LITERAL, 10, 12},
{ListFormatter.SpanField.LIST_SPAN, 12, 13, 4},
{ListFormatter.Field.ELEMENT, 12, 13},
{ListFormatter.Field.LITERAL, 13, 15},
{ListFormatter.SpanField.LIST_SPAN, 15, 16, 5},
{ListFormatter.Field.ELEMENT, 15, 16},
{ListFormatter.Field.LITERAL, 16, 18},
{ListFormatter.SpanField.LIST_SPAN, 18, 19, 6},
{ListFormatter.Field.ELEMENT, 18, 19},
{ListFormatter.Field.LITERAL, 19, 21},
{ListFormatter.SpanField.LIST_SPAN, 21, 22, 7},
{ListFormatter.Field.ELEMENT, 21, 22},
{ListFormatter.Field.LITERAL, 22, 24},
{ListFormatter.SpanField.LIST_SPAN, 24, 25, 8},
{ListFormatter.Field.ELEMENT, 24, 25},
};
FormattedValueTest.checkFormattedValue(
message,
result,
expectedString,
expectedFieldPositions);
}
}

@Test
Expand Down

0 comments on commit e7f6673

Please sign in to comment.