search

grid-x / modbus

BSD 3-Clause "New" or "Revised" License
78 stars 26 forks source link

Validate length of read result against request #53

Closed andig closed 1 year ago

andig commented 2 years ago

In https://github.com/evcc-io/evcc/discussions/3222 we've observed the infamous Huawei inverter return less bytes than requested as "success". Subsequently that leads to downstream panic when using the binary.BigEndian decoding functions.

Instead, I'd suggest to validate proper length in the library. Same could/should be added for:

Left out intentionally as I'm not fluent with what to expect in these cases.

andig commented 1 year ago

Friendly ping for review. Just had another incident of this at https://github.com/evcc-io/evcc/issues/8508 which causes application panic. This should be handled in the library, not on the application side.

andig commented 1 year ago

@guelfey @gq0 any chance for merging?

andig commented 1 year ago

@guelfey @gq0 any chance for merging?

gq0 commented 1 year ago

I'll consult with one of my colleagues about this matter.

frzifus commented 1 year ago

Just my rubber duck debugging brain now

If byte count is 2 and we also get 2 bytes in the data area, this code would succeed even though we might have requested 4 bytes (quantity = 2).

The higher layers could interpret this in a wrong way as they want to get 4 bytes but instead, they only got 2 bytes.

Yeah I can see that panicing in our application as well thinking

Yes, their might be a chance that the higher layer sees this: panic: runtime error index out of range

andig commented 1 year ago

Could you also add this for

I‘m not familiar with the other functions hence didn‘t add them.

andig commented 1 year ago

If byte count is 2 and we also get 2 bytes in the data area, this code would succeed even though we might have requested 4 bytes (quantity = 2).

Where do you see this in the PR? The comparison is received bytes vs 2*requested words?

dammarco commented 1 year ago

If byte count is 2 and we also get 2 bytes in the data area, this code would succeed even though we might have requested 4 bytes (quantity = 2).

Where do you see this in the PR? The comparison is received bytes vs 2*requested words?

My rubber duck brain wrote this for the old code ;) Just to see how it would have behaved. With your code change, this won't succeed ofcourse. Sorry for the confusion!

frzifus commented 1 year ago

Good, so whats the state here?

andig commented 1 year ago

Ready from by pov unless someone want to add the open points from initial post. Can be done in separate PR.

gq0 commented 1 year ago

From a protocol specification point of view, it makes sense 👍🏻 But I fear that not all devices in the field will behave according to specifications. If this error is triggered, it will break batch requests on our side, but we will monitor the rollout and merge it now.

andig commented 1 year ago

@gq0 as a follow-up we could still make this a sentinel error so it becomes easier to catch?