fix: validate byte array length in RegisterFunctions converters (#38)#216
Conversation
…bus#38) ReadIntHoldingRegisters, ReadUintHoldingRegisters and ReadFloatHoldingRegisters on ModbusMasterEnhanced crashed with a confusing "ArgumentOutOfRangeException: startIndex" thrown from BitConverter when the configured wordSize was smaller than the target numeric type (e.g. wordSize=16 for a 32-bit int read). Add an explicit length check to ByteValueArraysToInts, ByteValueArraysToUInts and ByteValueArraysToFloats so callers see an actionable ArgumentException naming the parameter, the offending element index, and the minimum wordSize required for that read. Also multi-target NModbus.UnitTests at net6.0 and net8.0 so the test suite can run on Linux .NET runtimes alongside the existing net4.6 target. Tests: * ReadIntHoldingRegisters_WordSize16_ThrowsClearArgumentException * ReadUintHoldingRegisters_WordSize16_ThrowsClearArgumentException * ReadFloatHoldingRegisters_WordSize16_ThrowsClearArgumentException * ReadShortHoldingRegisters_WordSize16_StillWorks * ReadIntHoldingRegisters_WordSize32_StillWorks * ByteValueArraysToInts_FrontPaddingWithSmallArray_ThrowsClearArgumentException
|
Great PR as far as I can see, all tests pass and I have manually tested the old behavior of BitConverter to verify it was already throwing. It's already good enough to merge. A nice addition for readability could be to also use sizeof(int) etc instead of the magic numbers that were already present in the RegisterFunctions class. |
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net4.6</TargetFramework> | ||
| <TargetFrameworks>net4.6;net6.0;net8.0</TargetFrameworks> |
There was a problem hiding this comment.
Let's remove this change here and handle it in #218
|
|
||
| private static void RequireMinimumByteLength(byte[][] data, int minBytes, string operation) | ||
| { | ||
| // Issue #38: previously, supplying byte arrays narrower than the target |
There was a problem hiding this comment.
Can remove long comment, code is nice and clear already and there does not seem to be a convention with references to specific issues in this code base.
|
|
||
| public static ushort[] ByteValueArraysToUShorts(byte[][] data, bool frontPadding = true) | ||
| { | ||
| return frontPadding |
There was a problem hiding this comment.
Also add validation here
| RequireMinimumByteLength(data, sizeof(ushort), nameof(ByteValueArraysToShorts)); | |
| return frontPadding |
|
|
||
| public static short[] ByteValueArraysToShorts(byte[][] data, bool frontPadding = true) | ||
| { | ||
| return frontPadding |
There was a problem hiding this comment.
Also add validation here
| RequireMinimumByteLength(data, sizeof(short), nameof(ByteValueArraysToShorts)); | |
| return frontPadding |
There was a problem hiding this comment.
paneA-2: 採用しました。peek / consume 初期 lookup / claim 失敗後の latest の3回 findUnique を mockResolvedValueOnce で順序固定し、updateMany count=0 経路を通すよう修正しました（d52bb1bf7）。
Summary
Closes #38.
ModbusMasterEnhanced.ReadIntHoldingRegisters(and itsUint/Floatsiblings) crashed withArgumentOutOfRangeException: startIndexwhen the configuredwordSizewas smaller than the requested numeric type. The exception came fromBitConverter.ToInt32(e, e.Length - 4)withe.Length == 2, which gave the caller no idea why their request failed.This change adds an explicit
RequireMinimumByteLengthguard inRegisterFunctionssoByteValueArraysToInts,ByteValueArraysToUIntsandByteValueArraysToFloatsreject narrow input up front with anArgumentExceptionthat names the offending element and the minimumwordSizerequired for the read.It also multi-targets
NModbus.UnitTestsatnet6.0;net8.0(in addition to the existingnet4.6) so the suite can run on Linux .NET runtimes.Test plan
dotnet test NModbus.UnitTests/NModbus.UnitTests.csproj -f net8.0 -c Releasepasses (265/265after the fix, including 6 new tests for the validation behavior)dotnet build NModbus.IntegrationTests/NModbus.IntegrationTests.csproj -f net6.0 -c Releasestill succeeds