ICU-23353 Fix double-delete in Region::cleanupRegionData

[hirorogo]

Checklist

• Required: Issue filed: ICU-23353
• Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable

Summary

Region::cleanupRegionData() in region.cpp deletes/closes availableRegions[], regionAliases, numericCodeMap, and regionIDMap but does not nullify the pointers afterward. If cleanup is called multiple times (e.g., via repeated u_cleanup() calls), the same memory is freed twice, causing a double-delete / use-after-free.

Fix: Set each pointer to NULL immediately after delete/uhash_close so subsequent calls are safe no-ops.

Test plan

• TestDoubleCleanup added to regiontst.cpp: initializes Region data, calls u_cleanup() twice in a row, then verifies Region can be re-initialized and used normally. Without the fix this crashes on the second u_cleanup().
• Existing ICU region tests should continue to pass.

[hirorogo]

Could a maintainer please transition the Jira ticket ICU-23353 from New to Accepted? The jira-ticket CI check requires the ticket to be in "Accepted" status, but as a new contributor I don't have permission to change the ticket status. Thank you!

[jira-pull-request-webhook]

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

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

Please replace NULL with nullptr everywhere in C++.

[hirorogo]

Done — replaced all NULL with nullptr in the changed lines.

[markusicu]

Calling u_cleanup() without closing all ICU objects is probably not defined, and calling it in the middle of the large test suite might have unforeseen side effects. Especially if we run tests in parallel.

The intltest test suite calls u_cleanup() at the very end. Does this problem reproduce if we call u_cleanup() twice there?

Otherwise, we might need to spin up a mini test suite just for this.

@aheninger @echeran WDYT?

[hirorogo]

Good point — removed the mid-suite u_cleanup() calls entirely. The test now only verifies Region data consistency (getInstance + contains).

The actual double-cleanup crash reproducer is documented in a comment block as a standalone program. This avoids side effects on other ICU services and is safe for parallel test execution.

If you'd prefer a dedicated mini test binary instead, happy to set that up.

[hirorogo]

While working on this fix, I noticed the same pattern (free-without-nullify in cleanup functions) exists in several other places on the main branch. These would also crash or corrupt memory if u_cleanup() is called twice.

1. dayPeriodRulesCleanup() — nullptr dereference

File: icu4c/source/i18n/dayperiodrules.cpp:307-313

U_CFUNC UBool U_CALLCONV dayPeriodRulesCleanup() {
    delete[] data->rules;          // 2nd call: data is nullptr → crash
    uhash_close(data->localeToRuleSetNumMap);
    delete data;
    data = nullptr;
    return true;
}

First call sets data = nullptr, but the function has no if (data) guard, so a second call dereferences nullptr.

2. allowedHourFormatsCleanup() — double uhash_close

File: icu4c/source/i18n/dtptngen.cpp:468-471

U_CFUNC UBool U_CALLCONV allowedHourFormatsCleanup() {
    uhash_close(localeToAllowedHourFormatsMap);
    // missing: localeToAllowedHourFormatsMap = nullptr;
    return true;
}

No nullification, no UInitOnce reset. Second call closes already-freed hash.

3. cleanupKnownCanonicalized() — double uhash_close

File: icu4c/source/common/locid.cpp:640-644

UBool U_CALLCONV cleanupKnownCanonicalized() {
    gKnownCanonicalizedInitOnce.reset();
    if (gKnownCanonicalized) { uhash_close(gKnownCanonicalized); }
    // missing: gKnownCanonicalized = nullptr;
    return true;
}

The if guard passes on second call because the pointer is dangling (non-null), not nullptr.

4. AliasData::cleanup() — double delete

File: icu4c/source/common/locid.cpp:806-812

UBool U_CALLCONV AliasData::cleanup() {
    gInitOnce.reset();
    delete gSingleton;
    // missing: gSingleton = nullptr;
    return true;
}

Happy to submit a separate PR for these if you'd like, or leave it to you. Just wanted to flag them since they're the same class of bug.

[jira-pull-request-webhook]

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

~ Your Friendly Jira-GitHub PR Checker Bot