Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing PR to check rebase against latest on maint/maint-75 for: Memory error handling #64

Open
wants to merge 6 commits into
base: maint/maint-75
Choose a base branch
from
+160 −94
Open

Testing PR to check rebase against latest on maint/maint-75 for: Memory error handling #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba13db1
ICU-22261 Refactor MF2 attributes and options parsing code
catamorphism Mar 29, 2024
5c0c9d5
Make OptionMap::Builder exported
catamorphism Mar 29, 2024
558bc69
Update icu4c/source/i18n/messageformat2_parser.h
catamorphism Apr 3, 2024
2497b11
Increase timeout for MSVC Debug CI jobs
catamorphism Apr 5, 2024
51f7e54
ICU-22261 Refactor error handling for internal MF2 array-allocating f…
catamorphism Apr 9, 2024
30e1c4f
Merge branch 'maint/maint-75' into memory-error-handling
echeran Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .ci-builds/.azure-pipelines-icu4c.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ jobs:
#-------------------------------------------------------------------------
- job: ICU4C_MSVC_x64_Debug
displayName: 'C: MSVC x64 Debug (VS 2022)'
timeoutInMinutes: 35
timeoutInMinutes: 60
pool:
vmImage: 'windows-2022'
demands:
Expand Down Expand Up @@ -487,7 +487,7 @@ jobs:
#-------------------------------------------------------------------------
- job: ICU4C_MSVC_x86_Debug_VS2019
displayName: 'C: MSVC 32-bit Debug (VS 2019)'
timeoutInMinutes: 60
timeoutInMinutes: 90
pool:
vmImage: 'windows-2019'
demands:
Expand Down
36 changes: 19 additions & 17 deletions icu4c/source/i18n/messageformat2_allocation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,15 @@ namespace message2 {
// Helpers

template<typename T>
static T* copyArray(const T* source, int32_t& len) { // `len` is an in/out param
if (source == nullptr) {
len = 0;
static T* copyArray(const T* source, int32_t len, UErrorCode& status) {
if (U_FAILURE(status)) {
return nullptr;
}
U_ASSERT(source != nullptr);
U_ASSERT(len >= 0);
T* dest = new T[len];
if (dest == nullptr) {
// Set length to 0 to prevent the
// array from being accessed
len = 0;
status = U_MEMORY_ALLOCATION_ERROR;
} else {
for (int32_t i = 0; i < len; i++) {
dest[i] = source[i];
Expand All @@ -43,13 +42,14 @@ namespace message2 {
}

template<typename T>
static T* copyVectorToArray(const UVector& source, int32_t& len) {
len = source.size();
static T* copyVectorToArray(const UVector& source, UErrorCode& status) {
if (U_FAILURE(status)) {
return nullptr;
}
int32_t len = source.size();
T* dest = new T[len];
if (dest == nullptr) {
// Set length to 0 to prevent the
// array from being accessed
len = 0;
status = U_MEMORY_ALLOCATION_ERROR;
} else {
for (int32_t i = 0; i < len; i++) {
dest[i] = *(static_cast<T*>(source.elementAt(i)));
Expand All @@ -59,19 +59,21 @@ namespace message2 {
}

template<typename T>
static T* moveVectorToArray(UVector& source, int32_t& len) {
len = source.size();
static T* moveVectorToArray(UVector& source, UErrorCode& status) {
if (U_FAILURE(status)) {
return nullptr;
}

int32_t len = source.size();
T* dest = new T[len];
if (dest == nullptr) {
// Set length to 0 to prevent the
// array from being accessed
len = 0;
status = U_MEMORY_ALLOCATION_ERROR;
} else {
for (int32_t i = 0; i < len; i++) {
dest[i] = std::move(*static_cast<T*>(source.elementAt(i)));
}
source.removeAllElements();
}
source.removeAllElements();
return dest;
}

Expand Down
160 changes: 104 additions & 56 deletions icu4c/source/i18n/messageformat2_data_model.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,10 @@ SelectorKeys::Builder::~Builder() {
}

SelectorKeys::SelectorKeys(const UVector& ks, UErrorCode& status) : len(ks.size()) {
Key* result = copyVectorToArray<Key>(ks, status);
if (U_FAILURE(status)) {
return;
}
Key* result = copyVectorToArray<Key>(ks, len);
if (result == nullptr) {
status = U_MEMORY_ALLOCATION_ERROR;
len = 0;
return;
}
keys.adoptInstead(result);
}

Expand All @@ -95,7 +90,13 @@ SelectorKeys& SelectorKeys::operator=(SelectorKeys other) noexcept {
}

SelectorKeys::SelectorKeys(const SelectorKeys& other) : len(other.len) {
keys.adoptInstead(copyArray(other.keys.getAlias(), len));
UErrorCode localErrorCode = U_ZERO_ERROR;
if (len != 0) {
keys.adoptInstead(copyArray(other.keys.getAlias(), len, localErrorCode));
}
if (U_FAILURE(localErrorCode)) {
len = 0;
}
}

SelectorKeys::~SelectorKeys() {
Expand Down Expand Up @@ -198,9 +199,18 @@ Key::~Key() {}
// ------------ Reserved

// Copy constructor
Reserved::Reserved(const Reserved& other) {
len = other.len;
parts.adoptInstead(copyArray(other.parts.getAlias(), len));
Reserved::Reserved(const Reserved& other) : len(other.len) {
U_ASSERT(!other.bogus);

UErrorCode localErrorCode = U_ZERO_ERROR;
if (len == 0) {
parts.adoptInstead(nullptr);
} else {
parts.adoptInstead(copyArray(other.parts.getAlias(), len, localErrorCode));
}
if (U_FAILURE(localErrorCode)) {
bogus = true;
}
}

Reserved& Reserved::operator=(Reserved other) noexcept {
Expand All @@ -212,14 +222,16 @@ Reserved::Reserved(const UVector& ps, UErrorCode& status) noexcept : len(ps.size
if (U_FAILURE(status)) {
return;
}
parts = LocalArray<Literal>(copyVectorToArray<Literal>(ps, len));
parts = LocalArray<Literal>(copyVectorToArray<Literal>(ps, status));
}

int32_t Reserved::numParts() const {
U_ASSERT(!bogus);
return len;
}

const Literal& Reserved::getPart(int32_t i) const {
U_ASSERT(!bogus);
U_ASSERT(i < numParts());
return parts[i];
}
Expand Down Expand Up @@ -257,14 +269,10 @@ Reserved::~Reserved() {

//------------------------ Operator

OptionMap::OptionMap(const UVector& opts, UErrorCode& status) {
CHECK_ERROR(status);

len = opts.size();
Option* result = copyVectorToArray<Option>(opts, len);
if (result == nullptr && len != 0) {
OptionMap::OptionMap(const UVector& opts, UErrorCode& status) : len(opts.size()) {
Option* result = copyVectorToArray<Option>(opts, status);
if (U_FAILURE(status)) {
bogus = true;
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
options.adoptInstead(result);
Expand All @@ -273,8 +281,9 @@ OptionMap::OptionMap(const UVector& opts, UErrorCode& status) {

OptionMap::OptionMap(const OptionMap& other) : len(other.len) {
U_ASSERT(!other.bogus);
Option* result = copyArray(other.options.getAlias(), len);
if (result == nullptr && len != 0) {
UErrorCode localErrorCode = U_ZERO_ERROR;
Option* result = copyArray(other.options.getAlias(), len, localErrorCode);
if (U_FAILURE(localErrorCode)) {
bogus = true;
return;
}
Expand Down Expand Up @@ -653,16 +662,13 @@ const Reserved* UnsupportedStatement::getBody(UErrorCode& errorCode) const {
UnsupportedStatement::UnsupportedStatement(const UnicodeString& k,
const std::optional<Reserved>& r,
const UVector& es,
UErrorCode& status) : keyword(k), body(r) {
UErrorCode& status)
: keyword(k), body(r), expressionsLen(es.size()) {
CHECK_ERROR(status);

U_ASSERT(es.size() >= 1);
Expression* result = copyVectorToArray<Expression>(es, expressionsLen);
if (result == nullptr) {
status = U_MEMORY_ALLOCATION_ERROR;
expressionsLen = 0;
return;
}
U_ASSERT(expressionsLen >= 1);
Expression* result = copyVectorToArray<Expression>(es, status);
CHECK_ERROR(status);
expressions.adoptInstead(result);
}

Expand All @@ -671,7 +677,11 @@ UnsupportedStatement::UnsupportedStatement(const UnsupportedStatement& other) {
body = other.body;
expressionsLen = other.expressionsLen;
U_ASSERT(expressionsLen > 0);
expressions.adoptInstead(copyArray(other.expressions.getAlias(), expressionsLen));
UErrorCode localErrorCode = U_ZERO_ERROR;
expressions.adoptInstead(copyArray(other.expressions.getAlias(), expressionsLen, localErrorCode));
if (U_FAILURE(localErrorCode)) {
expressionsLen = 0;
}
}

UnsupportedStatement& UnsupportedStatement::operator=(UnsupportedStatement other) noexcept {
Expand Down Expand Up @@ -722,21 +732,32 @@ Pattern::Pattern(const UVector& ps, UErrorCode& status) : len(ps.size()) {
if (U_FAILURE(status)) {
return;
}
PatternPart* result = copyVectorToArray<PatternPart>(ps, len);
if (result == nullptr) {
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
PatternPart* result = copyVectorToArray<PatternPart>(ps, status);
CHECK_ERROR(status);
parts.adoptInstead(result);
}

// Copy constructor
Pattern::Pattern(const Pattern& other) noexcept : len(other.len) {
parts.adoptInstead(copyArray(other.parts.getAlias(), len));
Pattern::Pattern(const Pattern& other) : len(other.len) {
U_ASSERT(!other.bogus);
UErrorCode localErrorCode = U_ZERO_ERROR;
if (len == 0) {
parts.adoptInstead(nullptr);
} else {
parts.adoptInstead(copyArray(other.parts.getAlias(), len, localErrorCode));
}
if (U_FAILURE(localErrorCode)) {
bogus = true;
}
}

int32_t Pattern::numParts() const {
U_ASSERT(!bogus);
return len;
}

const PatternPart& Pattern::getPart(int32_t i) const {
U_ASSERT(i < numParts());
U_ASSERT(!bogus && i < numParts());
return parts[i];
}

Expand Down Expand Up @@ -875,12 +896,26 @@ Matcher& Matcher::operator=(Matcher other) {
}

Matcher::Matcher(const Matcher& other) {
U_ASSERT(!other.bogus);
numSelectors = other.numSelectors;
numVariants = other.numVariants;
selectors.adoptInstead(copyArray<Expression>(other.selectors.getAlias(), numSelectors));
variants.adoptInstead(copyArray<Variant>(other.variants.getAlias(), numVariants));
UErrorCode localErrorCode = U_ZERO_ERROR;
selectors.adoptInstead(copyArray<Expression>(other.selectors.getAlias(),
numSelectors,
localErrorCode));
variants.adoptInstead(copyArray<Variant>(other.variants.getAlias(),
numVariants,
localErrorCode));
if (U_FAILURE(localErrorCode)) {
bogus = true;
}
}

Matcher::Matcher(Expression* ss, int32_t ns, Variant* vs, int32_t nv)
: selectors(ss), numSelectors(ns), variants(vs), numVariants(nv) {}

Matcher::~Matcher() {}

// --------------- MFDataModel

const Pattern& MFDataModel::getPattern() const {
Expand Down Expand Up @@ -1014,29 +1049,34 @@ MFDataModel::Builder& MFDataModel::Builder::setPattern(Pattern&& pat) {
MFDataModel::MFDataModel(const MFDataModel& other) : body(Pattern()) {
U_ASSERT(!other.bogus);

UErrorCode localErrorCode = U_ZERO_ERROR;

if (other.hasPattern()) {
body.emplace<Pattern>(Pattern(*std::get_if<Pattern>(&other.body)));
// body.emplace<Pattern>(Pattern(*std::get_if<Pattern>(&other.body)));
body = *std::get_if<Pattern>(&other.body);
} else {
const Expression* otherSelectors = other.getSelectorsInternal();
const Variant* otherVariants = other.getVariantsInternal();
int32_t numSelectors = other.numSelectors();
int32_t numVariants = other.numVariants();
Expression* copiedSelectors = copyArray(otherSelectors, numSelectors);
Variant* copiedVariants = copyArray(otherVariants, numVariants);
if (!(copiedSelectors != nullptr && copiedVariants != nullptr)) {
Expression* copiedSelectors = copyArray(otherSelectors, numSelectors, localErrorCode);
Variant* copiedVariants = copyArray(otherVariants, numVariants, localErrorCode);
if (U_FAILURE(localErrorCode)) {
bogus = true;
return;
}
body.emplace<Matcher>(Matcher(copiedSelectors, numSelectors, copiedVariants, numVariants));
// body.emplace<Matcher>(Matcher(copiedSelectors, numSelectors, copiedVariants, numVariants));
body = Matcher(copiedSelectors, numSelectors, copiedVariants, numVariants);
}

bindingsLen = other.bindingsLen;
bindings.adoptInstead(copyArray(other.bindings.getAlias(), bindingsLen));
if (!bindings.isValid()) {
bindings.adoptInstead(copyArray(other.bindings.getAlias(), bindingsLen, localErrorCode));
if (U_FAILURE(localErrorCode)) {
bogus = true;
}
unsupportedStatementsLen = other.unsupportedStatementsLen;
unsupportedStatements.adoptInstead(copyArray(other.unsupportedStatements.getAlias(), unsupportedStatementsLen));
if (!unsupportedStatements.isValid()) {
unsupportedStatements.adoptInstead(copyArray(other.unsupportedStatements.getAlias(), unsupportedStatementsLen, localErrorCode));
if (U_FAILURE(localErrorCode)) {
bogus = true;
}
}
Expand All @@ -1047,25 +1087,33 @@ MFDataModel::MFDataModel(const MFDataModel::Builder& builder, UErrorCode& errorC
if (builder.hasPattern) {
body.emplace<Pattern>(builder.pattern);
} else {
int32_t numVariants = builder.variants == nullptr ? 0 : builder.variants->size();
int32_t numSelectors = builder.selectors == nullptr ? 0 : builder.selectors->size();
Variant* variants = copyVectorToArray<Variant>(*builder.variants, numVariants);
Expression* selectors = copyVectorToArray<Expression>(*builder.selectors, numSelectors);
bogus &= (variants != nullptr && selectors != nullptr);
U_ASSERT(builder.variants != nullptr);
U_ASSERT(builder.selectors != nullptr);
int32_t numVariants = builder.variants->size();
int32_t numSelectors = builder.selectors->size();
Variant* variants = copyVectorToArray<Variant>(*builder.variants, errorCode);
Expression* selectors = copyVectorToArray<Expression>(*builder.selectors, errorCode);
if (U_FAILURE(errorCode)) {
bogus = true;
return;
}
body.emplace<Matcher>(Matcher(selectors, numSelectors, variants, numVariants));
}

U_ASSERT(builder.bindings != nullptr);
bindingsLen = builder.bindings->size();
bindings.adoptInstead(copyVectorToArray<Binding>(*builder.bindings, bindingsLen));
bindings.adoptInstead(copyVectorToArray<Binding>(*builder.bindings, errorCode));
unsupportedStatementsLen = builder.unsupportedStatements->size();
unsupportedStatements.adoptInstead(copyVectorToArray<UnsupportedStatement>(*builder.unsupportedStatements, unsupportedStatementsLen));
bogus &= ((bool) (bindings.isValid() && unsupportedStatements.isValid()));
unsupportedStatements.adoptInstead(copyVectorToArray<UnsupportedStatement>(*builder.unsupportedStatements, errorCode));
if (U_FAILURE(errorCode)) {
bogus = true;
}
}

MFDataModel::MFDataModel() : body(Pattern()) {}

MFDataModel& MFDataModel::operator=(MFDataModel other) noexcept {
U_ASSERT(!other.bogus);
swap(*this, other);
return *this;
}
Expand Down
6 changes: 2 additions & 4 deletions icu4c/source/i18n/messageformat2_evaluation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,8 @@ const ResolvedFunctionOption* FunctionOptions::getResolvedFunctionOptions(int32_
FunctionOptions::FunctionOptions(UVector&& optionsVector, UErrorCode& status) {
CHECK_ERROR(status);

options = moveVectorToArray<ResolvedFunctionOption>(optionsVector, functionOptionsLen);
if (options == nullptr) {
status = U_MEMORY_ALLOCATION_ERROR;
}
functionOptionsLen = optionsVector.size();
options = moveVectorToArray<ResolvedFunctionOption>(optionsVector, status);
}

UBool FunctionOptions::getFunctionOption(const UnicodeString& key, Formattable& option) const {
Expand Down
Loading