diff --git a/CHANGELOG.md b/CHANGELOG.md index cd18d01d..52ee8feb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Create nuget symbol package - Add BiDi class (#121; jeffska) +### Fixed + +- Crash on Linux disposing `RuleBasedCollator` + ## [2.5.4] - 2019-01-09 ### Fixed diff --git a/source/icu.net/Collation/RuleBasedCollator.cs b/source/icu.net/Collation/RuleBasedCollator.cs index 3366f2ec..7610f7e6 100644 --- a/source/icu.net/Collation/RuleBasedCollator.cs +++ b/source/icu.net/Collation/RuleBasedCollator.cs @@ -1,9 +1,8 @@ -// Copyright (c) 2013 SIL International +// Copyright (c) 2013-2019 SIL International // This software is licensed under the MIT license (http://opensource.org/licenses/MIT) using System; using System.Collections.Generic; using System.Runtime.InteropServices; - #if NETSTANDARD1_6 using Icu; #else @@ -38,7 +37,7 @@ protected override bool ReleaseHandle() { try { - if (handle != IntPtr.Zero) + if (!IsInvalid) NativeMethods.ucol_close(handle); handle = IntPtr.Zero; return true; @@ -56,14 +55,11 @@ protected override bool ReleaseHandle() /// ///true if the handle is valid; otherwise, false. /// - public override bool IsInvalid - { - get { return handle == IntPtr.Zero; } - } + public override bool IsInvalid => handle == IntPtr.Zero || handle == new IntPtr(-1) || IsClosed; } - private bool _disposingValue = false; // To detect redundant calls - private SafeRuleBasedCollatorHandle _collatorHandle = default(SafeRuleBasedCollatorHandle); + private bool _disposingValue; // To detect redundant calls + private SafeRuleBasedCollatorHandle _collatorHandle; private RuleBasedCollator() {} @@ -95,23 +91,21 @@ public RuleBasedCollator(string rules, NormalizationMode normalizationMode, CollationStrength collationStrength) { - ErrorCode status; var parseError = new ParseError(); _collatorHandle = NativeMethods.ucol_openRules(rules, rules.Length, normalizationMode, collationStrength, ref parseError, - out status); + out var status); try { ExceptionFromErrorCode.ThrowIfError(status, parseError.ToString(rules)); } catch { - if (_collatorHandle != default(SafeRuleBasedCollatorHandle)) - _collatorHandle.Dispose(); - _collatorHandle = default(SafeRuleBasedCollatorHandle); + _collatorHandle?.Dispose(); + _collatorHandle = default; throw; } } @@ -127,12 +121,9 @@ public RuleBasedCollator(string rules, /// public override CollationStrength Strength { - get { return (CollationStrength) GetAttribute(NativeMethods.CollationAttribute.Strength); } - set - { - SetAttribute(NativeMethods.CollationAttribute.Strength, - (NativeMethods.CollationAttributeValue) value); - } + get => (CollationStrength) GetAttribute(NativeMethods.CollationAttribute.Strength); + set => SetAttribute(NativeMethods.CollationAttribute.Strength, + (NativeMethods.CollationAttributeValue) value); } /// @@ -141,12 +132,9 @@ public override CollationStrength Strength /// public override NormalizationMode NormalizationMode { - get { return (NormalizationMode) GetAttribute(NativeMethods.CollationAttribute.NormalizationMode); } - set - { - SetAttribute(NativeMethods.CollationAttribute.NormalizationMode, - (NativeMethods.CollationAttributeValue) value); - } + get => (NormalizationMode) GetAttribute(NativeMethods.CollationAttribute.NormalizationMode); + set => SetAttribute(NativeMethods.CollationAttribute.NormalizationMode, + (NativeMethods.CollationAttributeValue) value); } /// @@ -154,12 +142,9 @@ public override NormalizationMode NormalizationMode /// public override FrenchCollation FrenchCollation { - get { return (FrenchCollation) GetAttribute(NativeMethods.CollationAttribute.FrenchCollation); } - set - { - SetAttribute(NativeMethods.CollationAttribute.FrenchCollation, - (NativeMethods.CollationAttributeValue) value); - } + get => (FrenchCollation) GetAttribute(NativeMethods.CollationAttribute.FrenchCollation); + set => SetAttribute(NativeMethods.CollationAttribute.FrenchCollation, + (NativeMethods.CollationAttributeValue) value); } /// @@ -171,12 +156,9 @@ public override FrenchCollation FrenchCollation /// public override CaseLevel CaseLevel { - get { return (CaseLevel) GetAttribute(NativeMethods.CollationAttribute.CaseLevel); } - set - { - SetAttribute(NativeMethods.CollationAttribute.CaseLevel, - (NativeMethods.CollationAttributeValue) value); - } + get => (CaseLevel) GetAttribute(NativeMethods.CollationAttribute.CaseLevel); + set => SetAttribute(NativeMethods.CollationAttribute.CaseLevel, + (NativeMethods.CollationAttributeValue) value); } /// @@ -187,12 +169,9 @@ public override CaseLevel CaseLevel [Obsolete("ICU 50 Implementation detail, cannot be set via API, was removed from implementation.")] public override HiraganaQuaternary HiraganaQuaternary { - get { return (HiraganaQuaternary) GetAttribute(NativeMethods.CollationAttribute.HiraganaQuaternaryMode); } - set - { - SetAttribute(NativeMethods.CollationAttribute.HiraganaQuaternaryMode, - (NativeMethods.CollationAttributeValue) value); - } + get => (HiraganaQuaternary) GetAttribute(NativeMethods.CollationAttribute.HiraganaQuaternaryMode); + set => SetAttribute(NativeMethods.CollationAttribute.HiraganaQuaternaryMode, + (NativeMethods.CollationAttributeValue) value); } /// @@ -202,12 +181,9 @@ public override HiraganaQuaternary HiraganaQuaternary /// public override NumericCollation NumericCollation { - get { return (NumericCollation) GetAttribute(NativeMethods.CollationAttribute.NumericCollation); } - set - { - SetAttribute(NativeMethods.CollationAttribute.NumericCollation, - (NativeMethods.CollationAttributeValue) value); - } + get => (NumericCollation) GetAttribute(NativeMethods.CollationAttribute.NumericCollation); + set => SetAttribute(NativeMethods.CollationAttribute.NumericCollation, + (NativeMethods.CollationAttributeValue) value); } /// @@ -215,12 +191,9 @@ public override NumericCollation NumericCollation /// public override CaseFirst CaseFirst { - get { return (CaseFirst) GetAttribute(NativeMethods.CollationAttribute.CaseFirst); } - set - { - SetAttribute(NativeMethods.CollationAttribute.CaseFirst, - (NativeMethods.CollationAttributeValue) value); - } + get => (CaseFirst) GetAttribute(NativeMethods.CollationAttribute.CaseFirst); + set => SetAttribute(NativeMethods.CollationAttribute.CaseFirst, + (NativeMethods.CollationAttributeValue) value); } /// @@ -228,12 +201,9 @@ public override CaseFirst CaseFirst /// public override AlternateHandling AlternateHandling { - get { return (AlternateHandling) GetAttribute(NativeMethods.CollationAttribute.AlternateHandling); } - set - { - SetAttribute(NativeMethods.CollationAttribute.AlternateHandling, - (NativeMethods.CollationAttributeValue) value); - } + get => (AlternateHandling) GetAttribute(NativeMethods.CollationAttribute.AlternateHandling); + set => SetAttribute(NativeMethods.CollationAttribute.AlternateHandling, + (NativeMethods.CollationAttributeValue) value); } private byte[] keyData = new byte[1024]; @@ -253,11 +223,8 @@ public override SortKey GetSortKey(string source) int actualLength; for (;;) { - actualLength = NativeMethods.ucol_getSortKey(_collatorHandle, - source, - source.Length, - keyData, - keyData.Length); + actualLength = NativeMethods.ucol_getSortKey(_collatorHandle, source, source.Length, + keyData, keyData.Length); if (actualLength > keyData.Length) { keyData = new byte[keyData.Length*2]; @@ -270,16 +237,14 @@ public override SortKey GetSortKey(string source) private NativeMethods.CollationAttributeValue GetAttribute(NativeMethods.CollationAttribute attr) { - ErrorCode e; - NativeMethods.CollationAttributeValue value = NativeMethods.ucol_getAttribute(_collatorHandle, attr, out e); + var value = NativeMethods.ucol_getAttribute(_collatorHandle, attr, out var e); ExceptionFromErrorCode.ThrowIfError(e); return value; } private void SetAttribute(NativeMethods.CollationAttribute attr, NativeMethods.CollationAttributeValue value) { - ErrorCode e; - NativeMethods.ucol_setAttribute(_collatorHandle, attr, value, out e); + NativeMethods.ucol_setAttribute(_collatorHandle, attr, value, out var e); ExceptionFromErrorCode.ThrowIfError(e); } @@ -290,18 +255,18 @@ private void SetAttribute(NativeMethods.CollationAttribute attr, NativeMethods.C /// public static IList GetAvailableCollationLocales() { - List locales = new List(); - // The ucol_openAvailableLocales call failes when there are no locales available, so check first. + var locales = new List(); + // The ucol_openAvailableLocales call fails when there are no locales available, so check first. if (NativeMethods.ucol_countAvailable() == 0) { return locales; } - ErrorCode ec; - SafeEnumeratorHandle en = NativeMethods.ucol_openAvailableLocales(out ec); + + var en = NativeMethods.ucol_openAvailableLocales(out var ec); ExceptionFromErrorCode.ThrowIfError(ec); try { - string str = en.Next(); + var str = en.Next(); while (str != null) { locales.Add(str); @@ -326,14 +291,13 @@ public static IList GetAvailableCollationLocales() /// public override object Clone() { - RuleBasedCollator copy = new RuleBasedCollator(); - ErrorCode status; - int buffersize = 512; + var copy = new RuleBasedCollator(); + var buffersize = 512; copy._collatorHandle = NativeMethods.ucol_safeClone( _collatorHandle, IntPtr.Zero, ref buffersize, - out status); + out var status); try { ExceptionFromErrorCode.ThrowIfError(status); @@ -341,9 +305,8 @@ public override object Clone() } catch { - if (copy._collatorHandle != default(SafeRuleBasedCollatorHandle)) - copy._collatorHandle.Dispose(); - copy._collatorHandle = default(SafeRuleBasedCollatorHandle); + copy._collatorHandle?.Dispose(); + copy._collatorHandle = default; throw; } } @@ -353,7 +316,7 @@ public override object Clone() /// Does not allow for locale fallback. /// /// Locale to use - public static new Collator Create(string localeId) + public new static Collator Create(string localeId) { return Create(localeId, Fallback.NoFallback); } @@ -370,50 +333,46 @@ public override object Clone() // format that ICU expects. var locale = new Locale(localeId); - var instance = new RuleBasedCollator(); - ErrorCode status; - instance._collatorHandle = NativeMethods.ucol_open(locale.Id, out status); + var instance = new RuleBasedCollator { + _collatorHandle = NativeMethods.ucol_open(locale.Id, out var status) + }; try { - if (status == ErrorCode.USING_FALLBACK_WARNING && fallback == Fallback.NoFallback) - { - throw new ArgumentException( - $"Could only create Collator '{localeId}' by falling back to " + - $"'{instance.ActualId}'. You can use the fallback option to create this."); - } - if (status == ErrorCode.USING_DEFAULT_WARNING && fallback == Fallback.NoFallback && - !instance.ValidId.Equals(locale.Id) && - locale.Id.Length > 0 && !locale.Id.Equals("root")) - { - throw new ArgumentException( - $"Could only create Collator '{localeId}' by falling back to the default " + - $"'{instance.ActualId}'. You can use the fallback option to create this."); - } - if (status == ErrorCode.INTERNAL_PROGRAM_ERROR && fallback == Fallback.FallbackAllowed) + switch (status) { - instance = new RuleBasedCollator(string.Empty); // fallback to UCA - } - else - { - try - { - ExceptionFromErrorCode.ThrowIfError(status); - } - catch (Exception e) - { + case ErrorCode.USING_FALLBACK_WARNING when fallback == Fallback.NoFallback: + throw new ArgumentException( + $"Could only create Collator '{localeId}' by falling back to " + + $"'{instance.ActualId}'. You can use the fallback option to create this."); + case ErrorCode.USING_DEFAULT_WARNING when fallback == Fallback.NoFallback && !instance.ValidId.Equals(locale.Id) && locale.Id.Length > 0 && !locale.Id.Equals("root"): throw new ArgumentException( - $"Unable to create a collator using the given localeId '{localeId}'.\n" + - "This is likely because the ICU data file was created without collation " + - "rules for this locale. You can provide the rules yourself or replace " + - "the data dll.", e); - } + $"Could only create Collator '{localeId}' by falling back to the default " + + $"'{instance.ActualId}'. You can use the fallback option to create this."); + case ErrorCode.INTERNAL_PROGRAM_ERROR when fallback == Fallback.FallbackAllowed: + instance = new RuleBasedCollator(string.Empty); // fallback to UCA + break; + default: + try + { + ExceptionFromErrorCode.ThrowIfError(status); + } + catch (Exception e) + { + throw new ArgumentException( + $"Unable to create a collator using the given localeId '{localeId}'.\n" + + "This is likely because the ICU data file was created without collation " + + "rules for this locale. You can provide the rules yourself or replace " + + "the data dll.", e); + } + + break; } + return instance; } catch { - if (instance._collatorHandle != default(SafeRuleBasedCollatorHandle)) - instance._collatorHandle.Dispose(); + instance._collatorHandle?.Dispose(); instance._collatorHandle = default(SafeRuleBasedCollatorHandle); throw; } @@ -423,17 +382,12 @@ private string ActualId { get { - ErrorCode status; if (_collatorHandle.IsInvalid) return string.Empty; // See NativeMethods.ucol_getLocaleByType for marshal information. - string result = Marshal.PtrToStringAnsi(NativeMethods.ucol_getLocaleByType( - _collatorHandle, NativeMethods.LocaleType.ActualLocale, out status)); - if (status != ErrorCode.NoErrors) - { - return string.Empty; - } - return result; + var result = Marshal.PtrToStringAnsi(NativeMethods.ucol_getLocaleByType( + _collatorHandle, NativeMethods.LocaleType.ActualLocale, out var status)); + return status != ErrorCode.NoErrors ? string.Empty : result; } } @@ -441,17 +395,12 @@ private string ValidId { get { - ErrorCode status; if (_collatorHandle.IsInvalid) return string.Empty; // See NativeMethods.ucol_getLocaleByType for marshal information. - string result = Marshal.PtrToStringAnsi(NativeMethods.ucol_getLocaleByType( - _collatorHandle, NativeMethods.LocaleType.ValidLocale, out status)); - if (status != ErrorCode.NoErrors) - { - return string.Empty; - } - return result; + var result = Marshal.PtrToStringAnsi(NativeMethods.ucol_getLocaleByType( + _collatorHandle, NativeMethods.LocaleType.ValidLocale, out var status)); + return status != ErrorCode.NoErrors ? string.Empty : result; } } #endregion @@ -495,25 +444,25 @@ public override int Compare(string string1, string string2) /// protected override void Dispose(bool disposing) { - if (!_disposingValue) + if (_disposingValue) + return; + + if (disposing) { - if (disposing) + // Dispose managed state (managed objects), if any. + + // although SafeRuleBasedCollatorHandle deals with an unmanaged resource + // it itself is a managed object, so we shouldn't try to dispose it + // if !disposing because that could lead to a corrupt stack (as observed + // in https://jenkins.lsdev.sil.org:45192/view/Icu/view/All/job/GitHub-IcuDotNet-Win-any-master-release/59) + if (!_collatorHandle.IsInvalid) { - // Dispose managed state (managed objects), if any. - - // although SafeRuleBasedCollatorHandle deals with an unmanaged resource - // it itself is a managed object, so we shouldn't try to dispose it - // if !disposing because that could lead to a corrupt stack (as observed - // in https://jenkins.lsdev.sil.org:45192/view/Icu/view/All/job/GitHub-IcuDotNet-Win-any-master-release/59) - if (_collatorHandle != default(SafeRuleBasedCollatorHandle)) - { - _collatorHandle.Dispose(); - } + _collatorHandle.Dispose(); } - _collatorHandle = default(SafeRuleBasedCollatorHandle); - - _disposingValue = true; } + _collatorHandle = default; + + _disposingValue = true; } /// diff --git a/source/icu.net/icu.net.csproj b/source/icu.net/icu.net.csproj index d35702ee..2e92425a 100644 --- a/source/icu.net/icu.net.csproj +++ b/source/icu.net/icu.net.csproj @@ -26,10 +26,10 @@ See full changelog at https://github.com/sillsdev/icu-dotnet/blob/master/CHANGEL false false false - true ../../CHANGELOG.md true snupkg + 7.1 @@ -41,7 +41,7 @@ See full changelog at https://github.com/sillsdev/icu-dotnet/blob/master/CHANGEL true icu.net.snk - + true