Skip to content

fix: validate byte array length in RegisterFunctions converters (#38) #216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JoshuaVlantis wants to merge 1 commit into
base: master
Choose a base branch
from
+124 −2
Open

fix: validate byte array length in RegisterFunctions converters (#38) #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions NModbus.UnitTests/Extensions/ModbusMasterEnhancedFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
using System;
using Moq;
using NModbus.Extensions;
using NModbus.Extensions.Functions;
using Xunit;

namespace NModbus.UnitTests.Extensions
{
/// <summary>
/// Regression coverage for issue #38: <see cref="ModbusMasterEnhanced.ReadIntHoldingRegisters"/>
/// (and the other Read*HoldingRegisters helpers) crashed with a confusing
/// "Destination array is not long enough" exception when the configured
/// <c>wordSize</c> was smaller than the requested numeric type.
/// </summary>
public class ModbusMasterEnhancedFixture
{
private static Mock<IModbusMaster> BuildMaster(ushort[] response)
{
var master = new Mock<IModbusMaster>(MockBehavior.Strict);
master
.Setup(m => m.ReadHoldingRegisters(It.IsAny<byte>(), It.IsAny<ushort>(), It.IsAny<ushort>()))
.Returns(response);
return master;
}

[Fact]
public void ReadIntHoldingRegisters_WordSize16_ThrowsClearArgumentException()
{
// Arrange: wordSize=16 means a "value" is a single 16-bit register,
// which is too small to hold a 32-bit int. The library should reject
// the call with an actionable error rather than crashing inside
// BitConverter with "Destination array is not long enough" (issue #38).
var master = BuildMaster(new ushort[] { 0x1234 });
var enhanced = new ModbusMasterEnhanced(master.Object, wordSize: 16);

// Act + Assert
var ex = Assert.Throws<ArgumentException>(() => enhanced.ReadIntHoldingRegisters(1, 10, 1));
Assert.Contains("wordSize", ex.Message, StringComparison.OrdinalIgnoreCase);
Assert.Contains("32", ex.Message);
}

[Fact]
public void ReadUintHoldingRegisters_WordSize16_ThrowsClearArgumentException()
{
var master = BuildMaster(new ushort[] { 0x1234 });
var enhanced = new ModbusMasterEnhanced(master.Object, wordSize: 16);

var ex = Assert.Throws<ArgumentException>(() => enhanced.ReadUintHoldingRegisters(1, 10, 1));
Assert.Contains("wordSize", ex.Message, StringComparison.OrdinalIgnoreCase);
}

[Fact]
public void ReadFloatHoldingRegisters_WordSize16_ThrowsClearArgumentException()
{
var master = BuildMaster(new ushort[] { 0x1234 });
var enhanced = new ModbusMasterEnhanced(master.Object, wordSize: 16);

var ex = Assert.Throws<ArgumentException>(() => enhanced.ReadFloatHoldingRegisters(1, 10, 1));
Assert.Contains("wordSize", ex.Message, StringComparison.OrdinalIgnoreCase);
}

[Fact]
public void ReadShortHoldingRegisters_WordSize16_StillWorks()
{
// 16-bit values fit in a 16-bit word, so this should succeed and not regress.
var master = BuildMaster(new ushort[] { 0x1234 });
var enhanced = new ModbusMasterEnhanced(master.Object, wordSize: 16);

short[] result = enhanced.ReadShortHoldingRegisters(1, 10, 1);
Assert.Single(result);
Assert.Equal(unchecked((short)0x1234), result[0]);
}

[Fact]
public void ReadIntHoldingRegisters_WordSize32_StillWorks()
{
// Sanity check that the wordSize=32 path is unchanged by the validation guard.
// We don't pin the exact byte order here, that's already covered by the
// existing extension tests and depends on host endianness.
var master = BuildMaster(new ushort[] { 0x1234, 0x5678 });
var enhanced = new ModbusMasterEnhanced(master.Object, wordSize: 32);

int[] result = enhanced.ReadIntHoldingRegisters(1, 10, 1);
Assert.Single(result);
}

[Fact]
public void ByteValueArraysToInts_FrontPaddingWithSmallArray_ThrowsClearArgumentException()
{
// Direct coverage on the low-level helper: passing 2-byte arrays
// would previously throw ArgumentOutOfRangeException from BitConverter
// with "index" name and a confusing "Destination array is not long enough"
// chain. We now throw ArgumentException up front with a clearer message.
byte[][] tooSmall = { new byte[] { 0x12, 0x34 } };

var ex = Assert.Throws<ArgumentException>(() => RegisterFunctions.ByteValueArraysToInts(tooSmall));
Assert.Contains("4 bytes", ex.Message);
}
}
}
3 changes: 1 addition & 2 deletions NModbus.UnitTests/NModbus.UnitTests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>net4.6</TargetFramework>
<TargetFrameworks>net4.6;net6.0;net8.0</TargetFrameworks>

@Flundrahn Flundrahn Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this change here and handle it in #218

<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<AssemblyName>NModbus.UnitTests</AssemblyName>
<PackageId>NModbus.UnitTests</PackageId>
<GenerateRuntimeConfigurationFiles>true</GenerateRuntimeConfigurationFiles>
<RuntimeFrameworkVersion>1.0.4</RuntimeFrameworkVersion>
<IsPackable>false</IsPackable>
</PropertyGroup>

Expand Down
23 changes: 23 additions & 0 deletions NModbus/Extensions/Functions/RegisterFunctions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,25 +67,48 @@ public static ushort[] ByteValueArraysToUShorts(byte[][] data, bool frontPadding

public static int[] ByteValueArraysToInts(byte[][] data, bool frontPadding = true)
{
RequireMinimumByteLength(data, 4, nameof(ByteValueArraysToInts));
return frontPadding
? data.Select(e => BitConverter.ToInt32(e, e.Length - 4)).ToArray()
: data.Select(e => BitConverter.ToInt32(e, 0)).ToArray();
}

public static uint[] ByteValueArraysToUInts(byte[][] data, bool frontPadding = true)
{
RequireMinimumByteLength(data, 4, nameof(ByteValueArraysToUInts));
return frontPadding
? data.Select(e => BitConverter.ToUInt32(e, e.Length - 4)).ToArray()
: data.Select(e => BitConverter.ToUInt32(e, 0)).ToArray();
}

public static float[] ByteValueArraysToFloats(byte[][] data, bool frontPadding = true)
{
RequireMinimumByteLength(data, 4, nameof(ByteValueArraysToFloats));
return frontPadding
? data.Select(e => BitConverter.ToSingle(e, e.Length - 4)).ToArray()
: data.Select(e => BitConverter.ToSingle(e, 0)).ToArray();
}

private static void RequireMinimumByteLength(byte[][] data, int minBytes, string operation)
{
// Issue #38: previously, supplying byte arrays narrower than the target

@Flundrahn Flundrahn Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// numeric type leaked through to BitConverter and raised an opaque
// ArgumentOutOfRangeException about "startIndex". Validate up front so
// callers configuring ModbusMasterEnhanced with too small a wordSize
// see an actionable message instead.
if (data == null) throw new ArgumentNullException(nameof(data));
for (int i = 0; i < data.Length; i++)
{
if (data[i] == null || data[i].Length < minBytes)
{
throw new ArgumentException(
$"{operation} requires each byte array to be at least {minBytes} bytes; element {i} has {data[i]?.Length ?? 0}. " +
"When using ModbusMasterEnhanced, set wordSize to at least the size of the value being read (e.g. 32 for int/uint/float).",
nameof(data));
}
}
}


public static byte[][] CharsToByteValueArrays(char[] data, uint wordSize, bool frontPadding = true, bool singleCharPerRegister = true)
{
Expand Down
Loading