From a3ff3528318accef1d4817aa16b0fe77b721581f Mon Sep 17 00:00:00 2001 From: Thomas Clegg Date: Sat, 5 Sep 2026 00:04:47 -0500 Subject: [PATCH 1/3] fix: make national-prefix stripping's null handling provable Two CodeQL correctness alerts, both in code whose null handling was correct but not locally provable. cs/dereferenced-value-may-be-null, PhoneNumberUtil.cs:2614. The internal MaybeStripNationalPrefixAndCarrierCode overload accepts the number as a StringBuilder, as a string, or as both, and folded the two into a single `numberString?.Length ?? number?.Length ?? 0` before dereferencing `number` in `numberString ??= number.ToString()`. The dereference is in fact unreachable with a null `number` -- reaching it requires `numberString` to be null, which makes the length come from `number`, so a null `number` returns early -- but that argument spans two statements and a `??` chain, and every other use of `number` in the method is null-conditional, so the code reads as if the ToString() were the one unguarded access. Branch on which form was supplied instead, so the ToString() sits inside the arm where `number` is known to be non-null. Behaviour and the allocation-free early return are unchanged: the string is still only materialised after the length and national-prefix checks pass, and a null or empty number still returns false without touching `metadata`. cs/null-argument-to-equals, TestMetadataFilter.cs:892. The test asserted MetadataFilter.Equals(null) is false, which is a real contract worth holding, but the literal null argument reads as a comparison the compiler can decide. Pass the null through an `object?` local so the call is a run-time reference comparison, and extend the test to the rest of the contract the override implements: false for a non-MetadataFilter object, true for a separately built filter with an equal blacklist, false for one with a different blacklist. Also add TestMaybeStripNationalPrefixLeavesEmptyNumberAlone, covering the zero-length early return that the restructure touched, with and without a carrier code requested. --- .../PhoneNumbers.Test/TestMetadataFilter.cs | 19 +++++++++++++++--- .../PhoneNumbers.Test/TestPhoneNumberUtil.cs | 19 ++++++++++++++++++ csharp/PhoneNumbers/PhoneNumberUtil.cs | 20 +++++++++++++++---- 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/csharp/PhoneNumbers.Test/TestMetadataFilter.cs b/csharp/PhoneNumbers.Test/TestMetadataFilter.cs index 61352bb0..8f688c22 100644 --- a/csharp/PhoneNumbers.Test/TestMetadataFilter.cs +++ b/csharp/PhoneNumbers.Test/TestMetadataFilter.cs @@ -887,10 +887,23 @@ public void TestIntegrityOfFieldSets() } [Fact] - public void TestEquals_WhenNull_ReturnsFalse() + public void TestEqualsRejectsNullAndForeignTypesAndAcceptsSameBlacklist() { - var result = new MetadataFilter(new Dictionary>()).Equals(null); - Assert.False(result); + var filter = new MetadataFilter(new Dictionary>()); + + // Object.Equals must answer false for a null reference rather than throwing. The null is + // held in a variable so the call is a real reference comparison at run time rather than + // a literal argument, which reads as a compile-time constant comparison. + object? nullReference = null; + Assert.False(filter.Equals(nullReference)); + + // ... and false for an object that is not a MetadataFilter at all. + Assert.False(filter.Equals("not a MetadataFilter")); + + // Equality is by blacklist contents, so a separately built empty filter is equal, and a + // filter carrying a non-empty blacklist is not. + Assert.True(filter.Equals(MetadataFilter.EmptyFilter())); + Assert.False(filter.Equals(MetadataFilter.ForLiteBuild())); } [Fact] diff --git a/csharp/PhoneNumbers.Test/TestPhoneNumberUtil.cs b/csharp/PhoneNumbers.Test/TestPhoneNumberUtil.cs index d2e365ff..418264e9 100644 --- a/csharp/PhoneNumbers.Test/TestPhoneNumberUtil.cs +++ b/csharp/PhoneNumbers.Test/TestPhoneNumberUtil.cs @@ -1952,6 +1952,25 @@ public void TestMaybeStripNationalPrefix() Assert.Equal(transformedNumber, numberToStrip.ToString()); } + [Fact] + public void TestMaybeStripNationalPrefixLeavesEmptyNumberAlone() + { + var metadata = new PhoneMetadata.Builder() + .SetNationalPrefixForParsing("34") + .SetGeneralDesc(new PhoneNumberDesc.Builder().SetNationalNumberPattern("\\d{4,8}").Build()) + .BuildPartial(); + // A zero-length number has no national prefix to strip, and asking must not throw even + // though there is nothing to turn into a string. + var numberToStrip = new StringBuilder(); + Assert.False(phoneUtil.MaybeStripNationalPrefixAndCarrierCode(numberToStrip, metadata, null)); + Assert.Equal("", numberToStrip.ToString()); + + // Same for a carrier code being requested. + var carrierCode = new StringBuilder(); + Assert.False(phoneUtil.MaybeStripNationalPrefixAndCarrierCode(numberToStrip, metadata, carrierCode)); + Assert.Equal("", carrierCode.ToString()); + } + [Fact] public void TestMaybeStripInternationalPrefix() { diff --git a/csharp/PhoneNumbers/PhoneNumberUtil.cs b/csharp/PhoneNumbers/PhoneNumberUtil.cs index 236b3424..93acdd97 100644 --- a/csharp/PhoneNumbers/PhoneNumberUtil.cs +++ b/csharp/PhoneNumbers/PhoneNumberUtil.cs @@ -2604,14 +2604,26 @@ public bool MaybeStripNationalPrefixAndCarrierCode(StringBuilder number, PhoneMe internal bool MaybeStripNationalPrefixAndCarrierCode(StringBuilder number, string numberString, PhoneMetadata metadata, bool getCarrier, out string carrierCode) { carrierCode = null; - var numberLength = numberString?.Length ?? number?.Length ?? 0; - if (numberLength == 0 || !metadata.HasNationalPrefixForParsing) + // Callers supply either form of the number: the ones that already hold a string pass it + // so the StringBuilder need not be materialised, while the public overload passes only + // the StringBuilder. Branch on which one was supplied rather than folding both into a + // single length, so the ToString() below is reached only when the StringBuilder is the + // form that is present. + if (numberString is null) + { + if (number is null || number.Length == 0 || !metadata.HasNationalPrefixForParsing) + { + // Early return for numbers of zero length. + return false; + } + // Attempt to parse the first digits as a national prefix. + numberString = number.ToString(); + } + else if (numberString.Length == 0 || !metadata.HasNationalPrefixForParsing) { // Early return for numbers of zero length. return false; } - // Attempt to parse the first digits as a national prefix. - numberString ??= number.ToString(); // Whether the groups are needed at all is known before matching: only a transform rule or // a requested carrier code reads them. Without either, the length of the prefix is the From 63aa8b2778e7d504f189c2688dd6bb4aa6681ed5 Mon Sep 17 00:00:00 2001 From: Thomas Clegg Date: Sat, 5 Sep 2026 00:35:38 -0500 Subject: [PATCH 2/3] test: compare against a foreign object through an object local The widened equality test passed a string literal straight to Equals, which trips cs/equals-on-unrelated-types (error) - CodeQL sees a comparison between statically incomparable types, which is exactly the smell that rule exists to catch. Asserting that Equals rejects a foreign type is the one place the comparison is deliberate, so hold it in an object local, the same way the null case already does. --- csharp/PhoneNumbers.Test/TestMetadataFilter.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/csharp/PhoneNumbers.Test/TestMetadataFilter.cs b/csharp/PhoneNumbers.Test/TestMetadataFilter.cs index 8f688c22..bc4664f8 100644 --- a/csharp/PhoneNumbers.Test/TestMetadataFilter.cs +++ b/csharp/PhoneNumbers.Test/TestMetadataFilter.cs @@ -897,8 +897,11 @@ public void TestEqualsRejectsNullAndForeignTypesAndAcceptsSameBlacklist() object? nullReference = null; Assert.False(filter.Equals(nullReference)); - // ... and false for an object that is not a MetadataFilter at all. - Assert.False(filter.Equals("not a MetadataFilter")); + // ... and false for an object that is not a MetadataFilter at all. Held in an object + // local for the same reason: passing the string literal straight in is a comparison + // between statically incomparable types, which is a real smell everywhere except here. + object foreignObject = "not a MetadataFilter"; + Assert.False(filter.Equals(foreignObject)); // Equality is by blacklist contents, so a separately built empty filter is equal, and a // filter carrying a non-empty blacklist is not. From fc0b8db44e7b1c2b6e087935900bf90a49703d1e Mon Sep 17 00:00:00 2001 From: Thomas Clegg Date: Sat, 5 Sep 2026 20:27:17 -0500 Subject: [PATCH 3/3] refactor: hoist the shared national-prefix guard Both arms of the numberString/number branch repeated !metadata.HasNationalPrefixForParsing. Check it once up front - it does not depend on which form of the number the caller supplied - so each arm is left with only its own zero-length test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K1NSXom6AbfgVknJDtfR9C --- csharp/PhoneNumbers/PhoneNumberUtil.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/csharp/PhoneNumbers/PhoneNumberUtil.cs b/csharp/PhoneNumbers/PhoneNumberUtil.cs index 93acdd97..35adc448 100644 --- a/csharp/PhoneNumbers/PhoneNumberUtil.cs +++ b/csharp/PhoneNumbers/PhoneNumberUtil.cs @@ -2604,6 +2604,10 @@ public bool MaybeStripNationalPrefixAndCarrierCode(StringBuilder number, PhoneMe internal bool MaybeStripNationalPrefixAndCarrierCode(StringBuilder number, string numberString, PhoneMetadata metadata, bool getCarrier, out string carrierCode) { carrierCode = null; + if (!metadata.HasNationalPrefixForParsing) + { + return false; + } // Callers supply either form of the number: the ones that already hold a string pass it // so the StringBuilder need not be materialised, while the public overload passes only // the StringBuilder. Branch on which one was supplied rather than folding both into a @@ -2611,15 +2615,15 @@ internal bool MaybeStripNationalPrefixAndCarrierCode(StringBuilder number, strin // form that is present. if (numberString is null) { - if (number is null || number.Length == 0 || !metadata.HasNationalPrefixForParsing) + // Early return for numbers of zero length. + if (number is null || number.Length == 0) { - // Early return for numbers of zero length. return false; } // Attempt to parse the first digits as a national prefix. numberString = number.ToString(); } - else if (numberString.Length == 0 || !metadata.HasNationalPrefixForParsing) + else if (numberString.Length == 0) { // Early return for numbers of zero length. return false;