From e181f5ebae5674b37d776eed8a9396f87282ecf1 Mon Sep 17 00:00:00 2001 From: Alex Klein Date: Fri, 17 Jul 2026 11:47:04 -0400 Subject: [PATCH] This fixes a couple of issues where GetCacheInfo is called with null exampleParameters for an Identity that has non-null parameters. For example, deferred GridReader and async MultiMap calls do an initial call with non-null example parameters, and then do a second call with null example parameters. Usually this works fine because the cache entry is created in the first call with the correct reader and the second call results in a cache hit based on the Identity. However, with multiple concurrent Dapper calls happening at once, CollectCacheGarbage can sometimes be hit after the first GetCacheInfo call and before the second. This results in the second call (with null example parameters) creating a cache entry with a bad reader that breaks all future usages of that Identity until that entry is removed from the cache. --- Dapper/SqlMapper.cs | 6 +- .../MultiMapCachePoisoningTests.cs | 250 ++++++++++++++++++ 2 files changed, 254 insertions(+), 2 deletions(-) create mode 100644 tests/Dapper.Tests/MultiMapCachePoisoningTests.cs diff --git a/Dapper/SqlMapper.cs b/Dapper/SqlMapper.cs index 2fa0e72b7..56e931c71 100644 --- a/Dapper/SqlMapper.cs +++ b/Dapper/SqlMapper.cs @@ -1863,11 +1863,13 @@ private static CacheInfo GetCacheInfo(Identity identity, object? exampleParamete if (identity.ParametersType is not null) { Action reader; - if (exampleParameters is IDynamicParameters) + if (exampleParameters is IDynamicParameters + || (exampleParameters is null && typeof(IDynamicParameters).IsAssignableFrom(identity.ParametersType))) { reader = (cmd, obj) => ((IDynamicParameters)obj!).AddParameters(cmd, identity); } - else if (exampleParameters is IEnumerable>) + else if (exampleParameters is IEnumerable> + || (exampleParameters is null && typeof(IEnumerable>).IsAssignableFrom(identity.ParametersType))) { reader = (cmd, obj) => { diff --git a/tests/Dapper.Tests/MultiMapCachePoisoningTests.cs b/tests/Dapper.Tests/MultiMapCachePoisoningTests.cs new file mode 100644 index 000000000..01a78f734 --- /dev/null +++ b/tests/Dapper.Tests/MultiMapCachePoisoningTests.cs @@ -0,0 +1,250 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Data; +using System.Data.Common; +using System.Linq; +using System.Threading.Tasks; +using Xunit; + +// the in-memory ADO fakes below intentionally use non-null signatures; base ADO nullability +// annotations vary across the net481/net8.0/net10.0 target frameworks +#pragma warning disable CS8765 + +namespace Dapper.Tests +{ + [Collection("MultiMapCachePoisoning")] + public sealed class MultiMapCachePoisoningTests + { + private sealed class Foo { public int Id { get; set; } } + private sealed class Bar { public int OtherId { get; set; } } + + private static readonly Type[] Types = { typeof(Foo), typeof(Bar) }; + private static Foo Map(object[] objs) => (Foo)objs[0]; + + // Regression tests for a multi-map cache-poisoning race: the async multi-map path + // re-derives its cache entry from a parameter-less command after setup, so if the entry is + // evicted in between, GetCacheInfo used to rebuild a reflection reader for the parameter + // type that bound nothing and skipped IN-expansion. Reused entries then sent every @pN + // unbound/unexpanded -> "Incorrect syntax near '@p0'" (list token) or "Must declare the + // scalar variable '@p0'" (scalar token). The tests hit this deterministically by purging + // the cache when the reader executes -- the exact window the eviction races into. + + // "Incorrect syntax near '@p0'" variant: an IN-list must still be expanded. + [Fact] + public async Task AsyncMultiMap_ListExpansion_SurvivesCacheEvictionMidQuery() + { + var (executed, _) = await RunReusedQueryAfterMidQueryEviction( + "select Id, OtherId from Whatever where Id in @ids", + () => new Dictionary { ["ids"] = new[] { 1, 2, 3 } }); + + Assert.NotNull(executed); + // pre-fix, the poisoned reflection reader leaves the raw token in place + Assert.DoesNotContain("in @ids", executed, StringComparison.OrdinalIgnoreCase); + // post-fix, the list is expanded to positional parameters + Assert.Contains("@ids1", executed!); + } + + // "Must declare the scalar variable '@p0'" variant: a scalar-only query must still bind its + // parameters (pre-fix the poisoned reader binds none, so the command executes with zero). + [Fact] + public async Task AsyncMultiMap_ScalarParameters_SurviveCacheEvictionMidQuery() + { + var (_, parameterCount) = await RunReusedQueryAfterMidQueryEviction( + "select Id, OtherId from Whatever where Id = @p0 and OtherId = @p1", + () => new Dictionary { ["p0"] = 1, ["p1"] = 2 }); + + // post-fix both parameters are bound; pre-fix this is 0 (the "Must declare" condition) + Assert.Equal(2, parameterCount); + } + + // A GridReader grid-read calls GetCacheInfo(ForGrid(type, index), null, ...); ForGrid + // carries the parameters-type, and for the first grid the identity is identical to a plain + // Query over the same SQL/params. On a cold cache the grid read therefore caches a + // reader derived from a null example under that shared identity, which a later Query + // reuses -- no eviction required. + [Fact] + public void GridReader_DoesNotPoisonParamReaderForColocatedQuery() + { + const string sql = "select Id, OtherId from Whatever where Id in @ids"; + static object NewParam() => new Dictionary { ["ids"] = new[] { 1, 2, 3 } }; + + SqlMapper.PurgeQueryCache(); + try + { + using (var conn = new FakeConnection()) + using (var grid = conn.QueryMultiple(sql, NewParam())) + { + grid.Read(); + } + + string? executed; + using (var conn = new FakeConnection()) + { + conn.Query(sql, NewParam()); + executed = conn.LastCommandText; + } + + Assert.NotNull(executed); + Assert.DoesNotContain("in @ids", executed, StringComparison.OrdinalIgnoreCase); + Assert.Contains("@ids1", executed!); + } + finally + { + SqlMapper.PurgeQueryCache(); + } + } + + // Runs one "poisoning" query that evicts its own cache entry mid-flight, then a second + // query over the same SQL, returning what the second query actually sent to the server. + private static async Task<(string? commandText, int parameterCount)> RunReusedQueryAfterMidQueryEviction( + string sql, Func newParam) + { + SqlMapper.PurgeQueryCache(); + try + { + // Poisoning pass: evict the freshly-cached entry the instant the reader executes, + // i.e. after parameter-setup but before the multi-map re-derivation. + using (var poison = new FakeConnection { OnExecute = SqlMapper.PurgeQueryCache }) + { + await poison.QueryAsync(sql, Types, (Func)Map, newParam(), splitOn: "OtherId"); + } + + // Assertion pass: no eviction; reuses whatever the poisoning pass left cached. + using var conn = new FakeConnection(); + await conn.QueryAsync(sql, Types, (Func)Map, newParam(), splitOn: "OtherId"); + return (conn.LastCommandText, conn.LastParameterCount); + } + finally + { + SqlMapper.PurgeQueryCache(); + } + } + + // minimal in-memory ADO provider (no database). ConnectionString is a fixed constant so + // both passes produce the same Dapper Identity (which keys on it) and share a cache entry. + private sealed class FakeConnection : DbConnection + { + public Action? OnExecute { get; set; } + public string? LastCommandText { get; private set; } + public int LastParameterCount { get; private set; } + internal void Executed(string commandText, int parameterCount) + { + LastCommandText = commandText; + LastParameterCount = parameterCount; + OnExecute?.Invoke(); + } + + public override string ConnectionString { get; set; } = "fake-poisonable"; + public override string Database => "fake"; + public override string DataSource => "fake"; + public override string ServerVersion => "1.0"; + public override ConnectionState State => ConnectionState.Open; + public override void ChangeDatabase(string databaseName) { } + public override void Close() { } + public override void Open() { } + protected override DbTransaction BeginDbTransaction(IsolationLevel il) => throw new NotSupportedException(); + protected override DbCommand CreateDbCommand() => new FakeCommand(this); + } + + private sealed class FakeCommand : DbCommand + { + private readonly FakeConnection _conn; + private readonly FakeParameterCollection _params = new(); + public FakeCommand(FakeConnection conn) { _conn = conn; DbConnection = conn; } + public override string CommandText { get; set; } = ""; + public override int CommandTimeout { get; set; } + public override CommandType CommandType { get; set; } + public override bool DesignTimeVisible { get; set; } + public override UpdateRowSource UpdatedRowSource { get; set; } + protected override DbConnection? DbConnection { get; set; } + protected override DbParameterCollection DbParameterCollection => _params; + protected override DbTransaction? DbTransaction { get; set; } + public override void Cancel() { } + public override int ExecuteNonQuery() => 0; + public override object? ExecuteScalar() => null; + public override void Prepare() { } + protected override DbParameter CreateDbParameter() => new FakeParameter(); + protected override DbDataReader ExecuteDbDataReader(CommandBehavior behavior) + { + // capture what would hit the server (SQL + bound parameter count) + fire the hook + _conn.Executed(CommandText, _params.Count); + return new SchemaReader(); + } + } + + private sealed class FakeParameter : DbParameter + { + public override DbType DbType { get; set; } + public override ParameterDirection Direction { get; set; } = ParameterDirection.Input; + public override bool IsNullable { get; set; } + public override string ParameterName { get; set; } = ""; + public override string SourceColumn { get; set; } = ""; + public override object? Value { get; set; } + public override bool SourceColumnNullMapping { get; set; } + public override int Size { get; set; } + public override void ResetDbType() { } + } + + private sealed class FakeParameterCollection : DbParameterCollection + { + private readonly List _list = new(); + public override int Count => _list.Count; + public override object SyncRoot { get; } = new(); + public override int Add(object value) { _list.Add(value); return _list.Count - 1; } + public override void AddRange(Array values) { foreach (var v in values) _list.Add(v); } + public override void Clear() => _list.Clear(); + public override bool Contains(object value) => _list.Contains(value); + public override bool Contains(string value) => _list.Any(p => ((DbParameter)p).ParameterName == value); + public override void CopyTo(Array array, int index) => ((ICollection)_list).CopyTo(array, index); + public override IEnumerator GetEnumerator() => _list.GetEnumerator(); + protected override DbParameter GetParameter(int index) => (DbParameter)_list[index]; + protected override DbParameter GetParameter(string name) => (DbParameter)_list.First(p => ((DbParameter)p).ParameterName == name); + public override int IndexOf(object value) => _list.IndexOf(value); + public override int IndexOf(string name) => _list.FindIndex(p => ((DbParameter)p).ParameterName == name); + public override void Insert(int index, object value) => _list.Insert(index, value); + public override void Remove(object value) => _list.Remove(value); + public override void RemoveAt(int index) => _list.RemoveAt(index); + public override void RemoveAt(string name) { var i = IndexOf(name); if (i >= 0) _list.RemoveAt(i); } + protected override void SetParameter(int index, DbParameter value) => _list[index] = value; + protected override void SetParameter(string name, DbParameter value) { var i = IndexOf(name); if (i >= 0) _list[i] = value; else _list.Add(value); } + } + + // reports the column schema (so multi-map deserializers can be generated) but returns no rows + private sealed class SchemaReader : DbDataReader + { + private static readonly string[] Cols = { "Id", "OtherId" }; + public override int FieldCount => Cols.Length; + public override string GetName(int ordinal) => Cols[ordinal]; + public override Type GetFieldType(int ordinal) => typeof(int); + public override string GetDataTypeName(int ordinal) => "int"; + public override int GetOrdinal(string name) => Array.IndexOf(Cols, name); + public override bool Read() => false; + public override bool NextResult() => false; + public override bool HasRows => false; + public override int Depth => 0; + public override bool IsClosed => false; + public override int RecordsAffected => 0; + public override object this[int ordinal] => throw new NotSupportedException(); + public override object this[string name] => throw new NotSupportedException(); + public override bool GetBoolean(int ordinal) => default; + public override byte GetByte(int ordinal) => default; + public override long GetBytes(int ordinal, long dataOffset, byte[]? buffer, int bufferOffset, int length) => 0; + public override char GetChar(int ordinal) => default; + public override long GetChars(int ordinal, long dataOffset, char[]? buffer, int bufferOffset, int length) => 0; + public override DateTime GetDateTime(int ordinal) => default; + public override decimal GetDecimal(int ordinal) => default; + public override double GetDouble(int ordinal) => default; + public override float GetFloat(int ordinal) => default; + public override Guid GetGuid(int ordinal) => default; + public override short GetInt16(int ordinal) => default; + public override int GetInt32(int ordinal) => default; + public override long GetInt64(int ordinal) => default; + public override string GetString(int ordinal) => ""; + public override object GetValue(int ordinal) => throw new NotSupportedException(); + public override int GetValues(object[] values) => 0; + public override bool IsDBNull(int ordinal) => true; + public override IEnumerator GetEnumerator() => Array.Empty().GetEnumerator(); + } + } +}