Wrapper.Cleanup() leaves outstanding ICU handles dangling, causing AccessViolationException

[imnasnainaec]

Describe the bug

Wrapper.Cleanup() unloads the native ICU libraries but leaves every outstanding ICU
handle pointing into the unloaded module. Using any object created before the call — a
RuleBasedCollator, a break iterator, a transliterator — then dereferences freed memory
and takes down the process with an AccessViolationException, which managed code cannot
catch.

Nothing marks those objects as invalid, so the failure surfaces as a hard crash inside
native ICU rather than as ObjectDisposedException.

To Reproduce

Wrapper.Init();
var collator = new RuleBasedCollator("&b < a");
Console.WriteLine(collator.Compare("a", "b"));   // 1

Wrapper.Cleanup();                               // unloads icuuc/icuin

Console.WriteLine(collator.Compare("a", "b"));   // AccessViolationException

Fatal error. System.AccessViolationException: Attempted to read or write protected memory.
   at Icu.NativeMethods.ucol_strcoll(SafeRuleBasedCollatorHandle, System.String, Int32, System.String, Int32)
   at Icu.Collation.RuleBasedCollator.Compare(System.String, System.String)

Reproduced on ICU 54 and 62, on net8.0 and net48, x64 and x86 — it is not
version-dependent.

Expected behavior

Use of an ICU object after Wrapper.Cleanup() should throw ObjectDisposedException, not
corrupt the process. Cleanup() could track outstanding SafeHandles and invalidate them
before unloading, or at minimum the handle wrappers could check whether the library
generation they were created under is still loaded.

Why it matters

NativeMethods.Cleanup()
unloads both libraries; a later call re-resolves the delegate against a freshly loaded ICU
and hands it a UCollator* from the previous load.

Easy to hit when one component tears ICU down while another still holds objects:
FieldWorks' PUAInstaller calls Wrapper.Cleanup() to regenerate nfc_fw.nrm, while
libpalaso's IcuRulesCollator reuses collators from a static dictionary with no disposed
check.

The stack above is also frame-for-frame identical to #130, open since 2020 as an
intermittent collation crash. I could not reproduce that on any ICU from 44 to 67; this
path reproduces it exactly, and would be intermittent in the field since it depends on the
module's re-mapped base address.

[imnasnainaec]

All of these fixes share one root cause: nothing connects the managed objects holding ICU pointers to the moment NativeMethods.Cleanup() unloads icuuc/icuin. They differ in where staleness is detected and what happens to stale objects.

Option 0 — Library generation stamp (smallest)

Cleanup() increments a static NativeMethods.Generation. Each object that owns a native handle records the generation when it opens the handle.

• Using a handle from an older generation throws ObjectDisposedException.
• Dispose()/ReleaseHandle() skip closing a handle from an older generation.
• Never-opened handles (e.g. ResourceBundle.Null) can't go stale.

Staleness is checked at use/release time, so there's no registry and nothing depends on GC timing.

Trade-offs:

• Outstanding native objects are never closed, so their memory leaks.
• On macOS the library is never unloaded, so objects throw even though they're still valid.

Size (estimate): ~9 files, ~100–150 changed lines. No new types; NativeMethods_*.cs untouched.

Option 1 — Invalidate outstanding handles at cleanup (PR #239)

A registry of weak references tracks every object that owns a native handle. Cleanup() closes and invalidates them all while ICU is still loaded. Afterwards, using one throws ObjectDisposedException, and disposing one no longer calls into ICU. Never-opened handles can't go stale, and stay registered for later cleanups.

• The three SafeHandle types (collator, transliterator, enumerator) share a new SafeIcuHandle base. Cleanup() disposes them, which closes the native object and makes the interop marshaler throw at every call site.
• The six classes holding a plain IntPtr (BiDi, RuleBasedBreakIterator, RegexMatcher, MessageFormatter, ResourceBundle, Normalizer2) keep it in a NativeHandle wrapper that knows the ICU close method (none for Normalizer2, whose pointer ICU owns). Its Pointer getter throws when stale.

The registry uses long weak references (trackResurrection: true). A short weak reference is cleared before finalization, so Cleanup() would miss an undisposed object already queued for finalization. Its finalizer would then close the stale pointer and crash the process.

Every release path, finalizers included, holds the same lock as Cleanup() through the native close, so a release can't interleave with an unload.

Size: 15 files, ~685 added / ~130 removed lines.

• 3 new source files (~235 lines): registry + interface, NativeHandle, SafeIcuHandle
• 10 modified source files (+146 / −131), almost all mechanical (_x → _x.Pointer)
• 1 new test file (~300 lines, 15 tests), plus a CHANGELOG entry
• NativeMethods_*.cs untouched

Option 2 — Convert every native pointer to SafeHandle (most idiomatic)

Option 1's registry, but the six IntPtr classes get real SafeHandle subclasses. NativeHandle and the hand-rolled finalizers go away, and lifetime management is uniform.

Cleanup still disposes every handle, as in Option 1. The gain is that the marshaler's reference counting then covers every handle, deferring a close until any in-progress call on it returns.

The cost: the handle type must go all the way down to the P/Invoke signatures, so every affected delegate and wrapper changes from IntPtr.

Size: ~18 files, roughly 600–700 changed lines.

• ~120 signatures across 6 NativeMethods_*.cs files (30 wrappers in NativeMethods_BiDi.cs alone)
• 6 new SafeHandle classes (~180 lines)
• unorm2_getInstance returns an ICU-owned singleton, so its handle needs ownsHandle: false

Option 3 — Transparently re-create immutable objects

Options 0–2 turn the crash into an ObjectDisposedException. The motivating scenario still breaks, though: libpalaso caches collators while FieldWorks calls Cleanup().

Here, immutable objects remember their construction arguments and re-open under the new ICU on next use. That covers RuleBasedCollator, Transliterator, Normalizer2 and MessageFormatter. Stateful objects (break iterators, BiDi, RegexMatcher, child ResourceBundles) fall back to Option 0's check.

Trade-off: a rebuilt object may behave differently against the reloaded ICU data. That's the point for FieldWorks, but it's a semantic change to document.

Size (estimate): Option 0 plus re-open logic in ~4 classes; roughly 250–350 changed lines.

Recommendation

• Option 1 (PR fix: Invalidate outstanding ICU handles in Wrapper.Cleanup() #239) for this issue. It fixes the crash without touching the P/Invoke layer.
• Option 0 is a smaller alternative, at the cost of leaking native objects.
• Option 2 is a worthwhile refactor, but deserves its own issue and review.
• Option 3 is worth raising with the libpalaso/FieldWorks maintainers. It's the only option that keeps their current usage working.

[imnasnainaec]

Deciding between Option 1 and 2.