Potential buffer over-read (yielding exception) in Npgsql code when reading arrays with ArrayNullabilityMode.PerInstance

[John-Paul-R]

Preface: It is entirely possible that this is a bug in Npgsql code. In fact, the fix I'm currently using for this (see the linked repro repo) involves patching Npgsql. I have not yet been able to reproduce the issue with anything other than Dapper for now, so I am opening an issue here first. Direct elsewhere if you deem it appropriate!

Error case

With Npgsql in ArrayNullabililityMode.PerInstance, is is possible to encounter a buffer over-read when the metadata block containing nullability information (seemingly an 8-byte long block) falls less than 8 bytes from the end of Npgsql's working buffer. In this case, there will be a buffer-over-read instead of reading new data from the underlying source into the buffer.

The exceptionsl Npgsql code PolymorphicArrayConverter<T>'s Read method leading with 2 unchecked ReadInt32 calls, which may read past the boundary of the working buffer.

permalink: https://github.com/npgsql/npgsql/blob/058894067d33229fbef2f3bcafbfa75858fc60fb/src/Npgsql/Internal/Converters/ArrayConverter.cs#L631-L639

It is unclear to me if it is the caller's responsibility to advance the reader before calling into this converter. If it is, this may be a Dapper bug. If it is not, this is undoubtedly an Npgsql bug.

---

You can view a full repro with execution steps here.

Example exception

Unhandled exception. System.Data.DataException: Error parsing column 0 (Set04=<null>)
 ---> System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at Npgsql.Internal.PgReader.ReadInt32()
   at Npgsql.Internal.Converters.PolymorphicArrayConverter`1.Read(PgReader reader)
   at Npgsql.Internal.PgStreamingConverter`1.ReadAsObject(Boolean async, PgReader reader, CancellationToken cancellationToken)
   at Npgsql.Internal.PgConverter.ReadAsObject(PgReader reader)
   at Npgsql.NpgsqlDataReader.GetValue(Int32 ordinal)
   at Npgsql.NpgsqlDataReader.get_Item(Int32 ordinal)
   at Deserialize2159941e-4fc4-4688-83e9-f9769e282037(DbDataReader)
   --- End of inner exception stack trace ---
   at Dapper.SqlMapper.ThrowDataException(Exception ex, Int32 index, IDataReader reader, Object value) in /_/Dapper/SqlMapper.cs:line 3928
   at Deserialize2159941e-4fc4-4688-83e9-f9769e282037(DbDataReader)
   at Dapper.SqlMapper.QueryImpl[T](IDbConnection cnn, CommandDefinition command, Type effectiveType)+MoveNext()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Dapper.SqlMapper.Query[T](IDbConnection cnn, String sql, Object param, IDbTransaction transaction, Boolean buffered, Nullable`1 commandTimeout, Nullable`1 commandType)
   at Program.<Main>$(String[] args)

[John-Paul-R]

Upon doing a bit more digging into the Npgsql source, I believe this is indeed a bug in their PolymorphicArrayConverter<T> implementation. The other converters I've seen specify the number of bytes that must be read-ahead in a CanConvert that looks something like this (this example is for Guids):

public override bool CanConvert(DataFormat format, out BufferRequirements bufferRequirements)
{
    bufferRequirements = BufferRequirements.CreateFixedSize(16 * sizeof(byte));
    return format is DataFormat.Binary;
}

PolymorphicArrayConverter<T> does not override CanConvert, despite unconditionally reading at least 8 bytes any time it is used.

Opening an issue in Npgsql.

[John-Paul-R]

closing as suspected Npgsql issue.