python4yahdlc icon indicating copy to clipboard operation
python4yahdlc copied to clipboard

Wrong processing of messages split into multiple buffers

Open Informatic opened this issue 7 years ago • 7 comments

Hey!

As far as I can see, python4yahdlc doesn't use a continuous buffer, like yahdlc recommends, but still persists its state. This makes it dump raw memory sometimes and doesn't allow for proper partial frames. You either need to store incoming data buffer somewhere and pull / discard bytes as they go (which would make it support partial frames), or call yahdlc_get_data_reset/yahdlc_get_data_reset_with_state after every call to yahdlc_get_data. (to just use yahdlc for HDLC frames parsing and just properly discard partial frames/buffers)

Best regards, Piotr.

Informatic avatar May 10 '18 07:05 Informatic

Hey!

Thank you for this issue. I will have a look at it as soon as possible.

Best

SkypLabs avatar May 10 '18 09:05 SkypLabs

As far as I can see, python4yahdlc doesn't use a continuous buffer, like yahdlc recommends, but still persists its state.

Actually it does through yahdlc itself. This last one uses an internal static variable to keep track of the frame decoding process.

This makes it dump raw memory sometimes and doesn't allow for proper partial frames.

Can you show me an example of python4yahdlc leaking memory?

You either need to store incoming data buffer somewhere and pull / discard bytes as they go (which would make it support partial frames), or call yahdlc_get_data_reset/yahdlc_get_data_reset_with_state after every call to yahdlc_get_data. (to just use yahdlc for HDLC frames parsing and just properly discard partial frames/buffers)

yahdlc_get_data_reset is used to reset the yahdlc_state internal variable once a frame has been decoded before starting to decode a new one. If you call it after each call of yahdlc_get_data, you won't be able to decode partial frames. Once a frame has been decoded, yahdlc_get_data_with_state calls yahdlc_get_data_reset_with_state by itself.

In addition, this test case is here to test that partial frame decoding works fine.

SkypLabs avatar May 15 '18 11:05 SkypLabs

@jeppefrandsen, could you please give me your feedback about this issue? Thank you in advance.

SkypLabs avatar Jun 14 '18 21:06 SkypLabs

Given that I haven't had any news on this issue in a while, I close it but feel free to reopen it if needed.

SkypLabs avatar Oct 06 '18 14:10 SkypLabs

Hi, @SkypLabs. I started to open a new issue, but then saw the reference to this one. I may be running into the same issue, and am seeing it when testing python4yahdlc in either a multi-threaded environment or in an interactive prompt. This is all with Python 3.6.

I wrote a short script to test:

#!/usr/bin/env python3
import yahdlc

message = "Hello World!"
frame = yahdlc.frame_data(message)
a = frame[:5]
b = frame[5:]

try:
    yahdlc.get_data(a)
except yahdlc.MessageError:
    pass

results = yahdlc.get_data(b)
print (results)
assert(results == (b'Hello World!', 0, 0))

If I run this, I get the expected result: (b'Hello World!', 0, 0)

If, however, I execute this line by line in the interactive shell (tried regular and ipython), I get a corrupted result:

(b'\x90qllo World!', 5, 0)
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-9-51f3b23b6df6> in <module>()
     14 results = yahdlc.get_data(b)
     15 print (results)
---> 16 assert(results == (b'Hello World!', 0, 0))

AssertionError:

We also are seeing this in production-intent code that monitors a serial port in a thread, using more or less your receive_data_frame.py example.

We'll try to track down the issue more here. Another interesting note is that if I rewrite the interactive example a bit, it works fine:

In [13]: for i in range(len(frame)):
    ...:     try:
    ...:         yahdlc.get_data(frame[:i])
    ...:     except yahdlc.MessageError:
    ...:         pass
    ...:     print(yahdlc.get_data(frame[i:]))

Output is correct for every split:

(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)
(b'Hello World!', 0, 0)

Thanks, Philip

podom avatar Nov 21 '18 22:11 podom

Hey @podom. Thank you for your message.

I will reopen this issue and have a look at your problem in details. I will come back to you as soon as I get a chance.

SkypLabs avatar Nov 21 '18 22:11 SkypLabs

Hey @podom.

Sorry I've been very busy, I didn't have the time to investigate in your issue. Have you made any progress on your side?

I will try to manage to work on it quite soon.

SkypLabs avatar Jan 17 '19 08:01 SkypLabs