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, 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. 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..6ddd7fb037d5 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,27 @@ public class Test public void M() { var h = new Hashtable(); - h.Add(this, null); // BAD + 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); // BAD + 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 139f62dfe5bd..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,2 +1,14 @@ | 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: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(). | +| 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(). | 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 +