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

test: Check msg type in msg capture is followed by zeros #25117

Merged
merged 1 commit into from May 13, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented May 12, 2022

Checking that they are not printable is an odd (and wrong) way to check that all chars are zero.

@maflcko maflcko added the Tests label May 12, 2022
@maflcko
Copy link
Member Author

maflcko commented May 12, 2022

See #21079

assert(len(msgtype) > 0)
assert(msgtype in MESSAGEMAP)
assert(len(remainder) == 0 or not remainder.decode().isprintable())
assert all(r == 0 for r in remainder)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, yes, this is weird code. Agree the new formulation is better.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Note that the .split() assignment fails if the raw_msgtype doesn't contain any zeroes:

>>> msgtype, remainder = bytes.fromhex("aabbcc").split(b'\x00', 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: not enough values to unpack (expected 2, got 1)

This problem is not introduced by this PR though; in the master branch it would occur as well:

>>> msgtype = bytes.fromhex("aabbcc").split(b'\x00', 1)[0]
>>> remainder = bytes.fromhex("aabbcc").split(b'\x00', 1)[1]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: list index out of range

I'm confused why this hasn't caused any problems. Whenever a msg-capture file is parsed that contains a message type that consumes the full 12 bytes (i.e. without zero), this should throw. This applies e.g. for sendaddrv2, getcfheaders, or getcfcheckpt.

@laanwj
Copy link
Member

laanwj commented May 12, 2022

Note that the .split() assignment fails if the raw_msgtype doesn't contain any zeroes:

Good point! May be better to use bytes.partition instead.

@maflcko
Copy link
Member Author

maflcko commented May 12, 2022

Thx, used rstrip + map lookup to check for trailing zeros

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, using rstrip is even simpler!

Code-review ACK faa5a7a

@maflcko maflcko merged commit 1d5325a into bitcoin:master May 13, 2022
@maflcko maflcko deleted the 2205-test-zero-🚛 branch May 13, 2022 06:14
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 13, 2022
@bitcoin bitcoin locked and limited conversation to collaborators May 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants