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

Junk data misinterpreted as valid header #1099

Open
rogersimmons opened this issue May 30, 2022 · 2 comments
Open

Junk data misinterpreted as valid header #1099

rogersimmons opened this issue May 30, 2022 · 2 comments
Assignees
Labels

Comments

@rogersimmons
Copy link

The IncompleteMessageTest tests the behaviour of Queue when a write is started but not completed. A second write is then attempted, which - correctly - forces the lock and succeeds.

The issue is that data written by the incomplete attempt remains in the queue. If the second write is shorter then this (junk) data will remain after the end of the valid write. This junk data can then be misread as a valid header, which leads to undefined behaviour.

eg, after the writes in the above test, the queue looks like:

00010110  06 00 00 00 e5 68 65 6c  6c 6f 00 00 06 00 00 00  |.....hello......|
00010120  e5 77 6f 72 6c 64 00 00  00 00 20 6c 6f 6e 67 65  |.world.... longe|    <=== error here
00010130  72 20 77 72 69 74 65 00  00 00 00 00 00 00 00 00  |r write.........|
00010140  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|

The last valid entry is "world". The "...longer write" is junk data which is not considered part of the queue and will be overwritten by further writes.

However, AbstractWire.readDataHeader sees the "00 00 20 6c" bytes, which should be "00 00 00 00" - end of queue - but the 20 6c dangling from the failed write are picked up as part of the header value.

Fix is to ensure the next 4 bytes (after padding) are cleared to 0 when a write context is closed.

@rogersimmons
Copy link
Author

@JerryShea
Copy link
Contributor

As per @nicktindall comment in OpenHFT/Chronicle-Wire#481, we should definitely enhance this test to illustrate this behaviour and the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants