Skip to content

bpo-28009: for AIX correct uuid.getnode() and add ctypes() based uuid._generate_time_safe #5183

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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion Lib/test/test_uuid.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import contextlib
import io
import os
import sys
import shutil
import subprocess

Expand Down Expand Up @@ -490,10 +491,19 @@ class BaseTestInternals:

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
def test_find_mac(self):
data = '''
# issue28009 - AIX netstat -i(a) has specific formatting,
# i.e., '.' rather than ':' as format character
if not sys.platform.startswith("aix"):
data = '''
fake hwaddr
cscotun0 Link encap:UNSPEC HWaddr 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
eth0 Link encap:Ethernet HWaddr 12:34:56:78:90:ab
'''
else:
data = '''
fake hwaddr
cscotun0 Link encap:UNSPEC HWaddr 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
eth0 Link encap:Ethernet HWaddr 12.34.56.78.90.ab
'''

popen = unittest.mock.MagicMock()
Expand Down Expand Up @@ -523,6 +533,8 @@ def check_node(self, node, requires=None):

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
def test_ifconfig_getnode(self):
if sys.platform.startswith("aix"):
self.skipTest('AIX ifconfig does not provide MAC addr')
node = self.uuid._ifconfig_getnode()
self.check_node(node, 'ifconfig')

Expand All @@ -533,6 +545,8 @@ def test_ip_getnode(self):

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
def test_arp_getnode(self):
if sys.platform.startswith("aix"):
self.skipTest('AIX arp does not provide MAC addr')
node = self.uuid._arp_getnode()
self.check_node(node, 'arp')

Expand Down
79 changes: 65 additions & 14 deletions Lib/uuid.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,11 @@ def _is_universal(mac):
return not (mac & (1 << 41))

def _find_mac(command, args, hw_identifiers, get_index):
# issue28009: AIX uses character '.' rather than ':'
if sys.platform.startswith("aix"):
c_field = b'.'
else:
c_field = b':'
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Comments should only provide required info that's not apparent from the code/repo metadata. In particular, issue ref is only useful if the matter isn't resolved yet. Should be like: AIX <commands involved> use '.' as MAC delimiter
  • ternary form to reduce size & duplication and a more descriptive var name (e.g. mac_delim)

first_local_mac = None
try:
proc = _popen(command, *args.split())
Expand All @@ -373,7 +378,7 @@ def _find_mac(command, args, hw_identifiers, get_index):
if words[i] in hw_identifiers:
try:
word = words[get_index(i)]
mac = int(word.replace(b':', b''), 16)
mac = int(word.replace(c_field, b''), 16)
if _is_universal(mac):
return mac
first_local_mac = first_local_mac or mac
Expand Down Expand Up @@ -437,6 +442,23 @@ def _lanscan_getnode():
# This might work on HP-UX.
return _find_mac('lanscan', '-ai', [b'lan0'], lambda i: 0)

def _netstat_getmac_posix(words,i):
"""Extract the MAC address from netstat -ia from posix netstat -ia."""
word = words[i]
if len(word) == 17 and word.count(b':') == 5:
return(int(word.replace(b':', b''), 16))
else:
return None

def _netstat_getmac_aix(words,i):
"""Extract the MAC address from netstat -ia specific for AIX netstat."""
word = words[i]
wlen = len(word)
if wlen >= 11 and wlen <=17 and word.count(b'.') == 5:
return int(word.replace(b'.', b''), 16)
else:
return None

Copy link
Contributor

Choose a reason for hiding this comment

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

  • A great deal of duplication between the two routines. Can be consolidated. Heed the general CR comment.
  • Include a comment with samples of the text chunk being parsed in both cases so that the magic numbers make sense.
  • Reuse the above MAC delimiter var -- maybe as a global var?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the light of "functional differences better than names" -- probably detect the format instead?

def _netstat_getnode():
"""Get the hardware address on Unix by running netstat."""
# This might work on AIX, Tru64 UNIX.
Expand All @@ -454,12 +476,15 @@ def _netstat_getnode():
for line in proc.stdout:
try:
words = line.rstrip().split()
word = words[i]
if len(word) == 17 and word.count(b':') == 5:
mac = int(word.replace(b':', b''), 16)
if _is_universal(mac):
return mac
first_local_mac = first_local_mac or mac
if sys.platform.startswith("aix"):
mac = _netstat_getmac_aix(words,i)
else:
mac = _netstat_getmac_posix(words,i)
if not mac:
continue
if _is_universal(mac):
return mac
first_local_mac = first_local_mac or mac
except (ValueError, IndexError):
pass
except OSError:
Expand Down Expand Up @@ -573,12 +598,18 @@ def _load_system_functions():

# The uuid_generate_* routines are provided by libuuid on at least
# Linux and FreeBSD, and provided by libc on Mac OS X.
# uuid_create() is used by other POSIX platforms (e.g., AIX, FreeBSD)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Contradicts the existing statement just before
  • fns names are formatted differently

_libnames = ['uuid']
if not sys.platform.startswith('win'):
_libnames.append('c')
# also need to look for uuid_create() but only look if something was located
lib = None
for libname in _libnames:
libnm = ctypes.util.find_library(libname)
if not libnm:
continue
try:
lib = ctypes.CDLL(ctypes.util.find_library(libname))
lib = ctypes.CDLL(libnm)
except Exception: # pragma: nocover
continue
# Try to find the safe variety first.
Expand All @@ -601,6 +632,22 @@ def _generate_time_safe():
_uuid_generate_time(_buffer)
return bytes(_buffer.raw), None
break
# if _uuid_generate_time has not been found (Linux) then we try libc
# as libc is already dynamically linked (or found above) verify a valid
# lib value, then look for uuid_generate()
if not lib:
try:
lib = ctypes.CDLL(None)
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this supposed to work? None arg effect is undocumented. According to https://www.programcreek.com/python/example/1108/ctypes.CDLL , this is going to get the main program itself (in POSIX, at least. In Windows, it raises a TypeError) which doesn't have the required export (and probably any). Looks redundant.

except:
lib = None
if lib:
if hasattr(lib, 'uuid_create'): # pragma: nocover
_uuid_generate_time = lib.uuid_create
def _generate_time_safe():
_buffer = ctypes.create_string_buffer(16)# uuid
_status = ctypes.create_string_buffer(2) # c_ushort
_uuid_generate_time(_buffer, _status)
return bytes(_buffer.raw), bytes(_status.raw)
Copy link
Contributor

Choose a reason for hiding this comment

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

This only searches the last found lib, whatever it happens to be.

Better search uuid_create with other exports and save in a temporary var if found. After the loop, execute this code if uuid_create is found and others aren't.


# On Windows prior to 2000, UuidCreate gives a UUID containing the
# hardware address. On Windows 2000 and later, UuidCreate makes a
Expand Down Expand Up @@ -656,7 +703,12 @@ def _random_getnode():

_node = None

def getnode():
_NODE_GETTERS_WIN32 = [_windll_getnode, _netbios_getnode, _ipconfig_getnode]

_NODE_GETTERS_UNIX = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]

def getnode(*, getters=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't see the purpose of any changes from here and further.

About this one specifically:

  • There's no discussion about an interface change or justification for it.
  • In any case, the getters arg is unused as the code is now.

"""Get the hardware address as a 48-bit positive integer.

The first time this runs, it may launch a separate program, which could
Expand All @@ -669,19 +721,18 @@ def getnode():
return _node

if sys.platform == 'win32':
getters = [_windll_getnode, _netbios_getnode, _ipconfig_getnode]
getters = _NODE_GETTERS_WIN32
else:
getters = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]
getters = _NODE_GETTERS_UNIX

for getter in getters + [_random_getnode]:
try:
_node = getter()
except:
continue
if _node is not None:
if (_node is not None) and (0 <= _node < (1 << 48)):
return _node
assert False, '_random_getnode() returned None'
assert False, '_random_getnode() returned invalid value: {}'.format(_node)
Copy link
Contributor

Choose a reason for hiding this comment

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

Getters are supposed to return valid ids or None. Any violations that warrant rejecting the value received from the OS should be checked for by the getter itself.



_last_timestamp = None
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
* AIX uses the character '.' rather than ':' to seperate the MAC ADDR values
returned by 'netstat -i' command.
* The commands 'arp' and 'ifconfig' do not return a local MAC address
* define uuid._generate_time_safe in uuid.py based on ctypes()
lib.uuid_create (for when _uuid is not available)