Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-98415: Fix UUID ifconfig MAC address detection#98423

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 5 commits intopython:mainfromcsanders-git:patch-1
Nov 2, 2022
Merged

gh-98415: Fix UUID ifconfig MAC address detection#98423

vstinner merged 5 commits intopython:mainfromcsanders-git:patch-1
Nov 2, 2022

Conversation

csanders-git
Copy link
Contributor

@csanders-gitcsanders-git commentedOct 19, 2022•
edited by bedevere-bot
Loading

@ghost
Copy link

ghost commentedOct 19, 2022•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Copy link
Member

@corona10corona10 left a comment

Choose a reason for hiding this comment

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

Nice catch!

I reproduce the issue with the following script:

importsubprocess"""Get the hardware address on Unix by running ifconfig."""# This works on Linux ('' or '-a'), Tru64 ('-av'), but not all Unixes.deffoo(*args):command= ("/sbin/ifconfig",)+argsproc=subprocess.Popen(command,stdout=subprocess.PIPE,stderr=subprocess.DEVNULL)ifnotproc:returnstdout,stderr=proc.communicate()mac=stdout# This is fine for this example (normally it'd have parsed out a macifnotmac:print(args,mac,stderr)forargsin ('','-a','-av'):foo(args)

@bedevere-bot
Copy link

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 phraseI 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.

@corona10corona10 added needs backport to 3.10only security fixes needs backport to 3.11only security fixes labelsOct 22, 2022
@corona10corona10 self-assigned thisOct 22, 2022
@vstinner
Copy link
Member

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

corona10 reacted with thumbs up emoji

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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,

@vstinnervstinner merged commite3ec272 intopython:mainNov 2, 2022
@miss-islington
Copy link
Contributor

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
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestNov 2, 2022
…98423)The uuid.getnode() function has multiple implementations, tested sequentially.The ifconfig implementation was incorrect and always failed: fix it.In practice, functions of libuuid library are preferred, if available:uuid_generate_time_safe(), uuid_create() or uuid_generate_time().(cherry picked from commite3ec272)Co-authored-by: Chaim Sanders <csanders-git@users.noreply.github.com>Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@bedevere-bot
Copy link

GH-99020 is a backport of this pull request to the3.10 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.10only security fixes labelNov 2, 2022
@vstinnervstinner added needs backport to 3.11only security fixes and removed needs backport to 3.11only security fixes labelsNov 2, 2022
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestNov 2, 2022
…98423)The uuid.getnode() function has multiple implementations, tested sequentially.The ifconfig implementation was incorrect and always failed: fix it.In practice, functions of libuuid library are preferred, if available:uuid_generate_time_safe(), uuid_create() or uuid_generate_time().(cherry picked from commite3ec272)Co-authored-by: Chaim Sanders <csanders-git@users.noreply.github.com>Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@bedevere-bot
Copy link

GH-99021 is a backport of this pull request to the3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11only security fixes labelNov 2, 2022
@vstinner
Copy link
Member

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

@vstinner
Copy link
Member

@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
Copy link
Member

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

miss-islington added a commit that referenced this pull requestNov 2, 2022
The uuid.getnode() function has multiple implementations, tested sequentially.The ifconfig implementation was incorrect and always failed: fix it.In practice, functions of libuuid library are preferred, if available:uuid_generate_time_safe(), uuid_create() or uuid_generate_time().(cherry picked from commite3ec272)Co-authored-by: Chaim Sanders <csanders-git@users.noreply.github.com>Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
miss-islington added a commit that referenced this pull requestNov 2, 2022
The uuid.getnode() function has multiple implementations, tested sequentially.The ifconfig implementation was incorrect and always failed: fix it.In practice, functions of libuuid library are preferred, if available:uuid_generate_time_safe(), uuid_create() or uuid_generate_time().(cherry picked from commite3ec272)Co-authored-by: Chaim Sanders <csanders-git@users.noreply.github.com>Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@csanders-gitcsanders-git deleted the patch-1 branchNovember 14, 2022 05:48
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner left review comments

@corona10corona10corona10 requested changes

Assignees

@vstinnervstinner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@csanders-git@bedevere-bot@vstinner@corona10@miss-islington
[8]ページ先頭
©2009-2025 Movatter.jp