gh-98415: Fix UUID ifconfig MAC address detection

[csanders-git]

See #98415 for explanation

• Issue: UUID _ifconfig_getnodes() will always return None due to broken logic #98415

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[corona10]

Nice catch!

I reproduce the issue with the following script:

import subprocess
"""Get the hardware address on Unix by running ifconfig."""
# This works on Linux ('' or '-a'), Tru64 ('-av'), but not all Unixes.

def foo(*args):
    command = ("/sbin/ifconfig",) + args
    proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL)
    if not proc:
        return
    stdout, stderr = proc.communicate()
    mac = stdout # This is fine for this example (normally it'd have parsed out a mac
    if not mac:
        print(args, mac, stderr)


for args in ('', '-a', '-av'):
    foo(args)

[bedevere-bot]

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.

[vstinner]

I have a preference for (executable, *args) syntax when building command line arguments :-) IMO it's more readable.

[vstinner]

Would it be possible to write an unit test with unittest.mock to check that subprocess is not called with an empty string argument?

[corona10]

Would it be possible to write an unit test with unittest.mock to check that subprocess is not called with an empty string argument?

It looks hard to write the unittest due to system-specific implementation.
I would like to skip the writing test code at this moment,

[miss-islington]

Thanks @csanders-git for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

[miss-islington]

Sorry @csanders-git and @vstinner, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker e3ec272f57c3948834a6159cf2604978d3db67a0 3.11

[bedevere-bot]

GH-99020 is a backport of this pull request to the 3.10 branch.

[miss-islington]

Thanks @csanders-git for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-99021 is a backport of this pull request to the 3.11 branch.

[vstinner]

Ok, thanks @csanders-git for the fix: I merged it. I rewrote the commit message to clarify its scope.

[vstinner]

@corona10:

It looks hard to write the unittest due to system-specific implementation.
I would like to skip the writing test code at this moment,

It makes me sad, but I'm fine with not adding a test :-)

[vstinner]

In the past, I fixed a risk of shell injection by avoiding a shell in the uuid module: commit b9d0199. I didn't add any test for that neither :-(