From 5cbbcef0e1f4274c51252cdf6e79d6667b8a0ddc Mon Sep 17 00:00:00 2001 From: Frank Tang Date: Wed, 2 Oct 2024 16:41:50 -0700 Subject: [PATCH] ICU-22934 Fix typo in PR#3230 ICU-22934 Fix error handling while new return nullptr ICU-22934 Simplfied ICU-22934 more ICU-22934 fix leak --- icu4c/source/common/rbbinode.cpp | 86 ++++++++++++++++++++++++-------- icu4c/source/common/rbbinode.h | 4 +- icu4c/source/common/rbbiscan.cpp | 15 ++++-- icu4c/source/common/rbbisetb.cpp | 12 ++++- icu4c/source/common/rbbitblb.cpp | 26 +++++++--- 5 files changed, 108 insertions(+), 35 deletions(-) diff --git a/icu4c/source/common/rbbinode.cpp b/icu4c/source/common/rbbinode.cpp index fdb015a7a16f..849ee7180a22 100644 --- a/icu4c/source/common/rbbinode.cpp +++ b/icu4c/source/common/rbbinode.cpp @@ -47,7 +47,10 @@ static int gLastSerial = 0; // Constructor. Just set the fields to reasonable default values. // //------------------------------------------------------------------------- -RBBINode::RBBINode(NodeType t) : UMemory() { +RBBINode::RBBINode(NodeType t, UErrorCode& status) : UMemory() { + if (U_FAILURE(status)) { + return; + } #ifdef RBBI_DEBUG fSerialNum = ++gLastSerial; #endif @@ -65,10 +68,13 @@ RBBINode::RBBINode(NodeType t) : UMemory() { fVal = 0; fPrecedence = precZero; - UErrorCode status = U_ZERO_ERROR; - fFirstPosSet = new UVector(status); // TODO - get a real status from somewhere + fFirstPosSet = new UVector(status); fLastPosSet = new UVector(status); fFollowPos = new UVector(status); + if (U_SUCCESS(status) && + (fFirstPosSet == nullptr || fLastPosSet == nullptr || fFollowPos == nullptr)) { + status = U_MEMORY_ALLOCATION_ERROR; + } if (t==opCat) {fPrecedence = precOpCat;} else if (t==opOr) {fPrecedence = precOpOr;} else if (t==opStart) {fPrecedence = precStart;} @@ -77,7 +83,10 @@ RBBINode::RBBINode(NodeType t) : UMemory() { } -RBBINode::RBBINode(const RBBINode &other) : UMemory(other) { +RBBINode::RBBINode(const RBBINode &other, UErrorCode& status) : UMemory(other) { + if (U_FAILURE(status)) { + return; + } #ifdef RBBI_DEBUG fSerialNum = ++gLastSerial; #endif @@ -94,10 +103,13 @@ RBBINode::RBBINode(const RBBINode &other) : UMemory(other) { fVal = other.fVal; fRuleRoot = false; fChainIn = other.fChainIn; - UErrorCode status = U_ZERO_ERROR; fFirstPosSet = new UVector(status); // TODO - get a real status from somewhere fLastPosSet = new UVector(status); fFollowPos = new UVector(status); + if (U_SUCCESS(status) && + (fFirstPosSet == nullptr || fLastPosSet == nullptr || fFollowPos == nullptr)) { + status = U_MEMORY_ALLOCATION_ERROR; + } } @@ -210,24 +222,37 @@ RBBINode *RBBINode::cloneTree(UErrorCode &status, int depth) { // If the current node is a variable reference, skip over it // and clone the definition of the variable instead. n = fLeftChild->cloneTree(status, depth+1); + if (U_FAILURE(status)) { + return nullptr; + } } else if (fType == RBBINode::uset) { n = this; } else { - n = new RBBINode(*this); + n = new RBBINode(*this, status); + if (U_FAILURE(status)) { + delete n; + return nullptr; + } // Check for null pointer. - if (n != nullptr) { - if (fLeftChild != nullptr) { - n->fLeftChild = fLeftChild->cloneTree(status, depth+1); - if (U_SUCCESS(status)) { - n->fLeftChild->fParent = n; - } + if (n == nullptr) { + status = U_MEMORY_ALLOCATION_ERROR; + return nullptr; + } + if (fLeftChild != nullptr) { + n->fLeftChild = fLeftChild->cloneTree(status, depth+1); + if (U_FAILURE(status)) { + delete n; + return nullptr; } - if (fRightChild != nullptr) { - n->fRightChild = fRightChild->cloneTree(status, depth+1); - if (U_SUCCESS(status)) { - n->fRightChild->fParent = n; - } + n->fLeftChild->fParent = n; + } + if (fRightChild != nullptr) { + n->fRightChild = fRightChild->cloneTree(status, depth+1); + if (U_FAILURE(status)) { + delete n; + return nullptr; } + n->fRightChild->fParent = n; } } return n; @@ -265,20 +290,33 @@ RBBINode *RBBINode::flattenVariables(UErrorCode& status, int depth) { } if (fType == varRef) { RBBINode *retNode = fLeftChild->cloneTree(status, depth+1); - if (retNode != nullptr) { - retNode->fRuleRoot = this->fRuleRoot; - retNode->fChainIn = this->fChainIn; + if (U_FAILURE(status)) { + return this; } + retNode->fRuleRoot = this->fRuleRoot; + retNode->fChainIn = this->fChainIn; delete this; // TODO: undefined behavior. Fix. return retNode; } if (fLeftChild != nullptr) { fLeftChild = fLeftChild->flattenVariables(status, depth+1); + if (fLeftChild == nullptr) { + status = U_MEMORY_ALLOCATION_ERROR; + } + if (U_FAILURE(status)) { + return this; + } fLeftChild->fParent = this; } if (fRightChild != nullptr) { fRightChild = fRightChild->flattenVariables(status, depth+1); + if (fRightChild == nullptr) { + status = U_MEMORY_ALLOCATION_ERROR; + } + if (U_FAILURE(status)) { + return this; + } fRightChild->fParent = this; } return this; @@ -312,8 +350,10 @@ void RBBINode::flattenSets(UErrorCode &status, int depth) { RBBINode *replTree = usetNode->fLeftChild; fLeftChild = replTree->cloneTree(status, depth+1); if (U_FAILURE(status)) { - fLeftChild->fParent = this; + delete setRefNode; + return; } + fLeftChild->fParent = this; delete setRefNode; } else { fLeftChild->flattenSets(status, depth+1); @@ -327,8 +367,10 @@ void RBBINode::flattenSets(UErrorCode &status, int depth) { RBBINode *replTree = usetNode->fLeftChild; fRightChild = replTree->cloneTree(status, depth+1); if (U_FAILURE(status)) { - fRightChild->fParent = this; + delete setRefNode; + return; } + fRightChild->fParent = this; delete setRefNode; } else { fRightChild->flattenSets(status, depth+1); diff --git a/icu4c/source/common/rbbinode.h b/icu4c/source/common/rbbinode.h index a854177348f4..8fbc9d1b588e 100644 --- a/icu4c/source/common/rbbinode.h +++ b/icu4c/source/common/rbbinode.h @@ -91,8 +91,8 @@ class RBBINode : public UMemory { UVector *fFollowPos; - RBBINode(NodeType t); - RBBINode(const RBBINode &other); + RBBINode(NodeType t, UErrorCode& status); + RBBINode(const RBBINode &other, UErrorCode& status); ~RBBINode(); static void NRDeleteNode(RBBINode *node); diff --git a/icu4c/source/common/rbbiscan.cpp b/icu4c/source/common/rbbiscan.cpp index cf2d63cd807b..77fc3bcd9b7e 100644 --- a/icu4c/source/common/rbbiscan.cpp +++ b/icu4c/source/common/rbbiscan.cpp @@ -767,15 +767,24 @@ void RBBIRuleScanner::findSetFor(const UnicodeString &s, RBBINode *node, Unicode c = s.char32At(0); setToAdopt = new UnicodeSet(c, c); } + if (setToAdopt == nullptr) { + error(U_MEMORY_ALLOCATION_ERROR); + return; + } } // // Make a new uset node to refer to this UnicodeSet // This new uset node becomes the child of the caller's setReference node. // - RBBINode *usetNode = new RBBINode(RBBINode::uset); + UErrorCode localStatus = U_ZERO_ERROR; + RBBINode *usetNode = new RBBINode(RBBINode::uset, localStatus); if (usetNode == nullptr) { - error(U_MEMORY_ALLOCATION_ERROR); + localStatus = U_MEMORY_ALLOCATION_ERROR; + } + if (U_FAILURE(localStatus)) { + delete usetNode; + error(localStatus); delete setToAdopt; return; } @@ -1191,7 +1200,7 @@ RBBINode *RBBIRuleScanner::pushNewNode(RBBINode::NodeType t) { return nullptr; } fNodeStackPtr++; - fNodeStack[fNodeStackPtr] = new RBBINode(t); + fNodeStack[fNodeStackPtr] = new RBBINode(t, *fRB->fStatus); if (fNodeStack[fNodeStackPtr] == nullptr) { *fRB->fStatus = U_MEMORY_ALLOCATION_ERROR; } diff --git a/icu4c/source/common/rbbisetb.cpp b/icu4c/source/common/rbbisetb.cpp index 6c22cf470f8b..df94fc8bc4fb 100644 --- a/icu4c/source/common/rbbisetb.cpp +++ b/icu4c/source/common/rbbisetb.cpp @@ -375,7 +375,11 @@ void RBBISetBuilder::addValToSets(UVector *sets, uint32_t val) { } void RBBISetBuilder::addValToSet(RBBINode *usetNode, uint32_t val) { - RBBINode *leafNode = new RBBINode(RBBINode::leafChar); + RBBINode *leafNode = new RBBINode(RBBINode::leafChar, *fStatus); + if (U_FAILURE(*fStatus)) { + delete leafNode; + return; + } if (leafNode == nullptr) { *fStatus = U_MEMORY_ALLOCATION_ERROR; return; @@ -388,9 +392,13 @@ void RBBISetBuilder::addValToSet(RBBINode *usetNode, uint32_t val) { // There are already input symbols present for this set. // Set up an OR node, with the previous stuff as the left child // and the new value as the right child. - RBBINode *orNode = new RBBINode(RBBINode::opOr); + RBBINode *orNode = new RBBINode(RBBINode::opOr, *fStatus); if (orNode == nullptr) { *fStatus = U_MEMORY_ALLOCATION_ERROR; + } + if (U_FAILURE(*fStatus)) { + delete orNode; + delete leafNode; return; } orNode->fLeftChild = usetNode->fLeftChild; diff --git a/icu4c/source/common/rbbitblb.cpp b/icu4c/source/common/rbbitblb.cpp index 4cb743240ba0..b89909807c20 100644 --- a/icu4c/source/common/rbbitblb.cpp +++ b/icu4c/source/common/rbbitblb.cpp @@ -99,13 +99,22 @@ void RBBITableBuilder::buildForwardTable() { // {bof} fake character. // if (fRB->fSetBuilder->sawBOF()) { - RBBINode *bofTop = new RBBINode(RBBINode::opCat); - RBBINode *bofLeaf = new RBBINode(RBBINode::leafChar); - // Delete and exit if memory allocation failed. - if (bofTop == nullptr || bofLeaf == nullptr) { + RBBINode *bofTop = new RBBINode(RBBINode::opCat, *fStatus); + if (bofTop == nullptr) { *fStatus = U_MEMORY_ALLOCATION_ERROR; + } + if (U_FAILURE(*fStatus)) { delete bofTop; + return; + } + RBBINode *bofLeaf = new RBBINode(RBBINode::leafChar, *fStatus); + // Delete and exit if memory allocation failed. + if (bofLeaf == nullptr) { + *fStatus = U_MEMORY_ALLOCATION_ERROR; + } + if (U_FAILURE(*fStatus)) { delete bofLeaf; + delete bofTop; return; } bofTop->fLeftChild = bofLeaf; @@ -120,18 +129,23 @@ void RBBITableBuilder::buildForwardTable() { // Appears as a cat-node, left child being the original tree, // right child being the end marker. // - RBBINode *cn = new RBBINode(RBBINode::opCat); + RBBINode *cn = new RBBINode(RBBINode::opCat, *fStatus); // Exit if memory allocation failed. if (cn == nullptr) { *fStatus = U_MEMORY_ALLOCATION_ERROR; + } + if (U_FAILURE(*fStatus)) { + delete cn; return; } cn->fLeftChild = fTree; fTree->fParent = cn; - RBBINode *endMarkerNode = cn->fRightChild = new RBBINode(RBBINode::endMark); + RBBINode *endMarkerNode = cn->fRightChild = new RBBINode(RBBINode::endMark, *fStatus); // Delete and exit if memory allocation failed. if (cn->fRightChild == nullptr) { *fStatus = U_MEMORY_ALLOCATION_ERROR; + } + if (U_FAILURE(*fStatus)) { delete cn; return; }