From 408a009e16655aeadb9e99e80ee027730330aae1 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Wed, 10 Jan 2024 22:14:35 -0500 Subject: [PATCH 1/3] PhysicalConnection: Better shutdown handling re: null refs This adds a bit of null ref handling (few ifs). Fixes #2576. --- src/StackExchange.Redis/PhysicalConnection.cs | 104 +++++++++++------- 1 file changed, 63 insertions(+), 41 deletions(-) diff --git a/src/StackExchange.Redis/PhysicalConnection.cs b/src/StackExchange.Redis/PhysicalConnection.cs index 2ed2a1819..623898059 100644 --- a/src/StackExchange.Redis/PhysicalConnection.cs +++ b/src/StackExchange.Redis/PhysicalConnection.cs @@ -790,39 +790,44 @@ internal void Write(in RedisKey key) var val = key.KeyValue; if (val is string s) { - WriteUnifiedPrefixedString(_ioPipe!.Output, key.KeyPrefix, s); + WriteUnifiedPrefixedString(_ioPipe?.Output, key.KeyPrefix, s); } else { - WriteUnifiedPrefixedBlob(_ioPipe!.Output, key.KeyPrefix, (byte[]?)val); + WriteUnifiedPrefixedBlob(_ioPipe?.Output, key.KeyPrefix, (byte[]?)val); } } internal void Write(in RedisChannel channel) - => WriteUnifiedPrefixedBlob(_ioPipe!.Output, ChannelPrefix, channel.Value); + => WriteUnifiedPrefixedBlob(_ioPipe?.Output, ChannelPrefix, channel.Value); [MethodImpl(MethodImplOptions.AggressiveInlining)] internal void WriteBulkString(in RedisValue value) - => WriteBulkString(value, _ioPipe!.Output); - internal static void WriteBulkString(in RedisValue value, PipeWriter output) + => WriteBulkString(value, _ioPipe?.Output); + internal static void WriteBulkString(in RedisValue value, PipeWriter? maybeNullWriter) { + if (maybeNullWriter is not PipeWriter writer) + { + return; // Prevent null refs during disposal + } + switch (value.Type) { case RedisValue.StorageType.Null: - WriteUnifiedBlob(output, (byte[]?)null); + WriteUnifiedBlob(writer, (byte[]?)null); break; case RedisValue.StorageType.Int64: - WriteUnifiedInt64(output, value.OverlappedValueInt64); + WriteUnifiedInt64(writer, value.OverlappedValueInt64); break; case RedisValue.StorageType.UInt64: - WriteUnifiedUInt64(output, value.OverlappedValueUInt64); + WriteUnifiedUInt64(writer, value.OverlappedValueUInt64); break; case RedisValue.StorageType.Double: // use string case RedisValue.StorageType.String: - WriteUnifiedPrefixedString(output, null, (string?)value); + WriteUnifiedPrefixedString(writer, null, (string?)value); break; case RedisValue.StorageType.Raw: - WriteUnifiedSpan(output, ((ReadOnlyMemory)value).Span); + WriteUnifiedSpan(writer, ((ReadOnlyMemory)value).Span); break; default: throw new InvalidOperationException($"Unexpected {value.Type} value: '{value}'"); @@ -833,6 +838,11 @@ internal static void WriteBulkString(in RedisValue value, PipeWriter output) internal void WriteHeader(RedisCommand command, int arguments, CommandBytes commandBytes = default) { + if (_ioPipe?.Output is not PipeWriter writer) + { + return; // Prevent null refs during disposal + } + var bridge = BridgeCouldBeNull ?? throw new ObjectDisposedException(ToString()); if (command == RedisCommand.UNKNOWN) @@ -856,14 +866,14 @@ internal void WriteHeader(RedisCommand command, int arguments, CommandBytes comm // *{argCount}\r\n = 3 + MaxInt32TextLen // ${cmd-len}\r\n = 3 + MaxInt32TextLen // {cmd}\r\n = 2 + commandBytes.Length - var span = _ioPipe!.Output.GetSpan(commandBytes.Length + 8 + Format.MaxInt32TextLen + Format.MaxInt32TextLen); + var span = writer.GetSpan(commandBytes.Length + 8 + Format.MaxInt32TextLen + Format.MaxInt32TextLen); span[0] = (byte)'*'; int offset = WriteRaw(span, arguments + 1, offset: 1); offset = AppendToSpanCommand(span, commandBytes, offset: offset); - _ioPipe.Output.Advance(offset); + writer.Advance(offset); } internal void RecordQuit() // don't blame redis if we fired the first shot @@ -1116,38 +1126,40 @@ private static int AppendToSpan(Span span, ReadOnlySpan value, int o internal void WriteSha1AsHex(byte[] value) { - var writer = _ioPipe!.Output; - if (value == null) + if (_ioPipe?.Output is PipeWriter writer) { - writer.Write(NullBulkString.Span); - } - else if (value.Length == ResultProcessor.ScriptLoadProcessor.Sha1HashLength) - { - // $40\r\n = 5 - // {40 bytes}\r\n = 42 + if (value == null) + { + writer.Write(NullBulkString.Span); + } + else if (value.Length == ResultProcessor.ScriptLoadProcessor.Sha1HashLength) + { + // $40\r\n = 5 + // {40 bytes}\r\n = 42 - var span = writer.GetSpan(47); - span[0] = (byte)'$'; - span[1] = (byte)'4'; - span[2] = (byte)'0'; - span[3] = (byte)'\r'; - span[4] = (byte)'\n'; + var span = writer.GetSpan(47); + span[0] = (byte)'$'; + span[1] = (byte)'4'; + span[2] = (byte)'0'; + span[3] = (byte)'\r'; + span[4] = (byte)'\n'; - int offset = 5; - for (int i = 0; i < value.Length; i++) + int offset = 5; + for (int i = 0; i < value.Length; i++) + { + var b = value[i]; + span[offset++] = ToHexNibble(b >> 4); + span[offset++] = ToHexNibble(b & 15); + } + span[offset++] = (byte)'\r'; + span[offset++] = (byte)'\n'; + + writer.Advance(offset); + } + else { - var b = value[i]; - span[offset++] = ToHexNibble(b >> 4); - span[offset++] = ToHexNibble(b & 15); + throw new InvalidOperationException("Invalid SHA1 length: " + value.Length); } - span[offset++] = (byte)'\r'; - span[offset++] = (byte)'\n'; - - writer.Advance(offset); - } - else - { - throw new InvalidOperationException("Invalid SHA1 length: " + value.Length); } } @@ -1156,8 +1168,13 @@ internal static byte ToHexNibble(int value) return value < 10 ? (byte)('0' + value) : (byte)('a' - 10 + value); } - internal static void WriteUnifiedPrefixedString(PipeWriter writer, byte[]? prefix, string? value) + internal static void WriteUnifiedPrefixedString(PipeWriter? maybeNullWriter, byte[]? prefix, string? value) { + if (maybeNullWriter is not PipeWriter writer) + { + return; // Prevent null refs during disposal + } + if (value == null) { // special case @@ -1259,8 +1276,13 @@ internal static unsafe void WriteRaw(PipeWriter writer, string value, int expect } } - private static void WriteUnifiedPrefixedBlob(PipeWriter writer, byte[]? prefix, byte[]? value) + private static void WriteUnifiedPrefixedBlob(PipeWriter? maybeNullWriter, byte[]? prefix, byte[]? value) { + if (maybeNullWriter is not PipeWriter writer) + { + return; // Prevent null refs during disposal + } + // ${total-len}\r\n // {prefix}{value}\r\n if (prefix == null || prefix.Length == 0 || value == null) From af70390bf01906f2bd77e2e6678e2147368b6743 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Wed, 10 Jan 2024 22:17:21 -0500 Subject: [PATCH 2/3] Align WriteSha1AsHex --- src/StackExchange.Redis/PhysicalConnection.cs | 58 ++++++++++--------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/src/StackExchange.Redis/PhysicalConnection.cs b/src/StackExchange.Redis/PhysicalConnection.cs index 623898059..70bc33c0d 100644 --- a/src/StackExchange.Redis/PhysicalConnection.cs +++ b/src/StackExchange.Redis/PhysicalConnection.cs @@ -1126,40 +1126,42 @@ private static int AppendToSpan(Span span, ReadOnlySpan value, int o internal void WriteSha1AsHex(byte[] value) { - if (_ioPipe?.Output is PipeWriter writer) + if (_ioPipe?.Output is not PipeWriter writer) { - if (value == null) - { - writer.Write(NullBulkString.Span); - } - else if (value.Length == ResultProcessor.ScriptLoadProcessor.Sha1HashLength) - { - // $40\r\n = 5 - // {40 bytes}\r\n = 42 + return; // Prevent null refs during disposal + } - var span = writer.GetSpan(47); - span[0] = (byte)'$'; - span[1] = (byte)'4'; - span[2] = (byte)'0'; - span[3] = (byte)'\r'; - span[4] = (byte)'\n'; + if (value == null) + { + writer.Write(NullBulkString.Span); + } + else if (value.Length == ResultProcessor.ScriptLoadProcessor.Sha1HashLength) + { + // $40\r\n = 5 + // {40 bytes}\r\n = 42 - int offset = 5; - for (int i = 0; i < value.Length; i++) - { - var b = value[i]; - span[offset++] = ToHexNibble(b >> 4); - span[offset++] = ToHexNibble(b & 15); - } - span[offset++] = (byte)'\r'; - span[offset++] = (byte)'\n'; + var span = writer.GetSpan(47); + span[0] = (byte)'$'; + span[1] = (byte)'4'; + span[2] = (byte)'0'; + span[3] = (byte)'\r'; + span[4] = (byte)'\n'; - writer.Advance(offset); - } - else + int offset = 5; + for (int i = 0; i < value.Length; i++) { - throw new InvalidOperationException("Invalid SHA1 length: " + value.Length); + var b = value[i]; + span[offset++] = ToHexNibble(b >> 4); + span[offset++] = ToHexNibble(b & 15); } + span[offset++] = (byte)'\r'; + span[offset++] = (byte)'\n'; + + writer.Advance(offset); + } + else + { + throw new InvalidOperationException("Invalid SHA1 length: " + value.Length); } } From 32de975c946851f60116110ab3a6f8575595f4b8 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Wed, 10 Jan 2024 22:21:27 -0500 Subject: [PATCH 3/3] Add release notes --- docs/ReleaseNotes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md index 3941c9c85..ad31188d2 100644 --- a/docs/ReleaseNotes.md +++ b/docs/ReleaseNotes.md @@ -10,6 +10,7 @@ Current package versions: - Fix [#2619](https://github.com/StackExchange/StackExchange.Redis/issues/2619): Type-forward `IsExternalInit` to support down-level TFMs ([#2621 by mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/2621)) - `InternalsVisibleTo` `PublicKey` enhancements([#2623 by WeihanLi](https://github.com/StackExchange/StackExchange.Redis/pull/2623)) +- Fix [#2576](https://github.com/StackExchange/StackExchange.Redis/issues/2576): Prevent `NullReferenceException` during shutdown of connections ([#2629 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2629)) ## 2.7.10