Skip to content
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

bpo-39184: Add audit events to functions in `fcntl`, `msvcrt`, `os`, `resource`, `shutil`, `signal`, `syslog` #18407

Merged
merged 4 commits into from Feb 13, 2020

Conversation

@gousaiyang
Copy link
Contributor

gousaiyang commented Feb 7, 2020

Add audit events to the following functions:

  • all functions in fcntl and syslog
  • file operations in msvcrt, os and shutil
  • os.putenv/unsetenv
  • os.add_dll_directory
  • os.fork/forkpty
  • os.kill/killpg
  • resource.setrlimit, resource.prlimit
  • signal.pthread_kill

https://bugs.python.org/issue39184

…`resource`, `shutil`, `signal`, `syslog`
@gousaiyang gousaiyang requested a review from python/windows-team as a code owner Feb 7, 2020
@gousaiyang gousaiyang requested review from zooba and removed request for python/windows-team Feb 7, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 8, 2020

Codecov Report

Merging #18407 into master will increase coverage by 1.08%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #18407       +/-   ##
===========================================
+ Coverage   82.11%   83.19%    +1.08%     
===========================================
  Files        1954     1570      -384     
  Lines      583559   414425   -169134     
  Branches    44429    44428        -1     
===========================================
- Hits       479190   344783   -134407     
+ Misses      94719    59995    -34724     
+ Partials     9650     9647        -3     
Impacted Files Coverage Δ
Lib/_weakrefset.py 59.18% <0.00%> (-0.69%) ⬇️
Lib/test/test_fractions.py 96.70% <0.00%> (-0.28%) ⬇️
Python/codecs.c
Objects/sliceobject.c
Modules/termios.c
Modules/clinic/itertoolsmodule.c.h
Modules/_decimal/libmpdec/numbertheory.c
Modules/md5module.c
Include/internal/pycore_ceval.h
Modules/_posixsubprocess.c
... and 377 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2844336...631f22b. Read the comment docs.

Copy link
Member

zooba left a comment

Overall, looks great! I checked all the events against the doc updates, and they all seem to be in good spots in the code.

We definitely should fix the PyLong_FromLong calls to avoid reference leaks.

I'm also 99% sure that we don't need to check path->object for NULL, but if you come back and tell me that we need to then I'll believe you :)

Modules/fcntlmodule.c Outdated Show resolved Hide resolved
Modules/fcntlmodule.c Outdated Show resolved Hide resolved
Modules/fcntlmodule.c Outdated Show resolved Hide resolved
Modules/fcntlmodule.c Outdated Show resolved Hide resolved
@@ -3007,6 +3015,13 @@ os_chmod_impl(PyObject *module, path_t *path, int mode, int dir_fd,
return NULL;
#endif

if (PySys_Audit("os.chmod", "OiO",
path->object ? path->object : Py_None, mode,
dir_fd == DEFAULT_DIR_FD ? Py_None : PyLong_FromLong(dir_fd)

This comment has been minimized.

Copy link
@zooba

zooba Feb 12, 2020

Member

We may just need to pick an "obvious" value to pass rather than None here... it's a shame that DEFAULT_DIR_FD is so arbitrary.

I'd rather not have to manage an object's lifetime around the PySys_Audit call. Passing 0 or -1 (assuming those aren't valid FDs? Are they?) would be obvious enough to me.

This comment has been minimized.

Copy link
@gousaiyang

gousaiyang Feb 13, 2020

Author Contributor

According to documentation from Linux, FreeBSD, macOS and Microsoft C Run-Time Library, file descriptors are non-negative integers, negative values are used to indicate errors, so here we can safely use -1 to indicate that dir_fd is not specified.

@@ -2911,6 +2911,11 @@ os_chdir_impl(PyObject *module, path_t *path)
{
int result;

if (PySys_Audit("os.chdir", "(O)",
path->object ? path->object : Py_None) < 0) {

This comment has been minimized.

Copy link
@zooba

zooba Feb 12, 2020

Member

Pretty sure path->object can't be NULL, right? Have you seen a case where this is not true?

I mean, there isn't another way to provide the path if you don't pass an object in...

This comment has been minimized.

Copy link
@zooba

zooba Feb 12, 2020

Member

I see there are a few existing cases where we check it like this, but AFAICT none of them should be necessary.

This comment has been minimized.

Copy link
@gousaiyang

gousaiyang Feb 13, 2020

Author Contributor

For path-like arguments with default values (like os.listdir and os.scandir), checking path->object for NULL is necessary, otherwise SystemError: NULL object passed to Py_BuildValue could happen. The audit hooks of os.listdir and os.scandir are already doing this.
For path-like arguments without default values, it seems that path->object could never be NULL since the argument is required. The audit hook in os.open does not check path->object for NULL, and the os_readlink_impl function actually assumed that path->object is not NULL (otherwise the PyUnicode_Check call will crash by dereferencing a null pointer).

This comment has been minimized.

Copy link
@zooba

zooba Feb 13, 2020

Member

Okay, that sounds good to me. Thanks!

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Feb 12, 2020

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.

- Do not use `PyLong_FromLong`
- Use -1 when `dir_fd` is not specified
- Remove unnecessary NULL checks
@gousaiyang

This comment has been minimized.

Copy link
Contributor Author

gousaiyang commented Feb 13, 2020

I have made the requested changes; please review again. Thanks!

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Feb 13, 2020

Thanks for making the requested changes!

@zooba: please review the changes made to this pull request.

@zooba
zooba approved these changes Feb 13, 2020
@zooba zooba merged commit 7514f4f into python:master Feb 13, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200213.7 succeeded
Details
bedevere/issue-number Issue number 39184 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Feb 13, 2020

Thanks @gousaiyang for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Feb 13, 2020

Sorry, @gousaiyang and @zooba, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 7514f4f 3.8

@gousaiyang gousaiyang deleted the gousaiyang:bpo-39184-file-misc branch Feb 13, 2020
zooba added a commit to zooba/cpython that referenced this pull request Feb 13, 2020
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Feb 13, 2020

GH-18500 is a backport of this pull request to the 3.8 branch.

zooba added a commit that referenced this pull request Feb 13, 2020
…`resource`, `shutil`, `signal`, `syslog` (GH-18407)

Co-authored-by: Saiyang Gou <gousaiyang@163.com>
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
petdance added a commit to petdance/cpython that referenced this pull request Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.