bpo-32502: Discard 64-bit (and other invalid) hardware addresses by bbayles · Pull Request #5254 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation29 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR adds a fix for bug 32502, which causes uuid.uuid1 to fail when uuid.getnode finds a hardware address that's not 48 bits (e.g., a 64-bit FireWire hardware address).
In this fix I'm simply discarding MAC addresses that appear to be outside of [0, 2^48). This means in principle that an EUI-64 address that doesn't have any high bits set could be accepted, but I think that's (a) OK, and (b) better than the status quo.
I'm also not modifying each of the platform-specific *_getnode functions - just the one that organizes them. If I should patch each of the existing ones I can, but in that case it might make sense to enforce that the *_getnode functions check for 48-bits.
https://bugs.python.org/issue32502
| @@ -656,7 +656,7 @@ def _random_getnode(): |
|---|
| _node = None |
| def getnode(): |
| def getnode(*, getters=None): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the getters keyword to make testing work without a bunch of mocks, but in principle it could be used to extend getnode and uuid1 for some other platform. uuid1(node=getnode(getters=[my_ifconfig_parser]))
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change is fine in principle, however this is an API change, so it needs to be documented in the getnode() docstring, and in the uuid.getnode() documentation. Don't forget a versionchanged in the latter. Both docs need to also mention that random UUID generation is always included in the list of getters. Are you okay with not providing a way to override that? (I think I am.)
The API change also needs to be documented in your News file entry.
Changing the API will also make this more difficult to backport, since you can't change the API in any previous version of Python. Are you okay with that (i.e. either not backporting it, or implementing the backport differently)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure about whether this counts as an API change or whether this needs to be backported, etc. To sidestep the problems that introduces I removed the API change and changed the approach a bit.
I'd like @warsaw to take a look at this since he has recently been in those places.
| # bpo-32502: UUID1 requires a 48-bit identifier, but hardware identifiers |
|---|
| # need not necessarily be 48 bits (e.g., EUI-64). |
| def test_uuid1_eui64(self): |
| # Reset any cached node value |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style nit: please use complete sentences everywhere (e.g. end them with a period).
| # Confirm that uuid.getnode ignores hardware addresses larger than 48 |
| # bits |
| bad_getter = lambda: 1 << 48 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this 64 bits? First, it more closely mirrors the failure in the bug, and second, it eliminates a minor cognitive hiccup. IOW, you have to think about any off-by-one possibility. Admittedly, it's minor, but think of the next person reading the code.
Maybe also call the variable too_large_getter or some such?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some more details in the comment to try to clarify this. 1 << 48, is, of course, 49 bits and not 48. My goal was to test something just outside the bounds of acceptability and not something well past it.
| # bits |
|---|
| bad_getter = lambda: 1 << 48 |
| node = self.uuid.getnode(getters=[bad_getter]) |
| self.assertTrue(0 < node < (1 << 48), '%012x' % node) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so why is that assertion going to be true? The reason is that the random UUID generator is always appended to getters, and it's that latter one that will return a valid value. I think you should write a comment above line 323 explaining that, otherwise it could be a bit of a head scratcher for someone just reading the test.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite right. Hopefully my new comment explains this.
| try: |
|---|
| self.uuid.uuid1(node=node) |
| except ValueError as e: |
| self.fail('uuid1 was given an invalid node ID') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a bit of a head scratcher. Why does the comment mention "only a 64-bit hardware address" but you're passing in node which should be 48 bits? It's not easy to follow the logic, so what's going on here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, I'm actually passing it in something that's 49 bits, which could happen in a 64-bit address space but not in a 48-bit address space.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify further: I'm having the "hardware" return a number that's from an address space larger than 48 bits. Running this test against master means that getnode() will return that value and this check will fail. On this branch, getnode() rejects the "hardware" address and properly constraints the value of node.
That is, this check is here to make sure that the patch actually addresses the problem in the bug report, which is that uuid.uuid1() fails.
| @@ -656,7 +656,7 @@ def _random_getnode(): |
|---|
| _node = None |
| def getnode(): |
| def getnode(*, getters=None): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change is fine in principle, however this is an API change, so it needs to be documented in the getnode() docstring, and in the uuid.getnode() documentation. Don't forget a versionchanged in the latter. Both docs need to also mention that random UUID generation is always included in the list of getters. Are you okay with not providing a way to override that? (I think I am.)
The API change also needs to be documented in your News file entry.
Changing the API will also make this more difficult to backport, since you can't change the API in any previous version of Python. Are you okay with that (i.e. either not backporting it, or implementing the backport differently)?
| return _node |
|---|
| assert False, '_random_getnode() returned None' |
| assert False, '_random_getnode() returned an invalid value' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about including the invalid value in the assertion message? (probably with a .format() call)
This case can't possibly happen but if for some odd reason it does, it would be good to know what the value was. Your test to the conditional means it could be something other than None.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
I have made the requested changes; please review again.
@warsaw, many thanks for the review. I'll make some comments to your inline notes.
Thanks for making the requested changes!
@warsaw: please review the changes made to this pull request.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for making the changes. I really only have some minor comments left, but I think you're really close to landing this. Thanks for your great contribution!
| # need not necessarily be 48 bits (e.g., EUI-64). |
|---|
| def test_uuid1_eui64(self): |
| # Reset any cached node value. |
| self.uuid._node = None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this restore the cached value after the test is done, or at least ensure that it's set back to None, even if the test fails? (E.g. add a self.addCleanup() or mock the global in this test.
| _node_getters_win32 = [_windll_getnode, _netbios_getnode, _ipconfig_getnode] |
|---|
| _node_getters_unix = [_unix_getnode, _ifconfig_getnode, _ip_getnode, |
| _arp_getnode, _lanscan_getnode, _netstat_getnode] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are constants, what do you think about capitalizing their names? E.g. _NODE_GETTERS_WIN32 and _NODE_GETTERS_UNIX (or maybe _NODE_GETTERS_POSIX)?
| return _node |
|---|
| assert False, '_random_getnode() returned None' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't need that extra blank line.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
I have made the requested changes; please review again.
@warsaw, many thanks again for the guidance.
Thanks for making the requested changes!
@warsaw: please review the changes made to this pull request.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome, thanks!
Thanks @bbayles for the PR, and @warsaw for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖
Sorry, @bbayles and @warsaw, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.cherry_picker 6b273f7f4056f8276f61a97c789d6bb4425e653c 3.6
@bbayles Do you want to give the cherry picker script a go?
bbayles added a commit to bbayles/cpython that referenced this pull request
@warsaw - I took a shot. I think I followed the directions, but please correct me if I tripped up!
warsaw pushed a commit that referenced this pull request
