From 2c5d85e1865f14e73af327f5dbfdb1139b232f03 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 May 2025 13:35:08 +0200 Subject: [PATCH 1/5] C#: Convert cs/gethashcode-is-not-defined to inline expectations tests. --- .../Likely Bugs/HashedButNoHash/HashedButNoHash.cs | 5 +++-- .../Likely Bugs/HashedButNoHash/HashedButNoHash.expected | 2 +- .../Likely Bugs/HashedButNoHash/HashedButNoHash.qlref | 4 +++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs index 1c088acfce23..7a69efca4440 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs +++ b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs @@ -6,9 +6,10 @@ public class Test public void M() { var h = new Hashtable(); - h.Add(this, null); // BAD + h.Add(this, null); // $ Alert + var d = new Dictionary(); - d.Add(this, false); // BAD + d.Add(this, false); // $ Alert } public override bool Equals(object other) diff --git a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected index 139f62dfe5bd..cde53dcd2470 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected +++ b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected @@ -1,2 +1,2 @@ | HashedButNoHash.cs:9:15:9:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | -| HashedButNoHash.cs:11:15:11:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:12:15:12:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | diff --git a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.qlref b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.qlref index 611db7b8e0de..2686c9c06e81 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.qlref +++ b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.qlref @@ -1 +1,3 @@ -Likely Bugs/HashedButNoHash.ql \ No newline at end of file +query: Likely Bugs/HashedButNoHash.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql + From 4b2d323cb6028209e48faf9bc484f7cf2e6e3a13 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 May 2025 13:44:12 +0200 Subject: [PATCH 2/5] C#: Add some more test cases. --- .../HashedButNoHash/HashedButNoHash.cs | 17 +++++++++++++++++ .../HashedButNoHash/HashedButNoHash.expected | 16 +++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs index 7a69efca4440..6ddd7fb037d5 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs +++ b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs @@ -7,9 +7,26 @@ public void M() { var h = new Hashtable(); h.Add(this, null); // $ Alert + h.Contains(this); // $ Alert + h.ContainsKey(this); // $ Alert + h[this] = null; // $ Alert + h.Remove(this); // $ Alert + + var l = new List(); + l.Add(this); // Good var d = new Dictionary(); d.Add(this, false); // $ Alert + d.ContainsKey(this); // $ Alert + d[this] = false; // $ Alert + d.Remove(this); // $ Alert + d.TryAdd(this, false); // $ Alert + d.TryGetValue(this, out bool _); // $ Alert + + var hs = new HashSet(); + hs.Add(this); // $ Alert + hs.Contains(this); // $ Alert + hs.Remove(this); // $ Alert } public override bool Equals(object other) diff --git a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected index cde53dcd2470..2e6e3cba4ba5 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected +++ b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected @@ -1,2 +1,16 @@ +#select | HashedButNoHash.cs:9:15:9:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | -| HashedButNoHash.cs:12:15:12:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:11:23:11:26 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:19:15:19:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:20:23:20:26 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +testFailures +| HashedButNoHash.cs:10:27:10:36 | // ... | Missing result: Alert | +| HashedButNoHash.cs:12:25:12:34 | // ... | Missing result: Alert | +| HashedButNoHash.cs:13:25:13:34 | // ... | Missing result: Alert | +| HashedButNoHash.cs:21:26:21:35 | // ... | Missing result: Alert | +| HashedButNoHash.cs:22:25:22:34 | // ... | Missing result: Alert | +| HashedButNoHash.cs:23:32:23:41 | // ... | Missing result: Alert | +| HashedButNoHash.cs:24:42:24:51 | // ... | Missing result: Alert | +| HashedButNoHash.cs:27:23:27:32 | // ... | Missing result: Alert | +| HashedButNoHash.cs:28:28:28:37 | // ... | Missing result: Alert | +| HashedButNoHash.cs:29:26:29:35 | // ... | Missing result: Alert | From 72d3814e08624d051493391532c5089453ce546e Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 May 2025 14:03:22 +0200 Subject: [PATCH 3/5] C#: Include dictionary indexers and more methods in cs/gethashcode-is-not-defined. --- .../frameworks/system/collections/Generic.qll | 17 ++++++++ csharp/ql/src/Likely Bugs/HashedButNoHash.ql | 40 ++++++++++++++----- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/system/collections/Generic.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/system/collections/Generic.qll index 24c1277af026..35b23ffc18e8 100644 --- a/csharp/ql/lib/semmle/code/csharp/frameworks/system/collections/Generic.qll +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/system/collections/Generic.qll @@ -159,6 +159,15 @@ class SystemCollectionsGenericIDictionaryInterface extends SystemCollectionsGene } } +/** The ``System.Collections.Generic.IReadOnlyDictionary`2`` interface. */ +class SystemCollectionsGenericIReadOnlyDictionaryInterface extends SystemCollectionsGenericUnboundGenericInterface +{ + SystemCollectionsGenericIReadOnlyDictionaryInterface() { + this.hasName("IReadOnlyDictionary`2") and + this.getNumberOfTypeParameters() = 2 + } +} + /** The ``System.Collections.Generic.IReadOnlyCollection`1`` interface. */ class SystemCollectionsGenericIReadOnlyCollectionTInterface extends SystemCollectionsGenericUnboundGenericInterface { @@ -176,3 +185,11 @@ class SystemCollectionsGenericIReadOnlyListTInterface extends SystemCollectionsG this.getNumberOfTypeParameters() = 1 } } + +/** The ``System.Collections.Generic.HashSet`1`` class. */ +class SystemCollectionsGenericHashSetClass extends SystemCollectionsGenericUnboundGenericClass { + SystemCollectionsGenericHashSetClass() { + this.hasName("HashSet`1") and + this.getNumberOfTypeParameters() = 1 + } +} diff --git a/csharp/ql/src/Likely Bugs/HashedButNoHash.ql b/csharp/ql/src/Likely Bugs/HashedButNoHash.ql index 195c0e3a0560..9948def013c9 100644 --- a/csharp/ql/src/Likely Bugs/HashedButNoHash.ql +++ b/csharp/ql/src/Likely Bugs/HashedButNoHash.ql @@ -11,24 +11,44 @@ import csharp import semmle.code.csharp.frameworks.System +import semmle.code.csharp.frameworks.system.Collections +import semmle.code.csharp.frameworks.system.collections.Generic -predicate dictionary(ConstructedType constructed) { - exists(UnboundGenericType dict | - dict.hasFullyQualifiedName("System.Collections.Generic", "Dictionary`2") and - constructed = dict.getAConstructedGeneric() +/** + * Holds if `t` is a dictionary type. + */ +predicate dictionary(ValueOrRefType t) { + exists(Type base | base = t.getABaseType*().getUnboundDeclaration() | + base instanceof SystemCollectionsGenericIDictionaryInterface or + base instanceof SystemCollectionsGenericIReadOnlyDictionaryInterface or + base instanceof SystemCollectionsIDictionaryInterface ) } -predicate hashtable(Class c) { c.hasFullyQualifiedName("System.Collections", "Hashtable") } +/** + * Holds if `c` is a hashset type. + */ +predicate hashSet(ValueOrRefType t) { + t.getABaseType*().getUnboundDeclaration() instanceof SystemCollectionsGenericHashSetClass +} -predicate hashstructure(Type t) { hashtable(t) or dictionary(t) } +predicate hashStructure(Type t) { dictionary(t) or hashSet(t) } -predicate hashAdd(Expr e) { +/** + * Holds if the expression `e` relies on `GetHashCode()` implementation. + * That is, if the call assumes that `e1.Equals(e2)` implies `e1.GetHashCode() == e2.GetHashCode()`. + */ +predicate usesHashing(Expr e) { exists(MethodCall mc, string name | - (name = "Add" or name = "ContainsKey") and + name = ["Add", "Contains", "ContainsKey", "Remove", "TryAdd", "TryGetValue"] and mc.getArgument(0) = e and mc.getTarget().hasName(name) and - hashstructure(mc.getTarget().getDeclaringType()) + hashStructure(mc.getTarget().getDeclaringType()) + ) + or + exists(IndexerCall ic | + ic.getArgument(0) = e and + dictionary(ic.getTarget().getDeclaringType()) ) } @@ -46,7 +66,7 @@ predicate hashCall(Expr e) { from Expr e, Type t where - (hashAdd(e) or hashCall(e)) and + (usesHashing(e) or hashCall(e)) and e.getType() = t and eqWithoutHash(t) select e, From 3080dfafb6bfbbc3f3b08c389d88aebf4317a5f6 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 May 2025 14:04:40 +0200 Subject: [PATCH 4/5] C#: Update test expected output. --- .../HashedButNoHash/HashedButNoHash.expected | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected index 2e6e3cba4ba5..ad5ab000c7d0 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected +++ b/csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected @@ -1,16 +1,14 @@ -#select | HashedButNoHash.cs:9:15:9:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:10:20:10:23 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | | HashedButNoHash.cs:11:23:11:26 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:12:11:12:14 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:13:18:13:21 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | | HashedButNoHash.cs:19:15:19:18 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | | HashedButNoHash.cs:20:23:20:26 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | -testFailures -| HashedButNoHash.cs:10:27:10:36 | // ... | Missing result: Alert | -| HashedButNoHash.cs:12:25:12:34 | // ... | Missing result: Alert | -| HashedButNoHash.cs:13:25:13:34 | // ... | Missing result: Alert | -| HashedButNoHash.cs:21:26:21:35 | // ... | Missing result: Alert | -| HashedButNoHash.cs:22:25:22:34 | // ... | Missing result: Alert | -| HashedButNoHash.cs:23:32:23:41 | // ... | Missing result: Alert | -| HashedButNoHash.cs:24:42:24:51 | // ... | Missing result: Alert | -| HashedButNoHash.cs:27:23:27:32 | // ... | Missing result: Alert | -| HashedButNoHash.cs:28:28:28:37 | // ... | Missing result: Alert | -| HashedButNoHash.cs:29:26:29:35 | // ... | Missing result: Alert | +| HashedButNoHash.cs:21:11:21:14 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:22:18:22:21 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:23:18:23:21 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:24:23:24:26 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:27:16:27:19 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:28:21:28:24 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | +| HashedButNoHash.cs:29:19:29:22 | this access | This expression is hashed, but type 'Test' only defines Equals(...) not GetHashCode(). | From 4d7901573ac17ea9d6b6bc7db8b1abab5a2cb5c6 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 May 2025 14:07:50 +0200 Subject: [PATCH 5/5] C#: Add change note. --- .../src/change-notes/2025-05-15-gethashcode-is-not-defined.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/src/change-notes/2025-05-15-gethashcode-is-not-defined.md diff --git a/csharp/ql/src/change-notes/2025-05-15-gethashcode-is-not-defined.md b/csharp/ql/src/change-notes/2025-05-15-gethashcode-is-not-defined.md new file mode 100644 index 000000000000..2d8c5c1c56e0 --- /dev/null +++ b/csharp/ql/src/change-notes/2025-05-15-gethashcode-is-not-defined.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The precision of the query `cs/gethashcode-is-not-defined` has been improved (false negative reduction). Calls to more methods (and indexers) that rely on the invariant `e1.Equals(e2)` implies `e1.GetHashCode() == e2.GetHashCode()` are taken into account.