search

mycroes / Sally7

C# implementation of Siemens S7 connections with a focus on performance
MIT License
56 stars 22 forks source link

Guard against truncation of dataItems.Length #22

Closed gfoidl closed 2 years ago

gfoidl commented 2 years ago

In https://github.com/mycroes/Sally7/blob/d7e5b450de6508f80c7fda1681e70d12f66099d6/Sally7/S7ConnectionHelpers.cs#L48 and https://github.com/mycroes/Sally7/blob/d7e5b450de6508f80c7fda1681e70d12f66099d6/Sally7/S7ConnectionHelpers.cs#L63 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.

gfoidl commented 2 years ago

Maybe this needs to be more restrictive. With the debugger I see that https://github.com/mycroes/Sally7/blob/d7e5b450de6508f80c7fda1681e70d12f66099d6/Sally7/S7ConnectionHelpers.cs#L49 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.

scamille commented 2 years ago

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.

mycroes commented 2 years ago

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...