[Bug]: Exception while parsing the data received #206
Comments
|
I have seen this error a few times before and I thought it was fixed adjust heade_len? it never shows up to me after that, the corrupted data isn't necessary needed because It will only handle the necessary one. Is this issue happen to you before/after beta5? try to delete both lines "header_len = header_len_6699" and "header_len = header_len_55AA" inside the while loop. |
Yes, I've seen it long beffore beta5.
Done. I'll report you how it works. |
|
I'm really not sure about this this line, However this caused an issues for some devices probably similar to "IR Remote" when learn a button and sends a payload with the base64 code of learned button. it raises an error... so I may just stop it from raising an errors. even tho yeah the payload you received hass-localtuya/custom_components/localtuya/core/pytuya/__init__.py Lines 488 to 492 in 9363387 |
All the exceptions of this kind I experienced are for "gigabytes"! That's why I believe it is either a packet with wrong format (not related to LocalTuya actually), or a bug somewhere in the data flow. BTW first I thought they are negative numbers converted to unsigned values, but they are not: inverse values are also too big. Not a big issue if it is unrelated data. But I'd prefer to ensure it is not a possible bug which leads to information loss. |
You may compare the calculated |
|
That fix didn't help: |
|
Also, I believe that here: hass-localtuya/custom_components/localtuya/core/pytuya/__init__.py Lines 631 to 633 in 9363387 if both 55AA and 6699 found, prefix_offset should be the minimum of both offsets...
|
|
Sorry, I was wrong and deleted messages I wrote without paying enough attention! |
This can not happen. Actually, looking at this, I may need to adjust some lines. This will only handle 55aa |
|
I'm not sure if this will make a difference with corrupted data. However, if it still does, I'll make a change to ignore the payload that has a length of millions. Can you test if the last commit would make any difference, I don't know how to trigger this on my device. |
Yesterday, I've made some changes to this line and around, and, so far, my log confirms that:
But only presuming that corrupted data will never come.
Strictly speaking, the buffer contains random bytes (encrypted payload, crc/hmac) which may have any value. The chance is small, and may be even zero (cryptographic data usualy does not contain zero byte sequences). The current code does not presume that incoming Note, that I still can't explain how those |
I'm currently running a test with more comprehensive changes, and with logging of suspicious events. I'll be back 😄 |
|
Maybe you can also add |
|
I log the buffer bytes, and do it in the |
|
@xZetsubou Please help me to understand, because I found nothing in the sources. I look into I can't find a line where the buffer is reset due to its invalid content! |
|
Because this was never an issue before adding support for protocol '3.5', LocalTuya never expected the devices to send a payload of millions in size so there is no expectation for it. And this what I meant by I may make changes to ignore it by adding a handler for this and reset the buffer when it happens. but I was curious of what this mass payload has.
note: That when we raise an error to |
|
Does the invalid payload still shows up? |
|
No, nothing yet. |
|
It happened, and I was right: both signatures found in one, good, message! The message is from a gateway: It has "00006699" as a seqno, "00000008" as a cmd, "00000098" as the length (158 bytes). There are no a decoded message in the log, because I didn't turn debug for it. But there are no other errors, meaning, it was decoded and dispatched. The code I run: def add_data(self, data):
"""Add new data to the buffer and try to parse messages."""
self.buffer += data
header_len_55AA = struct.calcsize(MESSAGE_RECV_HEADER_FMT)
header_len_6699 = struct.calcsize(MESSAGE_HEADER_FMT_6699)
header_len = header_len_55AA
prefix_len = len(PREFIX_55AA_BIN)
while self.buffer:
prefix_offset_55AA = self.buffer.find(PREFIX_55AA_BIN)
prefix_offset_6699 = self.buffer.find(PREFIX_6699_BIN)
if prefix_offset_55AA > 0 or prefix_offset_6699 > 0:
self.warning("Odd bytes, 6699: %d, 55AA: %d, buffer: %d %r", prefix_offset_6699, prefix_offset_55AA, len(self.buffer), binascii.hexlify(self.buffer))
if prefix_offset_55AA < 0 and prefix_offset_6699 < 0:
self.warning("Odd data: %d-%d\n%r\n%r", len(data), len(self.buffer), binascii.hexlify(data), binascii.hexlify(self.buffer))
self.buffer = self.buffer[1 - prefix_len :]
else:
if prefix_offset_55AA < 0:
prefix_offset = prefix_offset_6699
elif prefix_offset_6699 < 0:
prefix_offset = prefix_offset_55AA
else:
prefix_offset = (
prefix_offset_55AA if prefix_offset_55AA < prefix_offset_6699 else prefix_offset_6699
)
self.warning("Both signatures found: %d-%d\n%r\n%r", len(data), len(self.buffer), binascii.hexlify(data), binascii.hexlify(self.buffer))
header_len = (
header_len_6699 if prefix_offset == prefix_offset_6699 else header_len_55AA
)
# Check if enough data for message header
if len(self.buffer) < header_len:
break
self.buffer = self.buffer[prefix_offset:]
header = parse_header(self.buffer)
if header.length > 2000:
self.error("Packet size is too big: %d, size: %d buffer: %r", header.length, len(self.buffer), binascii.hexlify(self.buffer))
hmac_key = self.local_key if self.version >= 3.4 else None
no_retcode = False
msg = unpack_message(
self.buffer,
header=header,
hmac_key=hmac_key,
no_retcode=no_retcode,
logger=self,
)
self.buffer = self.buffer[header.total_length:]
self._dispatch(msg)So please implement the full check of the signature offsets, like I do, including checks for -1, in case of damaged data! Meanwhile, I'll turn the debugging on, because I'm curious what the gateway is sending. |
|
Assuming that both of them exist, how would this cause an issue? "prefix_offset" will prioritize finding 55aa over 6699, but for some reason, it actually hasn't found either of them in your payload since both of them returned -1. So instead we can make a check before it goes to parse header, if both prefix failed to found then ignore it. |
If both values can be found in a message, why can't "000055AA" be found in a 6699 message? This log record is a proof of the concept. It means, the check for booth offsets != -1 is the must, and the lowest offset should be taken. Or, would you wait more for a proof of "000055AA" found inside 6699 message? I strongly believe that a good code should check for all possible values without presuming "it will never happen", when the data comes from untrusted sources. |
|
Can you decode that payload? You have the local key in the diagnistics data. |
This comment was marked as off-topic.
This comment was marked as off-topic.
"else" there means "both found" (both are not < 0), and that's what the log record says. I don't understand why do you think opposite... |
|
Probably because I'm half a sleep 👀 sorry for that not sure why did think of that too. I'll look into this deep later thanks for the debugging ❤️ really useful informations. edit: btw did the error raised after this payload? |
No, because it was correctly handled :) |
|
I can't decode the payload because protocols edit im pretty sure this is unrelated to the mass payload issue and this message handled like normally when I wake up I'll double check this. |
|
I can understand your point that offset may cause an issues if it 55aa existed in 6699 message there was no issues reported that related to this matter. So let's see if 55aa existed in 6699 and we take 6699 offset would this break the data? I think after changing buffer offset to takes Technically I added add_data
def add_data(self, data):
"""Add new data to the buffer and try to parse messages."""
self.buffer += data
header_len = struct.calcsize(MESSAGE_RECV_HEADER_FMT)
while self.buffer:
# Check if enough data for measage header
if len(self.buffer) < header_len:
break
header = parse_header(self.buffer, logger=self)
hmac_key = self.local_key if self.version >= 3.4 else None
no_retcode = False
msg = unpack_message(
self.buffer,
header=header,
hmac_key=hmac_key,
no_retcode=no_retcode,
logger=self,
)
self.buffer = self.buffer[header.total_length :]
self._dispatch(msg)We can also add this line to always take the minimum prefix, But then again is there any point of adding it when we already resolved buffer offset? because it was the main issue it wasn't clean the buffer correctly before takes the lowest prefix
prefix_offset_55AA = self.buffer.find(PREFIX_55AA_BIN)
prefix_offset_6699 = self.buffer.find(PREFIX_6699_BIN)
prefixes = (prefix_offset_6699, prefix_offset_55AA)
prefix_offset = min(prefix for prefix in prefixes if not prefix < 0)note that having 6699 in 55aa messages was already taken in conclusions here. hass-localtuya/custom_components/localtuya/core/pytuya/__init__.py Lines 457 to 462 in 133b26e |
It will definitely break, if it would happen. But waiting is a waste of time: it is the case when OfftopicLooks like you have no experience writing software for something like a missile, when it is not possible to make real life tests of all the cases! 😄 The simplified version of your code without any search is good, presuming the data is always correct. But, anyway, I'd put an explicit check for |
|
For sure I have never had that experience 😄 I always go with the saying if it's not broken don't fix it Then let's go with your suggestion, however Let's not pass the data to |
* Reset the buffer if we can't find valid message. * Ensure that the prefix at the start of the message.
* Reset the buffer if we can't find valid message. * Ensure that the prefix at the start of the message.
xZetsubou
added
the
master/next-release
|
This is an example of 55AA inside 6699, finally: It would produce exactly the exception in the first post. |
|
This is an example of a damaged packet: Obviously, it is a tail of a message, but, unlike cases in the #213, I have no log records of attemps to parse the head. This damaged packet was produced by a cheap BLE T&H sensor with display, like this. They are useless for automation, because the send measurements very rare, but sometimes they dump all they've collected for past hours at once. See an a small extraction when two such sensors decided to free their history: T&H sensors outputNote the data rate: several messages per second! It lasted for many seconds, with that damaged message in between. So, there are real life confirmations that the checks I suggested and you've implemented are all required. |
Yes, please do. As I said before, there were lots of presumptions in the original code, but the logs showed they all were wrong. |
|
By the way I did try to parse the message you got, as expected to raised an error with damaged prefix.
|
|
Right. But exception is a performance drop, also may loose the next packet already buffered in the socket. |
github-actions
bot
added
stale
and removed
master/next-release
LocalTuya Version
3.2.5.2b5
Home Assistant Version
2024.4.3
Environment
What happened?
I see such log records regularly, I think, since I first installed LocalTuya.
It may happen that HA engine sends wrong data sometimes, but may be a parser error. The following lines:
hass-localtuya/custom_components/localtuya/core/pytuya/__init__.py
Lines 626 to 634 in 3a42a20
for me look like you always expect a valid data, presuming that either prefix
55AAor6699exists even if not found, and does not handle well the case when there are both, and the expression "1 - prefix_len" scares me, because it must be a negative value always (I'm not familiar with Python!). But it's only MHO.I tried to uncomment the line
hass-localtuya/custom_components/localtuya/core/pytuya/__init__.py
Line 894 in 3a42a20
and turn on debug for both custom_components.localtuya and custom_components.localtuya.pytuya, but I see no related records in the log. Anyway, even if that line wrote something, it would be overkill: the data content is useful only if the exception happened, and it happens only a few times per day.
If the actual data that causes exceptions is required, I believe better to write it into files in binary form, piece by piece, with autogenerated file names. If you can give me a piece of such a code, I'd be happy to collect the data.
Steps to reproduce.
Don't know
Relevant log output
Diagnostics information.
No response
The text was updated successfully, but these errors were encountered: