bpo-32107 - Better merge of #4494 by warsaw · Pull Request #4576 · python/cpython

warsaw · 2017-11-26T23:41:29Z

https://bugs.python.org/issue32107

The original check was erroneous in two ways. First, the documentation said that "47 bit will never be set in IEEE 802 addresses obtained from network cards". I think this is referring to the universally vs. locally administered MAC addresses. Network cards from hardware manufacturers will always be universally administered in order to guarantee global uniqueness. But from https://en.wikipedia.org/wiki/MAC_address it says that this flag is the "distinguished by setting the second-least-significant bit of the first octet of the address", where a 0 means universally administered and a 1 means it's locally administered. This works out to the 42nd bit when counting the LSB as bit 1, or 1<<41. The second bug is that the original bitmask value isn't right for either description: % ./python.exe -c "from math import log2; print(log2(0x010000000000))" 40.0 This causes the test to fail on valid MAC addresses. Fix this by improving the comment, with references, and using the correct bitmask.

Also, clean up some return sites.

* Add a helper function to determine whether a MAC address is universally administered or not. * In the various MAC getter functions, keep track of the first locally administered MAC, and if no universally administered MAC is found, return the first local MAC. * In the ultimate fallback _random_getnode(), add a comment for the multicast bit being set, and use a better bitmask * In getnode(), just fall back to _random_getnode() explicitly if no other MAC has been found.

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

The original check was erroneous in two ways. First, the documentation said that "47 bit will never be set in IEEE 802 addresses obtained from network cards". I think this is referring to the universally vs. locally administered MAC addresses. Network cards from hardware manufacturers will always be universally administered in order to guarantee global uniqueness. But from https://en.wikipedia.org/wiki/MAC_address it says that this flag is the "distinguished by setting the second-least-significant bit of the first octet of the address", where a 0 means universally administered and a 1 means it's locally administered. This works out to the 42nd bit when counting the LSB as bit 1, or 1<<41. The second bug is that the original bitmask value isn't right for either description: % ./python.exe -c "from math import log2; print(log2(0x010000000000))" 40.0 This causes the test to fail on valid MAC addresses. Fix this by improving the comment, with references, and using the correct bitmask.

Also, clean up some return sites.

* Add a helper function to determine whether a MAC address is universally administered or not. * In the various MAC getter functions, keep track of the first locally administered MAC, and if no universally administered MAC is found, return the first local MAC. * In the ultimate fallback _random_getnode(), add a comment for the multicast bit being set, and use a better bitmask * In getnode(), just fall back to _random_getnode() explicitly if no other MAC has been found.

…torchaka/cpython into test_uuid This merges the relevant bits of python#4572 and should be considered instead of that PR.

The original check was erroneous in two ways. First, the documentation said that "47 bit will never be set in IEEE 802 addresses obtained from network cards". I think this is referring to the universally vs. locally administered MAC addresses. Network cards from hardware manufacturers will always be universally administered in order to guarantee global uniqueness. But from https://en.wikipedia.org/wiki/MAC_address it says that this flag is the "distinguished by setting the second-least-significant bit of the first octet of the address", where a 0 means universally administered and a 1 means it's locally administered. This works out to the 42nd bit when counting the LSB as bit 1, or 1<<41. The second bug is that the original bitmask value isn't right for either description: % ./python.exe -c "from math import log2; print(log2(0x010000000000))" 40.0 This causes the test to fail on valid MAC addresses. Fix this by improving the comment, with references, and using the correct bitmask.

Also, clean up some return sites.

* Add a helper function to determine whether a MAC address is universally administered or not. * In the various MAC getter functions, keep track of the first locally administered MAC, and if no universally administered MAC is found, return the first local MAC. * In the ultimate fallback _random_getnode(), add a comment for the multicast bit being set, and use a better bitmask * In getnode(), just fall back to _random_getnode() explicitly if no other MAC has been found.

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

serhiy-storchaka · 2017-11-27T00:14:52Z

                   _arp_getnode, _lanscan_getnode, _netstat_getnode]

-    for getter in getters + [_random_getnode]:
+    for getter in getters:


The difference is that exceptions in _random_getnode() no longer ignored. Ignoring all exceptions is not a good idea, but what exceptions can raise _random_getnode()?

I think the only possibility is an import error. Ultimately, random.getrandbits() boils down to os.urandom() which I think can block but AFAICT won't raise an exception. And it bothers me (as I think it bothers you) that all exceptions in all those getters are just swallowed. Here's my thinking: since I'm not sure of the best way to report any errors in the other getters, and since the last getter is _random_getnode(), and since it's pretty unlikely that the function will experience an ImportError, and if it does then I think it's better to report that than to silently swallow the exception and return None from getnode(), since that will lead to even more mysterious bugs, I think letting any rare exceptions in _random_getnode() not be swallowed is the better choice.

It's LGTM. I just wanted to make sure that this is an intentional change.

serhiy-storchaka · 2017-11-27T00:29:17Z

+        # `test_random_getnode()` method specifically.  Another case is the
+        # Travis-CI case, which apparently only has locally administered MAC
+        # addresses.
+        if not random and not os.getenv('TRAVIS'):


If add this test then restore the network parameter. check_node() is called for addresses obtained from network cards, for random addresses, but it also is called in cases where it is not known (and is not important) whether the address is real or random.

However, if you look at the original code, every call to check_node() except the one in test_random_getnode() called it with network=True. If I do add it back, which I'm willing to do, then I propose that we call this argument multicast, default it to False, and make it a keyword-only argument. I will see how that looks.

And I will change random to local.

There are three tests in which check_node() is called without network=True. Only one of them explicitly tests _random_getnode(). In the other twos we can't say what is the source of the address. We shouldn't test either 1<<40 nor 1<<41 bits in that cases.

serhiy-storchaka · 2017-11-27T00:37:43Z

                        "%s is not an RFC 4122 node ID" % hex)

    @unittest.skipUnless(os.name == 'posix', 'requires Posix')
+    @unittest.skipIf(os.getenv('TRAVIS'),


Don't skip this test. It should test that corresponding method don't raise an exception, crash or hang. And returns a reasonable integer or None. Just skip additional checks.

Yep, I meant to remove those skipIfs; I'll push an update for that.

* Restored the check for the multicast bit. * Remove some now-extraneous skipIfs.

serhiy-storchaka · 2017-11-27T17:06:02Z

-        if not random and not os.getenv('TRAVIS'):
+        if not local and not os.getenv('TRAVIS'):
            self.assertFalse(node & (1 << 41), '%012x' % node)
+        is_multicast = (node & (1 << 40))


We shouldn't test the multicast bit for _unix_getnode() and _windll_getnode(). Actually we should test it only for _random_getnode(), because the other methods can return an address with this bit set or clear (your original issue).

serhiy-storchaka · 2017-11-27T17:06:25Z

        # Travis-CI case, which apparently only has locally administered MAC
        # addresses.
-        if not random and not os.getenv('TRAVIS'):
+        if not local and not os.getenv('TRAVIS'):


We shouldn't test the multicast bit for _unix_getnode() and _windll_getnode().

Good point. I'm removing the multicast bit check from check_node() since only the test for _random_getnode() needs to check that.

* Rename `local` to `admin` and use this to control whether the local vs. universal admin bit is set. Generally we do want to test that, except for the random case, and the two cases where we don't know what the provenance of the MAC address is. * We only need to check the multicast bit in the random test.

serhiy-storchaka · 2017-11-27T17:23:55Z

-    def check_node(self, node, requires=None, *,
-                   # Additional bitmask checks to perform.
-                   local=False, multicast=False):
+    def check_node(self, node, requires=None, *, admin=True):


The meaning of the admin flag is less obvious than the meaning of the opposite network flag. I expected this is something related to administrative privileges. Could you add an elaboration?

For me, network wasn't really descriptive either. Something like check_universally_administered_bit would be more descriptive but prohibitively long. We could go back to local or switch the parity to universal. I'm open to other suggestions.

serhiy-storchaka · 2017-11-27T17:32:36Z

-        self.check_node(node, 'unix')
+        # Since we don't know the provenance of the MAC address, don't check
+        # whether it is locally or universally administered.
+        self.check_node(node, 'unix', admin=False)


I would prefer to not do non-guarantied tests by default, and do them only if this is specified explicitly.

Hmm, I obviously mildly (+0) prefer it the other way around. Would your opinion change if we named the flag universal?

This is your check, I left it on to you.

I chose check_bit as the parameter name, but didn't change the default value. I'm happy with this.

warsaw · 2017-11-27T19:40:43Z

@serhiy-storchaka Thanks for the great review!

This reverts commit 9522a21.

Remove a flakey test and rewrite another one for readability.

warsaw and others added 30 commits November 21, 2017 12:00

Return None instead of 0

941b47d

Never return a locally administered MAC address

e529214

Also, clean up some return sites.

A missed return

c056584

Two typos

7fd446e

DEBUGGING TRAVIS - DO NOT MERGE

fb2c2f2

Two tests cannot succeed on Travis-CI

99e86b2

bpo-32107: Remove the check of the multicast bit in test_uuid.

74702e3

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

Return None instead of 0

d3d0465

Never return a locally administered MAC address

7c17cd8

Also, clean up some return sites.

A missed return

af7929c

Two typos

51ba0b3

DEBUGGING TRAVIS - DO NOT MERGE

909ee79

Two tests cannot succeed on Travis-CI

6acb120

Merge branch 'test_uuid' of github.com:warsaw/cpython into test_uuid

b7d647c

Merge branch 'test-uuid-multicast-bit' of https://github.com/serhiy-s…

26db2f5

…torchaka/cpython into test_uuid This merges the relevant bits of python#4572 and should be considered instead of that PR.

Return None instead of 0

83dc3a3

Never return a locally administered MAC address

fb156f3

Also, clean up some return sites.

A missed return

668f4f6

Two typos

6eecb80

DEBUGGING TRAVIS - DO NOT MERGE

87712f3

Two tests cannot succeed on Travis-CI

740393e

bpo-32107: Remove the check of the multicast bit in test_uuid.

225b063

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

DEBUGGING TRAVIS - DO NOT MERGE

5b8ca90

Two tests cannot succeed on Travis-CI

2aa4f50

Merge branch 'test_uuid' of github.com:warsaw/cpython into test_uuid

71ab3b5

warsaw added skip issue skip news labels Nov 26, 2017

bedevere-bot added the awaiting merge label Nov 26, 2017

the-knights-who-say-ni added the CLA signed label Nov 26, 2017

warsaw changed the title ~~Better merge of #4494~~ bpo-32107 - Better merge of #4494 Nov 26, 2017

warsaw mentioned this pull request Nov 26, 2017

bpo-32107 - Fix test_uuid #4494

Closed

warsaw removed the skip news label Nov 26, 2017

Add NEWS after all.

efb6c07

serhiy-storchaka reviewed Nov 27, 2017

View reviewed changes

Respond to review comments.

17deb59

* Restored the check for the multicast bit. * Remove some now-extraneous skipIfs.

serhiy-storchaka reviewed Nov 27, 2017

View reviewed changes

s/admin/check_bit/

af4a656

serhiy-storchaka approved these changes Nov 27, 2017

View reviewed changes

warsaw merged commit 9522a21 into python:master Nov 27, 2017

bedevere-bot removed the awaiting merge label Nov 27, 2017

warsaw deleted the test_uuid_2 branch November 27, 2017 19:40

vstinner added a commit that referenced this pull request Nov 27, 2017

Revert "bpo-32107 - Better merge of #4494 (#4576)" (#4593)

c9409f7

This reverts commit 9522a21.

warsaw restored the test_uuid_2 branch November 27, 2017 23:43

warsaw added a commit that referenced this pull request Nov 29, 2017

[2.7] bpo-32107 - Backport bitmask check fix (GH-4576) (#4590)

56e444f

Remove a flakey test and rewrite another one for readability.

warsaw added a commit that referenced this pull request Nov 29, 2017

[3.6] bpo-32107 - Backport bitmask check fix (GH-4576) (#4591)

23cc8c0

Remove a flakey test and rewrite another one for readability.

warsaw deleted the test_uuid_2 branch November 29, 2017 15:36

Uh oh!

Conversation

warsaw commented Nov 26, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

warsaw Nov 27, 2017 • 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!

warsaw commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

warsaw commented Nov 26, 2017 •

edited by bedevere-bot

Loading

warsaw Nov 27, 2017 •

edited

Loading