bpo-32502: Discard 64-bit (and other invalid) hardware addresses #5254

bbayles · 2018-01-20T23:47:25Z

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

bbayles · 2018-01-20T23:49:20Z

Lib/uuid.py

 _node = None

-def getnode():
+def getnode(*, getters=None):


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]))

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)?

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.

ned-deily · 2018-01-22T23:57:21Z

I'd like @warsaw to take a look at this since he has recently been in those places.

warsaw · 2018-01-23T03:18:22Z

Lib/test/test_uuid.py

+    # 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


Minor style nit: please use complete sentences everywhere (e.g. end them with a period).

warsaw · 2018-01-23T03:26:04Z

Lib/test/test_uuid.py

+
+        # Confirm that uuid.getnode ignores hardware addresses larger than 48
+        # bits
+        bad_getter = lambda: 1 << 48


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?

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.

warsaw · 2018-01-23T03:28:32Z

Lib/test/test_uuid.py

+        # bits
+        bad_getter = lambda: 1 << 48
+        node = self.uuid.getnode(getters=[bad_getter])
+        self.assertTrue(0 < node < (1 << 48), '%012x' % node)


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.

Quite right. Hopefully my new comment explains this.

warsaw · 2018-01-23T03:31:21Z

Lib/test/test_uuid.py

+        try:
+            self.uuid.uuid1(node=node)
+        except ValueError as e:
+            self.fail('uuid1 was given an invalid node ID')


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?

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.

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.

warsaw · 2018-01-23T03:34:21Z

Lib/uuid.py

 _node = None

-def getnode():
+def getnode(*, getters=None):


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)?

warsaw · 2018-01-23T03:41:29Z

Lib/uuid.py

+        if (_node is not None) and (0 <= _node < (1 << 48)):
            return _node
-    assert False, '_random_getnode() returned None'
+    assert False, '_random_getnode() returned an invalid value'


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.

bedevere-bot · 2018-01-23T03:41:46Z

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.

bbayles · 2018-01-23T12:18:06Z

I have made the requested changes; please review again.

@warsaw, many thanks for the review. I'll make some comments to your inline notes.

bedevere-bot · 2018-01-23T12:18:08Z

Thanks for making the requested changes!

@warsaw: please review the changes made to this pull request.

warsaw

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!

warsaw · 2018-01-23T20:43:56Z

Lib/test/test_uuid.py

+    # need not necessarily be 48 bits (e.g., EUI-64).
+    def test_uuid1_eui64(self):
+        # Reset any cached node value.
+        self.uuid._node = None


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.

warsaw · 2018-01-23T20:46:02Z

Lib/uuid.py

+_node_getters_win32 = [_windll_getnode, _netbios_getnode, _ipconfig_getnode]
+
+_node_getters_unix = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
+                      _arp_getnode, _lanscan_getnode, _netstat_getnode]


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)?

warsaw · 2018-01-23T20:46:24Z

Lib/uuid.py

+        if (_node is not None) and (0 <= _node < (1 << 48)):
            return _node
-    assert False, '_random_getnode() returned None'
+


Probably don't need that extra blank line.

bedevere-bot · 2018-01-23T20:47:07Z

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.

bbayles · 2018-01-24T00:10:39Z

I have made the requested changes; please review again.

@warsaw, many thanks again for the guidance.

bedevere-bot · 2018-01-24T00:10:41Z

Thanks for making the requested changes!

@warsaw: please review the changes made to this pull request.

warsaw

Looks awesome, thanks!

miss-islington · 2018-01-24T01:14:47Z

Thanks @bbayles for the PR, and @warsaw for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington · 2018-01-24T01:14:53Z

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

warsaw · 2018-01-24T01:15:27Z

@bbayles Do you want to give the cherry picker script a go?

pythonGH-5254). (cherry picked from commit 6b273f7)

bedevere-bot · 2018-01-24T02:14:26Z

GH-5290 is a backport of this pull request to the 3.6 branch.

bbayles · 2018-01-24T02:15:01Z

@warsaw - I took a shot. I think I followed the directions, but please correct me if I tripped up!

GH-5254) (#5290) * [3.6] bpo-32502: Discard 64-bit (and other invalid) hardware addresses (GH-5254). (cherry picked from commit 6b273f7)

bpo-32502: Discard 64-bit (and other invalid) hardware addresses

67c3a72

the-knights-who-say-ni added the CLA signed label Jan 20, 2018

bedevere-bot added the awaiting review label Jan 20, 2018

bbayles commented Jan 20, 2018

View reviewed changes

ned-deily requested a review from warsaw January 22, 2018 23:56

warsaw requested changes Jan 23, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 23, 2018

bbayles added 2 commits January 23, 2018 06:12

bpo-32502: Address review comments

b5e1c70

bpo-32502: Clarify comment in test

2c12989

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 23, 2018

warsaw requested changes Jan 23, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jan 23, 2018

bbayles added 2 commits January 23, 2018 18:06

bpo-32502: Address follow-up review comments

6e6d721

bpo-32502: Actually remove the line

d4b98df

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 24, 2018

warsaw approved these changes Jan 24, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 24, 2018

warsaw merged commit 6b273f7 into python:master Jan 24, 2018

bedevere-bot removed the awaiting merge label Jan 24, 2018

warsaw added the needs backport to 3.6 label Jan 24, 2018

bbayles added a commit to bbayles/cpython that referenced this pull request Jan 24, 2018

[3.6] bpo-32502: Discard 64-bit (and other invalid) hardware addresses (

02a4701

pythonGH-5254). (cherry picked from commit 6b273f7)

bbayles mentioned this pull request Jan 24, 2018

[3.6] bpo-32502: Discard 64-bit (and other invalid) hardware addresses (GH-5254) #5290

Merged

bedevere-bot removed the needs backport to 3.6 label Jan 24, 2018

warsaw pushed a commit that referenced this pull request Jan 24, 2018

[3.6] bpo-32502: Discard 64-bit (and other invalid) hardware addresses (

d69794f

GH-5254) (#5290) * [3.6] bpo-32502: Discard 64-bit (and other invalid) hardware addresses (GH-5254). (cherry picked from commit 6b273f7)

lordgordon mentioned this pull request Feb 21, 2018

Homebrew Installed Azure CLI not working - ValueError: field 6 out of range (need a 48-bit value) Azure/azure-cli#5184

Closed

rdeits mentioned this pull request Nov 8, 2018

trying to add a text geometry, stuck in JavaScript meshcat-dev/meshcat-python#28

Open

bebound mentioned this pull request Aug 12, 2024

{Core} Remove uuid workaround code Azure/azure-cli#29706

Merged

Uh oh!

bpo-32502: Discard 64-bit (and other invalid) hardware addresses #5254

bpo-32502: Discard 64-bit (and other invalid) hardware addresses #5254

Uh oh!

Conversation

bbayles commented Jan 20, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbayles Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ned-deily commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbayles Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 23, 2018

Uh oh!

bbayles commented Jan 23, 2018

Uh oh!

bedevere-bot commented Jan 23, 2018

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 23, 2018

Uh oh!

bbayles commented Jan 24, 2018

Uh oh!

bedevere-bot commented Jan 24, 2018

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jan 24, 2018

Uh oh!

miss-islington commented Jan 24, 2018

Uh oh!

warsaw commented Jan 24, 2018

Uh oh!

bedevere-bot commented Jan 24, 2018

Uh oh!

bbayles commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

bbayles commented Jan 20, 2018 •

edited by bedevere-bot

Loading

bbayles Jan 20, 2018 •

edited

Loading

bbayles Jan 23, 2018 •

edited

Loading