The check_node() function in test_uuid.py uses the wrong bitmask, causing the tests to fail on valid MAC addresses. Also, the description in the comment is incorrect. From my reading of the wikipedia page, the test is trying to ensure that the MAC address is "universally administered", which is designated by a zero value in "the second least significant digit of the first octet".

Thus the bitmask value *and* the comment are both wrong AFAICT, given that the original value is:

% ./python.exe -c "from math import log2; print(log2(0x010000000000))"
40.0

I have a fix that passes on my machine, so I'll PR that and see if it passes on CI.

I'm just wondering why tests were passing successfully before.

Pure chance I believe. It all depends on the MAC address of the machines the test runs on (which is perhaps an unacceptable environmental variability in the test suite, and should be fixed/mocked?).

FWIW, I saw the failures on my 2017 MacBook Pro, where the MAC address did have that bit set on two of its interfaces.

Yikes. It's entirely possible that these tests are tainted by environmental leakage. I was looking into why Travis fails on my PR:

https://travis-ci.org/python/cpython/jobs/305433725

and stepping through _ifconfig_getnode() on my Mac. The "ibridge" interface is getting returned, which has a MAC address of:

>>> from uuid import _ifconfig_getnode
>>> mac = _ifconfig_getnode()
>>> hex(mac)
'0xacde48001122'

That's for the en5 interface, which according to this article is the bridge to the Touch Bar, and *the same on every Mac*.

https://discussions.apple.com/thread/7763102?start=0&tstart=0

Why I think that's problematic for this particular test is that whatever gets returned is going to be highly dependent on the hardware the test is run on, and it's entirely possible that the MAC address returned is indeed locally administered and not tied to a physical external (and thus required to be universally administered) MAC address.

Mocking _ifconfig_() probably isn't a good idea because these tests are worthless that way. But it's also true that the _ifconfig_() methods can match unexpected interfaces which cause the test to fail incorrectly. It's a mess, and I'm not sure what to do about it.

If your fix is correct (and it looks correct to me), we should fix not only the test, but, first of all, the _*_getnode functions. Perhaps they should ignore locally administered MAC addresses and continue searching better variants or fall back to other method.

On Nov 21, 2017, at 15:47, Serhiy Storchaka <report@bugs.python.org> wrote:

If your fix is correct (and it looks correct to me), we should fix not only the test, but, first of all, the _*_getnode functions. Perhaps they should ignore locally administered MAC addresses and continue searching better variants or fall back to other method.

I think it does make sense to ignore locally administered MACs from _getnode() and friends. E.g. there’s no use in returning the interface for the Touch Bar bridge :)

I don’t have time right now to elaborate on that, but I’ll work on improving the PR with that change.

https://travis-ci.org/python/cpython/jobs/305433725

AssertionError: 2199023255552 is not false : 0242ac110051
AssertionError: 2199023255552 is not false : 0242ac110051

So the Travis CI MAC address is 02:42:ac:11:00:51.

Wait, now I understand the purpose of the current test! There is just a typo in a comment, should be 41 instead of 47. The test itself is correct. If the address is obtained from network cards, it should have the 41 bit clear. If it is generated randomly, it should have the 41 bit set, to avoid conflicts with addresses obtained from network cards. There is nothing about the 42 bit.

On Nov 23, 2017, at 11:24, Serhiy Storchaka <report@bugs.python.org> wrote:

Wait, now I understand the purpose of the current test! There is just a typo in a comment, should be 41 instead of 47. The test itself is correct. If the address is obtained from network cards, it should have the 41 bit clear. If it is generated randomly, it should have the 41 bit set, to avoid conflicts with addresses obtained from network cards. There is nothing about the 42 bit.

It’s not just a typo in the comment, it’s a typo in the bitmask, at least if the intent of the test is to ensure that the MAC address is universally administered. That is defined as having a 0 in the “second least significant bit of the first octet”. Here’s a way to make that clearer.

# Second least significant bit of the first octet of the 48-bit MAC address
>>> 0b00000010_00000000_00000000_00000000_00000000_00000000
2199023255552
# Constant from the PR
>>> 1<<41
2199023255552
# Literal bit mask from the original code.
>>> 0x010000000000
1099511627776

The latter is off by 1 place.

I'm sorry, but this tests and the typo were added by me in bpo-23015. This check is a pair of the check for _random_getnode(). They use the same mask and test the same bit. I forgot about this.

If I understand correctly, the original problem was that tests failed in some environments, right? In that case we should either change the environment or just remove this check. I had added it in assumption that it is never failed for addresses obtained from network cards. If my assumption is wrong, it should be just removed.

On Nov 26, 2017, at 12:40, Serhiy Storchaka <report@bugs.python.org> wrote:

Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment:

If I understand correctly, the original problem was that tests failed in some environments, right? In that case we should either change the environment or just remove this check. I had added it in assumption that it is never failed for addresses obtained from network cards. If my assumption is wrong, it should be just removed.

I think your assumption is generally true in that *real* network cards (i.e. ethernet ports or wireless adapters) will always have universally administered MAC addresses. But what you probably didn’t expect is that certain environments can also have locally administered MAC addresses (e.g. MBP’s Touch Bar interfaces - that one surprised me too!) and some may *only* have local MACs (e.g. Travis).

We can fix the first assumption by preferentially returning universal MACs, and only return local MACs if there are no other interfaces available. My latest changes do this. (Remember, I only started seeing the failures on my 2017 MBP with Touch Bar.)

I can’t think of any way of solving the second one, short of just skipping those tests on Travis. I think any reasonable machine with real interfaces will have at least one universal MAC, so it’s just Travis being weird. My latest commit adds a skip for Travis. (That test is still running, so we’ll see.)

The original problem really was that the test for universal vs. local MAC address was using the incorrect bitmask. The comment was also wrong, so my changes fix both the comment and the bitmask.

Another problem I found was that the UUID1 spec says that if a random MAC is used as a fallback, then the multicast bit must be set, so my changes also do this.

For now, I’d prefer to take the narrower approach of enabling the tests everywhere but Travis. What I don’t know is whether any of the buildbots will fail in a similar way, but I’m hoping they’ll be okay. If we see buildbot failures because they also lack universal MACs, then yes, we’ll have to reevaluate whether the tests make sense, or whether we should mock out the environment at a lower level.

Universally/locally administered MAC addresses doesn't have relation to this issue. My assumption was about the multicast bit (1<<40).

If my assumption was correct, the test is correct, and only the comment should be fixed. If it was wrong, the test should be removed. That's all.

On Nov 26, 2017, at 14:07, Serhiy Storchaka <report@bugs.python.org> wrote:

Universally/locally administered MAC addresses doesn't have relation to this issue. My assumption was about the multicast bit (1<<40).

AFAICT, the multicast bit is only required to be set on random MAC.

If my assumption was correct, the test is correct, and only the comment should be fixed. If it was wrong, the test should be removed. That's all.

I don’t understand. Are you saying that _find_mac() should return whatever random MAC is found first, as the original code does? That doesn’t seem right either, since as the MacBook Pro case shows, it’s entirely possible to get a MAC address that is the same on every single laptop.

This is a different issue.

PR 4572 removes the wrong check.

PR bpo-4576 includes all related changes.

Currently PR 4576 combines a bug fix and a new feature.

1. Removes wrong check and adds a verbose clarifying comment. This part should be backported to maintained versions.

2. Adds multiple explicit "return None". This is a minor improvement the quality of the code. Too minor for separate PR.

3. Makes methods for getting the address of the real network card preferring universally administered addresses and falling back to locally administered address only if universally administered addresses are not found. This is a new feature. But are there computers that have both universally and locally administered addresses (and with locally administered addresses enumerated first)?

On Nov 27, 2017, at 01:24, Serhiy Storchaka <report@bugs.python.org> wrote:

3. Makes methods for getting the address of the real network card preferring universally administered addresses and falling back to locally administered address only if universally administered addresses are not found. This is a new feature. But are there computers that have both universally and locally administered addresses (and with locally administered addresses enumerated first)?

There are certainly computers that have both universal and local admin MAC addresses, as proven by the MBP+TouchBar case that triggered this. I don’t know whether we can assume anything about the order in which those are enumerated.

New changeset 9522a21 by Barry Warsaw in branch 'master':
bpo-32107 - Better merge of bpo-4494 (bpo-4576)
9522a21

My apologies for my dim comment and my obliviousness that leaded to spending your time on trying to fix a wrong issue.

Do you mind to fix the original issue (by removing the wrong check) in maintenance branches?

On Nov 27, 2017, at 15:18, Serhiy Storchaka <report@bugs.python.org> wrote:

My apologies for my dim comment and my obliviousness that leaded to spending your time on trying to fix a wrong issue.

No worries at all. Thanks for working with me to make the fix as good as it can be.

Do you mind to fix the original issue (by removing the wrong check) in maintenance branches?

I don’t mind, but interestingly enough, test_uuid does not fail for me locally in either the 2.7 or 3.6 branches! I don’t know what’s different, since I’m running them on exactly the same machine, but that does seem to be the case.

Still, I think it makes sense to backport the change in check, without backporting the feature change or code cleanups, so I’ll work on branches for those.

We shouldn't check the 42nd bit in maintenance releases. This check will fail on some computers.

On Nov 27, 2017, at 16:08, Serhiy Storchaka <report@bugs.python.org> wrote:

We shouldn't check the 42nd bit in maintenance releases. This check will fail on some computers.

Oh, without the preferential return of the universally administered MAC, you’re right. So we should just remove the check for the 41st bit and not add the check for the 42nd bit.

The latest checkin seems to have broken several buildbots:

http://buildbot.python.org/all/#builders/47/builds/243
http://buildbot.python.org/all/#builders/88/builds/248
http://buildbot.python.org/all/#builders/13/builds/255
http://buildbot.python.org/all/#builders/16/builds/252

On Nov 27, 2017, at 16:21, Ned Deily <report@bugs.python.org> wrote:

The latest checkin seems to have broken several buildbots:

http://buildbot.python.org/all/#builders/47/builds/243
http://buildbot.python.org/all/#builders/88/builds/248
http://buildbot.python.org/all/#builders/13/builds/255
http://buildbot.python.org/all/#builders/16/builds/252

Those are all locally administered MAC addresses. Does that mean some of the buildbot machines also have no universally administered MAC address, like Travis? Or is something wrong with our logic?

If it’s the former, then it might just make better sense to remove the 42nd bit test in all cases.

http://buildbot.python.org/all/#/builders/16/builds/252

======================================================================
FAIL: test_ifconfig_getnode (test.test_uuid.TestInternalsWithoutExtModule)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-sles-z/build/Lib/test/test_uuid.py", line 538, in test_ifconfig_getnode
    self.check_node(node, 'ifconfig')
  File "/home/dje/cpython-buildarea/3.x.edelsohn-sles-z/build/Lib/test/test_uuid.py", line 531, in check_node
    self.assertFalse(node & (1 << 41), '%012x' % node)
AssertionError: 2199023255552 is not false : 020000000030

======================================================================
FAIL: test_ip_getnode (test.test_uuid.TestInternalsWithoutExtModule)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-sles-z/build/Lib/test/test_uuid.py", line 543, in test_ip_getnode
    self.check_node(node, 'ip')
  File "/home/dje/cpython-buildarea/3.x.edelsohn-sles-z/build/Lib/test/test_uuid.py", line 531, in check_node
    self.assertFalse(node & (1 << 41), '%012x' % node)
AssertionError: 2199023255552 is not false : 020000000030

New changeset c9409f7 by Victor Stinner in branch 'master':
Revert "bpo-32107 - Better merge of bpo-4494 (bpo-4576)" (bpo-4593)
c9409f7

The commit 9522a21 broke multiple buildbots. Since I don't how to fix it, I reverted it, to get more time to design, implement and test a proper fix.

On Nov 27, 2017, at 18:38, STINNER Victor <report@bugs.python.org> wrote:

STINNER Victor <victor.stinner@gmail.com> added the comment:

The commit 9522a21 broke multiple buildbots. Since I don't how to fix it, I reverted it, to get more time to design, implement and test a proper fix.

Thanks. There was no way to predict the failures, but I thought that it could happen. I think at this point the only proper fix is to remove the bitmask test. I’ll work up a follow-on branch and port that back to 2.7 and 3.6.

New changeset 23df2d1 by Barry Warsaw in branch 'master':
bpo-32107 - Improve MAC address calculation and fix test_uuid.py (bpo-4600)
23df2d1

New changeset 56e444f by Barry Warsaw in branch '2.7':
[2.7] bpo-32107 - Backport bitmask check fix (GH-4576) (bpo-4590)
56e444f

New changeset 23cc8c0 by Barry Warsaw in branch '3.6':
[3.6] bpo-32107 - Backport bitmask check fix (GH-4576) (bpo-4591)
23cc8c0

test_uuid fails now on android-24-armv7 on the master branch:

======================================================================
FAIL: test_getnode (test.test_uuid.TestUUIDWithoutExtModule)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/sdcard/org.python/lib/python3.7/test/test_uuid.py", line 312, in test_getnode
    self.assertEqual(node1, node2, '%012x != %012x' % (node1, node2))
AssertionError: 237015144408656 != 105397654869517 : d790637d2650 != 5fdbcdc7560d

Some context:

• There is no _uuid extension module.
• All the getters in uuid.getnode() fail: _ip_getnode() fails because the 'ip link list' command fails on Android while 'ip link' would have succeeded (and would have hidden the above bug), 'ifconfig' does not print MAC addresses and the commands of the other getters do not exist.

The following patch fixes the problem:

diff --git a/Lib/uuid.py b/Lib/uuid.py
index cb2bc092bd..be06a6eff3 100644
--- a/Lib/uuid.py
+++ b/Lib/uuid.py
@@ -674,14 +674,14 @@ def getnode():
         getters = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
                    _arp_getnode, _lanscan_getnode, _netstat_getnode]
 
-    for getter in getters:
+    for getter in getters + [_random_getnode]:
         try:
             _node = getter()
         except:
             continue
         if _node is not None:
             return _node
-    return _random_getnode()
+    assert False, '_random_getnode() returned None'

I don't know how the proposed change can help.

PR 4677 makes the test more lenient.

I don't know how the proposed change can help.

It helps by fixing the regression introduced by a change made in 23df2d1 in this issue, a change that has been made only in the master branch. Before that change, successive calls to getnode() returned the same value on Android. The proposed PR 4677 does not fix that regression and hides that regression instead in the test suite.

The fact that getnode() should always return the same value or should not, must be discussed in another issue in the case where that point needs to be raised.

Sorry, but I can't understand how 23df2d1 could break getnode() on Android and how your changes can fix this. The only effect of this change is that an error in _random_getnode() no longer silenced. But this is not the case, because if _random_getnode() raises an exception you can't get two different results (you can't get even the single result) for comparison.

Sorry, but I can't understand how 23df2d1 could break getnode() on Android and how your changes can fix this. The only effect of this change is that an error in _random_getnode() no longer silenced. But this is not the case, because if _random_getnode() raises an exception you can't get two different results (you can't get even the single result) for comparison.

I concur. Your test appears to be failing because it wants two successive calls to getnode() to return the same value. But the proposed change doesn’t change the fact that if you fall back to _random_getnode() you’re going to get a different random value every time. I think you need to look into the problem more deeply. (It doesn’t help that the code swallows all exceptions from the other getters - I pointed that out in my previous PRs. I’ll bet the clue to your problem is being hidden by that bare except.)

1. Before the regression made by 23df2d1, on the first invocation of getnode(), the returned value is cached in '_node' (a global variable) and getnode() is idempotent.

2. After 23df2d1, the returned value is not cached in '_node' when it is obtained through _random_getnode() and getnode() returns different values each time in that case.

Not sure how you can miss that point :-(

Ah, good catch! I missed this. Do you mind to create a PR Xavier?

When I am the author of a regression (as I have been too many times unfortunately), I prefer to be the one that reverts the changes and I would not like someone else to do it.

I missed it too. Serhiy, do you want to add that fix to your already open PR 4677?

New changeset e69fbb6 by Serhiy Storchaka in branch 'master':
Fix a regression in uuid added in bpo-32107. (bpo-4677)
e69fbb6

I think this issue is resolved, right? Closing. Please reopen if there's anything left to do.