mycroes/Sally7

Guard against truncation of dataItems.Length

Closed this issue · 3 comments

In

read.ItemCount = (byte) dataItems.Count;
and
span[1] = (byte) dataItems.Count;
then int-count is casted (= truncated) to byte. This may lead to unexpected results for the user, iif there are more than 255 dataItems.

I'd expect that a reasonable exception will be thrown, that indicates the cause of the failure.
"Fail early" is better than letting a user believe to be able to read more than 255 dataItems.

Maybe this needs to be more restrictive. With the debugger I see that

var parameters = MemoryMarshal.Cast<byte, RequestItem>(buffer.Slice(19));

is of size 83, then there will be an IndexOutOfRangeException. So this should be the upper limit for the dataItems-count?

Or go a different route: if there are more dataItems than what can be handled, execute them in batches.
More work here, but less work for the user 😉

I'd just throw here, and the user is responsible to adapt to a batching strategy.

I think most cpus cant handle that many date items anyway. There was an issue about it in S7NetPlus where we stumbled into this through a snap7 limit which someone put there likely because of real limitations, but we never figured out what the true limit for each current cpus was.

So I think just throwing is good enough. Nobody will ever have that many small data items. And if they are larger you are limited by the max message size anyway and not by the data item count.

I think most cpus cant handle that many date items anyway. There was an issue about it in S7NetPlus where we stumbled into this through a snap7 limit which someone put there likely because of real limitations, but we never figured out what the true limit for each current cpus was.

So I think just throwing is good enough. Nobody will ever have that many small data items. And if they are larger you are limited by the max message size anyway and not by the data item count.

I actually think the amount is limited by PDU size only. There still is the issue of also being able to fit the data that needs to be read or written, so PR #24 isn't a full fail-safe either. It's one of those things that's still on my todo list, now if time would permit this as well...