Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong processing of messages split into multiple buffers #1

Open
Informatic opened this issue May 10, 2018 · 7 comments
Open

Wrong processing of messages split into multiple buffers #1

Informatic opened this issue May 10, 2018 · 7 comments
Assignees
Labels
bug New bug

Comments

@Informatic
Copy link

Informatic commented May 10, 2018

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.

@SkypLabs
Copy link
Owner

Hey!

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

Best

@SkypLabs SkypLabs self-assigned this May 10, 2018
@SkypLabs SkypLabs added the bug New bug label May 10, 2018
@SkypLabs
Copy link
Owner

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
Copy link
Owner

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

@SkypLabs SkypLabs removed the bug New bug label Oct 5, 2018
@SkypLabs
Copy link
Owner

SkypLabs commented Oct 6, 2018

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 SkypLabs closed this as completed Oct 6, 2018
@podom
Copy link

podom commented Nov 21, 2018

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

@SkypLabs
Copy link
Owner

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 SkypLabs reopened this Nov 21, 2018
@SkypLabs
Copy link
Owner

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 SkypLabs added the bug New bug label Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug New bug
Projects
None yet
Development

No branches or pull requests

3 participants