GetFieldValue<T?> returns broken Nullable<T> in npgsql 9.*

[kolonist]

Hello!

I use npgsql v9 and I get strange behavior when getting NULL value from DB bigint column and reading it with reader.GetFieldValue<long?>(). I sometimes get nullable variable with HasValue == false but Value is some random value. Normally, I can't get it by just myVar.Value because .NET Nullable struct denies it:

but if I use myVar.GetValueOrDefault() then I directly get private field value without any validations:

so if there is some value inside then I just get it as it is default value of myVar.

.NET normally doesn't allow me to create this kind of broken Nullable, so it could be done only in unsafe operations or with reflection.

I believe that it's not bug of MS .NET because I have this behavior in npgsql 9.0.0 and newer but I don't have this issue in versions 8.0.7 or older.

Reproduction:

I used MacOS running on M2 Arm CPU, but it it also can be reproduced in MS docker image with sdk:9.0. For Postgres I use postgres:14-alpine image.

1. Create some DB, you don't even need to create any tables
2. Write the following method to perform queries in DB:

    public async Task<IReadOnlyCollection<long?>> GetData()
    {
        await using var connection = new NpgsqlConnection(_connectionString);
        await connection.OpenAsync();
   
        // just take NULL value from DB
        await using var command = new NpgsqlCommand("SELECT NULL::bigint AS value1;", connection);
   
        await using var reader = await command.ExecuteReaderAsync();
   
        var data = new List<long?>();
        while (await reader.ReadAsync())
        {
            data.Add(reader.GetFieldValue<long?>(0));
        }
   
        return data;
    }

3. And code to call it:

    public async Task Success()
    {
        var repository = new Repository();
   
        var i = 0;
        while (true)
        {
            var result = await repository.GetData();
   
            foreach (var data in result)
            {
                var d = data.GetValueOrDefault();
                outputHelper.WriteLine("{0}: `{1}` / {2}, `{3}`", i, data, d, data.HasValue);
   
                if (d != 0)
                {
                    outputHelper.WriteLine($"Unexpected value: {d} / {d:X} / {d:b8}");
                    throw new Exception("We are here");
                }
            }
   
            i++;
        }
    }

   Note that I call that query in infinite loop.
4. The result will be the following:

   0: `` / 0, `False`
   1: `` / 0, `False`
   2: `` / 0, `False`
   3: `` / 0, `False`
   
   ...
   
   227: `` / 0, `False`
   228: `` / 0, `False`
   229: `` / 0, `False`
   230: `` / 4403988152, `False`
   Unexpected value: 4403988152 / 1067F86B8 / 100000110011111111000011010111000
   
   System.Exception "We are here"
   

Count of iterations before error is always different but it is always under 1000 (even under 300). Unexpected value is always different too, and there is no order except that their hex representation always ends with B8.

Interesting is that when I use GetFieldValueAsync() I don't get this error. When I run the code under debugger I also can't get the error.

And that make me believe that this error connected to unsafe casting of BD data to .NET nullable value types in some kind of race condition when internal value of nullable value type can be set to random value (maybe memory address itself or some old value in process memory).

Please could you take a look at this issue.

[vonzshik]

The fact that it's not stable (doesn't repro under debugger and iteration count is different) leads me to believe that it's either codegen issue or some sort of race condition (as you mentioned above). While I'll try to investigate, a few questions:

1. What version of .NET are you using?
2. When you tried Npgsql 8.0.7, were you using the exact same version of .NET runtime?

[kolonist]

1. What version of .NET are you using?
   SDK: 9.0.100, Runtime: Microsoft.NETCore.App 9.0.0

2. When you tried Npgsql 8.0.7, were you using the exact same version of .NET runtime?
   Yes, I changed only value in <PackageReference Include="Npgsql" Version="__HERE__"/>

[vonzshik]

using Npgsql;

var i = 0;
while (true)
{
    var result = await GetData();

    foreach (var data in result)
    {
        var d = data.GetValueOrDefault();
        Console.WriteLine("{0}: `{1}` / {2}, `{3}`", i, data, d, data.HasValue);

        if (d != 0)
        {
            Console.WriteLine($"Unexpected value: {d} / {d:X} / {d:b8}");
            throw new Exception("We are here");
        }
    }

    i++;
}

static async Task<IReadOnlyCollection<long?>> GetData()
{
    const string _connectionString = "Host=localhost; Database=db; User ID=postgres; Password=pass; Pooling=true";

    await using var connection = new NpgsqlConnection(_connectionString);
    await connection.OpenAsync();

    // just take NULL value from DB
    await using var command = new NpgsqlCommand("SELECT NULL::bigint AS value1;", connection);

    await using var reader = await command.ExecuteReaderAsync();

    var data = new List<long?>();
    while (await reader.ReadAsync())
    {
        data.Add(reader.GetFieldValue<long?>(0));
    }

    return data;
}

Tried it locally on windows. 300k cycles in, no failures in sight.

I can try to also run it in docker, though I have some suspicions that it might be related to M2 (and ARM in general) rather than anything else. @NinoFloris maybe you have some other ideas? I do not really have an M2 anywhere nearby so can't really test it out...

[kolonist]

I can't check it on Windows but we have it on Linux

[roji]

I can confirm this happens on my MacBook Pro M2 Max:

15519: `` / 0, `False`
15520: `` / 0, `False`
15521: `` / 0, `False`
15522: `` / 0, `False`
15523: `` / 4378927112, `False`
Unexpected value: 4378927112 / 105012008 / 100000101000000010010000000001000
Unhandled exception. System.Exception: We are here
   at Program.<Main>$(String[] args) in /Users/roji/projects/test/Program.cs:line 22
   at Program.<Main>(String[] args)

@vonzshik I wonder if this can be repro'd without Npgsql? Though even with Npgsql, the repro above looks like enough to take to the runtime folks (unless there's something funky we're doing inside Npgsql, of course).

[vonzshik]

@vonzshik I wonder if this can be repro'd without Npgsql? Though even with Npgsql, the repro above looks like enough to take to the runtime folks (unless there's something funky we're doing inside Npgsql, of course).

Pretty sure it can, just wanted to get a confirmation that it is indeed due to ARM/M2.

Maybe calling this code is enough?

npgsql/src/Npgsql/NpgsqlDataReader.cs

Lines 2083 to 2095 in 562d395

	[MethodImpl(MethodImplOptions.NoInlining)]
	T DbNullValueOrThrow<T>(int ordinal)
	{
	// When T is a Nullable<T> (and only in that case), we support returning null
	if (default(T) is null && typeof(T).IsValueType)
	return default!;
	if (typeof(T) == typeof(object))
	return (T)(object)DBNull.Value;
	ThrowHelper.ThrowInvalidCastException_NoValue(RowDescription![ordinal]);
	return default;
	}

[vonzshik]

Maybe something like this?

using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

var i = 0;
while (true)
{
    var result = await GetData();

    foreach (var data in result)
    {
        var d = data.GetValueOrDefault();
        Console.WriteLine("{0}: `{1}` / {2}, `{3}`", i, data, d, data.HasValue);

        if (d != 0)
        {
            Console.WriteLine($"Unexpected value: {d} / {d:X} / {d:b8}");
            throw new Exception("We are here");
        }
    }

    i++;
}

static async Task<IReadOnlyCollection<long?>> GetData()
{
    var data = new List<long?>();

    for (var i = 0; i < 10; i++)
    {
        data.Add(DbNullValueOrThrow<long?>(0));
    }

    return data;
}

[MethodImpl(MethodImplOptions.NoInlining)] 
static T DbNullValueOrThrow<T>(int ordinal) 
{ 
    // When T is a Nullable<T> (and only in that case), we support returning null 
    if (default(T) is null && typeof(T).IsValueType) 
        return default!; 

    if (typeof(T) == typeof(object)) 
        return (T)(object)DBNull.Value;

    ThrowInvalidCastException_NoValue(ordinal);

    return default; 
}

[DoesNotReturn]
static void ThrowInvalidCastException_NoValue(int ordinal) =>
    throw new InvalidCastException($"Field is {ordinal} null.");

[kolonist]

Probably it's not:

[vonzshik]

Welp, the easiest way to repro goes out of the window, I suppose. I can't really make a solid repro without having an actual machine to experiment with, so the only thing left is to try to raise this with runtime folks as @roji suggested?

[NinoFloris]

Can't get it to reproduce with my m1 max :)

EDIT: I wasn't running a release build, can repro now.

[NinoFloris]

unless there's something funky we're doing inside Npgsql, of course

Nullable is a very simple path, we essentially let the runtime handle the conversion if we have a value.

https://github.com/npgsql/npgsql/blob/562d3954042ae005fef0f9bac918786a51fe3cc9/src/Npgsql/Internal/Converters/NullableConverter.cs

If we have a NULL we run the code @vonzshik referenced: returning default for the given nullable T.

[vonzshik]

A random thought. Could it be due to SkipLocalsInit we have?

[NinoFloris]

I was thinking in the same direction, then I concluded this kind of code shouldn't be affected by SkipLocalsInit in C#. But seeing how we may be looking at broken codegen I tried anyway. It stops reproducing with SkipLocalsInit removed from Npgsql.

The minimal repro seems to be:

using System;
using System.Runtime.CompilerServices;

// Necessary
[module: SkipLocalsInit]

static class Program
{
    public static void Main()
    {
        var i = 0;
        while (true)
        {
            var data = FaultyDefaultNullable<long?>();
            var d = data.GetValueOrDefault();
            // We need to dirty the stack and make sure this won't be optimized out by doing GC.Keepalive.
            var iObject = (object)i;
            var dataObject = (object?)data;
            var dObject = (object?)d;
            var hasValueObject = (object?)data.HasValue;
            // Alternatively
            // Console.WriteLine("{0}: `{1}` / {2}, `{3}`", i, data, d, data.HasValue);

            if (d != 0)
            {
                Console.WriteLine($"Unexpected value: {d} / {d:X} / {d:b8}");
                throw new Exception("We are here");
            }

            GC.KeepAlive(iObject);
            GC.KeepAlive(dataObject);
            GC.KeepAlive(dObject);
            GC.KeepAlive(hasValueObject);

            i++;
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static T FaultyDefaultNullable<T>()
    {
        // When T is a Nullable<T> (and only in that case), we support returning null
        if (default(T) is null && typeof(T).IsValueType)
            return default!;

        throw new InvalidOperationException("Not nullable");
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static T OkDefaultNullable<T>() => default!;
}

This reproduces in a release build from 9.0 onwards, including 10.0 preview 1.

export DOTNET_JitDisasm="Main FaultyDefaultNullable OkDefaultNullable"

; Assembly listing for method Program:FaultyDefaultNullable[System.Nullable`1[long]]():System.Nullable`1[long] (Tier1)
; Emitting BLENDED_CODE for generic ARM64 - Apple
; Tier1 code
; optimized code
; optimized using Synthesized PGO
; fp based frame
; partially interruptible
; with Synthesized PGO: fgCalledCount is 100
; No PGO data

G_M000_IG01:                ;; offset=0x0000
            stp     fp, lr, [sp, #-0x20]!
            mov     fp, sp
 
G_M000_IG02:                ;; offset=0x0008
            mov     w0, wzr
            ldr     x1, [fp, #0x18]
 
G_M000_IG03:                ;; offset=0x0010
            ldp     fp, lr, [sp], #0x20
            ret     lr
 
; Total bytes of code 24

; Assembly listing for method Program:OkDefaultNullable[System.Nullable`1[long]]():System.Nullable`1[long] (Tier1)
; Emitting BLENDED_CODE for generic ARM64 - Apple
; Tier1 code
; optimized code
; optimized using Synthesized PGO
; fp based frame
; partially interruptible
; with Synthesized PGO: fgCalledCount is 100
; No PGO data

G_M000_IG01:                ;; offset=0x0000
            stp     fp, lr, [sp, #-0x10]!
            mov     fp, sp
 
G_M000_IG02:                ;; offset=0x0008
            mov     w0, wzr
            mov     x1, xzr
 
G_M000_IG03:                ;; offset=0x0010
            ldp     fp, lr, [sp], #0x10
            ret     lr
 
; Total bytes of code 24

Main codegen

; Assembly listing for method Program:Main() (Tier1-OSR) ; Emitting BLENDED_CODE for generic ARM64 - Apple ; Tier1-OSR code ; OSR variant for entry point 0x2 ; optimized code ; optimized using Dynamic PGO ; fp based frame ; partially interruptible ; with Dynamic PGO: fgCalledCount is 10 ; 9 inlinees with PGO data; 5 single block inlinees; 3 inlinees without PGO data

G_M000_IG01: ;; offset=0x0000
stp fp, lr, [sp, #-0x60]!
stp x19, x20, [sp, #0x18]
stp x21, x22, [sp, #0x28]
stp x23, x24, [sp, #0x38]
stp x25, x26, [sp, #0x48]
str x27, [sp, #0x58]
mov fp, sp

G_M000_IG02: ;; offset=0x001C
movz x19, #0xE6A8
movk x19, #458 LSL #16
movk x19, #1 LSL #32
movz x20, #0x65B0
movk x20, #736 LSL #16
movk x20, #1 LSL #32
movz x21, #0xF8C8
movk x21, #730 LSL #16
movk x21, #1 LSL #32
movz x22, #0xA1D8
movk x22, #458 LSL #16
movk x22, #1 LSL #32

G_M000_IG03: ;; offset=0x004C
movz x0, #0x6AD8
movk x0, #729 LSL #16
movk x0, #1 LSL #32
ldr x0, [x0]
blr x0
stp x0, x1, [fp, #0x128]
ldr x23, [fp, #0x130]
mov x0, x19
bl CORINFO_HELP_NEWSFAST
mov x24, x0
ldr w25, [fp, #0x13C]
str w25, [x24, #0x08]
add x1, fp, #296
mov x0, x20
bl CORINFO_HELP_BOX_NULLABLE
mov x26, x0
mov x0, x21
bl CORINFO_HELP_NEWSFAST
mov x27, x0
str x23, [x27, #0x08]
mov x0, x22
bl CORINFO_HELP_NEWSFAST
ldrb w1, [fp, #0x128]
strb w1, [x0, #0x08]
cbnz x23, G_M000_IG04
add w25, w25, #1
str w25, [fp, #0x13C]
b G_M000_IG03

G_M000_IG04: ;; offset=0x00BC
add x0, fp, #224
mov w1, #24
mov w2, #3
movz x3, #0x6B50
movk x3, #729 LSL #16
movk x3, #1 LSL #32
ldr x3, [x3]
blr x3
ldr w0, [fp, #0xF0]
ldr w1, [fp, #0x100]
cmp w0, w1
bhi G_M000_IG14
ldr x1, [fp, #0xF8]
ubfiz x2, x0, #1, #32
add x1, x1, x2
ldr w2, [fp, #0x100]
sub w0, w2, w0
cmp w0, #18
blo G_M000_IG11

G_M000_IG05: ;; offset=0x0108
ldr q16, [@rwd00]
ldr q17, [@RWD16]
stp q16, q17, [x1]
movz w0, #58
movk w0, #32 LSL #16
str w0, [x1, #0x20]
ldr w0, [fp, #0xF0]
add w0, w0, #18
str w0, [fp, #0xF0]

G_M000_IG06: ;; offset=0x012C
add x0, fp, #224
mov x1, x23
movz x2, #0x6AF0
movk x2, #729 LSL #16
movk x2, #1 LSL #32
ldr x2, [x2]
blr x2
ldr w0, [fp, #0xF0]
ldr w1, [fp, #0x100]
cmp w0, w1
bhi G_M000_IG14
ldr x1, [fp, #0xF8]
ubfiz x2, x0, #1, #32
add x1, x1, x2
ldr w2, [fp, #0x100]
sub w0, w2, w0
cmp w0, #3
blo G_M000_IG12

G_M000_IG07: ;; offset=0x0174
movz x19, #0xC3E8
movk x19, #289 LSL #16
movk x19, #1 LSL #32
movz w0, #32
movk w0, #47 LSL #16
str w0, [x1]
mov w0, #32
strh w0, [x1, #0x04]
ldr w0, [fp, #0xF0]
add w0, w0, #3
str w0, [fp, #0xF0]

G_M000_IG08: ;; offset=0x01A0
add x0, fp, #224
mov x1, x23
movz x2, #0xC408
movk x2, #289 LSL #16
movk x2, #1 LSL #32
movz x3, #0x6B08
movk x3, #729 LSL #16
movk x3, #1 LSL #32
ldr x3, [x3]
blr x3
ldr w0, [fp, #0xF0]
ldr w1, [fp, #0x100]
cmp w0, w1
bhi G_M000_IG14
ldr x1, [fp, #0xF8]
ubfiz x2, x0, #1, #32
add x1, x1, x2
ldr w2, [fp, #0x100]
sub w0, w2, w0
cmp w0, #3
blo G_M000_IG13

G_M000_IG09: ;; offset=0x01F4
movz w0, #32
movk w0, #47 LSL #16
str w0, [x1]
mov w0, #32
strh w0, [x1, #0x04]
ldr w0, [fp, #0xF0]
add w0, w0, #3
str w0, [fp, #0xF0]

G_M000_IG10: ;; offset=0x0214
mov w0, #51
movz x1, #0x1488
movk x1, #736 LSL #16
movk x1, #1 LSL #32
bl CORINFO_HELP_STRCNS
mov x2, x0
add x0, fp, #224
mov x1, x23
movz x3, #0x6B08
movk x3, #729 LSL #16
movk x3, #1 LSL #32
ldr x3, [x3]
blr x3
add x0, fp, #224
movz x1, #0x6B80
movk x1, #729 LSL #16
movk x1, #1 LSL #32
ldr x1, [x1]
blr x1
movz x1, #0x6B98
movk x1, #729 LSL #16
movk x1, #1 LSL #32
ldr x1, [x1]
blr x1
movz x0, #0x99C0
movk x0, #733 LSL #16
movk x0, #1 LSL #32
bl CORINFO_HELP_NEWSFAST
mov x19, x0
mov w0, #57
movz x1, #0x1488
movk x1, #736 LSL #16
movk x1, #1 LSL #32
bl CORINFO_HELP_STRCNS
mov x1, x0
mov x0, x19
movz x2, #0x6BB0
movk x2, #729 LSL #16
movk x2, #1 LSL #32
ldr x2, [x2]
blr x2
mov x0, x19
bl CORINFO_HELP_THROW
brk #0

G_M000_IG11: ;; offset=0x02C4
add x0, fp, #224
movz x1, #0xC3A8
movk x1, #289 LSL #16
movk x1, #1 LSL #32
movz x2, #0x6CA0
movk x2, #729 LSL #16
movk x2, #1 LSL #32
ldr x2, [x2]
blr x2
b G_M000_IG06

G_M000_IG12: ;; offset=0x02EC
add x0, fp, #224
movz x19, #0xC3E8
movk x19, #289 LSL #16
movk x19, #1 LSL #32
mov x1, x19
movz x2, #0x6CA0
movk x2, #729 LSL #16
movk x2, #1 LSL #32
ldr x2, [x2]
blr x2
b G_M000_IG08

G_M000_IG13: ;; offset=0x0318
add x0, fp, #224
mov x1, x19
movz x2, #0x6CA0
movk x2, #729 LSL #16
movk x2, #1 LSL #32
ldr x2, [x2]
blr x2
b G_M000_IG10

G_M000_IG14: ;; offset=0x0338
movz x0, #0x6CB8
movk x0, #729 LSL #16
movk x0, #1 LSL #32
ldr x0, [x0]
blr x0
brk #0

RWD00 dq 00780065006E0055h, 0074006300650070h
RWD16 dq 0076002000640065h, 00650075006C0061h

; Total bytes of code 848

[roji]

Great work guys, I wish I had time to dive into things like this...

Close as external?