ICU-22934 clean up error handling in PR#3230

[FrankYFTang]

Checklist

• [X] Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22934
• [X] Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
• [X] Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
• [X] Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
• [X] Issue accepted (done by Technical Committee after discussion)
• [ ] Tests included, if applicable
• [ ] API docs and/or User Guide docs changed or added, if applicable

[aheninger]

Please add a unit test that triggers the recursion depth failures. It's not immediately obvious that there won't be memory leaks or other problems when they occur.

In functions with a newly added UErrorCode in PR #3230, double check the error handling. RBBINode::cloneTree(), for instance,

• at line 216, set a U_MEMORY_ALLOCATION_ERROR and return immediately if the new fails.
• make sure that you return a nullptr right away, and don't leak, in the event of a failure returned from the recursive calls at lines 220 and 226

In RBBINode::flattenSets(), maybe make the setRefNode variables into localPointers, so you can more easily return immediately if the cloneTree() calls set an error.

[jira-pull-request-webhook[bot]]

Notice: the branch changed across the force-push!

• icu4c/source/common/rbbinode.cpp is different
• icu4c/source/common/rbbiscan.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[FrankYFTang]

Please add a unit test that triggers the recursion depth failures.

I have the problem to do that. The fuzzer found test case is > 400k in a file.

[jira-pull-request-webhook[bot]]

Notice: the branch changed across the force-push!

• icu4c/source/common/rbbinode.cpp is different
• icu4c/source/common/rbbiscan.cpp is different
• icu4c/source/common/rbbisetb.cpp is different
• icu4c/source/common/rbbitblb.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[aheninger]

Please add a unit test that triggers the recursion depth failures.

I have the problem to do that. The fuzzer found test case is > 400k in a file.

Hi Frank, I've started to look at creating test cases, but have run into some behavior that I don't fully understand. I'll continue with it and get you something workable. Unfortunately I will be busy for most of the coming week, so it won't happen right away.

[FrankYFTang]

Please add a unit test that triggers the recursion depth failures.

I have the problem to do that. The fuzzer found test case is > 400k in a file.

Hi Frank, I've started to look at creating test cases, but have run into some behavior that I don't fully understand. I'll continue with it and get you something workable. Unfortunately I will be busy for most of the coming week, so it won't happen right away.

ping

[FrankYFTang]

ping

[FrankYFTang]

ping

[richgillam]

Well, looks like @aheninger is not responding. Either I can take a look or @eggrobin can...

[markusicu]

@richgillam:

Well, looks like @aheninger is not responding. Either I can take a look or @eggrobin can...

The ticket already has another merged PR, so the ticket must be closed by Wed feb12. Please work with @FrankYFTang to either land this PR here or retarget it to a new ticket.

Note that aside from Andy's comments there are also CI check failures.

[jira-pull-request-webhook[bot]]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[FrankYFTang]

rebase to figut out what is the other breakage on the CI first.

[FrankYFTang]

@eggrobin @richgillam could you review this again. I try to fix the CI failure if they show up. @aheninger said he will add more tests but not coming back to shis for along time. How about we land this after I fix all CI issues

[richgillam]

@eggrobin @richgillam could you review this again.

The changes look fine to me.

I try to fix the CI failure if they show up. @aheninger said he will add more tests but not coming back to shis for along time. How about we land this after I fix all CI issues

I'm fine with landing this in its current state and filing a ticket or something for the additional tests Andy wants to see.