AdvancedClimateSystems / uModbus

Python implementation of the Modbus protocol.
Mozilla Public License 2.0
211 stars 81 forks source link

Redundancy Check issues #97

Open dmhouston95 opened 4 years ago

dmhouston95 commented 4 years ago

Function validate_crc does not evaluate the CRC bytes correctly. Incorrect use of not and ==

OrangeTux commented 4 years ago

What do you mean? Instead of

    if not struct.unpack('<H', get_crc(msg[:-2])) ==\
            struct.unpack('<H', msg[-2:]):
        raise CRCError('CRC validation failed.')

you suggest:


    if  struct.unpack('<H', get_crc(msg[:-2])) !=\
            struct.unpack('<H', msg[-2:]):
        raise CRCError('CRC validation failed.')
dmhouston95 commented 4 years ago

Yes. The expression evaluates false because not of an integer is always false unless the integer is zero. And then if you are trying to compare a boolean false (evaluated from the not expression) with an integer that will also always be false.

dmhouston95 commented 4 years ago

This would also be vaild

if not (struct.unpack('<H', get_crc(msg[:-2])) ==struct.unpack('<H', msg[-2:])):
        raise CRCError('CRC validation failed.')
rgov commented 4 years ago

See operator precedence: https://docs.python.org/3/reference/expressions.html#operator-precedence

not is less binding than == so I believe that not a == b is (in most cases?) the same as a != b. Since == and is have the same precedence, this is easier to see this way:

>>> not 1
False
>>> not 1 is True
True

If not bound more tightly, then it would be the same as (not 1) is True which doesn't match the behavior. But instead we get not (1 is True).

However != reads better to me.