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

Fix possible double close and use of unopened socket #825

Merged
merged 2 commits into from
Sep 22, 2016
Merged

Fix possible double close and use of unopened socket #825

merged 2 commits into from
Sep 22, 2016

Conversation

hardikar
Copy link
Contributor

  • On line 407, I assume the code should jump to error if either value is -1 or python error-ed, and not when both.
  • According to http://linux.die.net/man/3/socket, Upon successful completion, socket() shall return a non-negative integer, the socket file descriptor. Otherwise, a value of -1 shall be returned and errno set to indicate the error.. Thus on line 540, 0 is a valid socket fd - and the check should be against -1
  • If py_retlist > 1, i.e Py_BuildValue returned an error, the code will jump to error and double close the socket.

@coveralls
Copy link

coveralls commented May 24, 2016

Coverage Status

Coverage remained the same at 94.48% when pulling b856d85 on hardikar:master into e738353 on giampaolo:master.

close(sock);
error_after_close:
Py_XDECREF(py_is_up);
Copy link
Owner

Choose a reason for hiding this comment

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

I kinda don't like having two separate goto handlers. I think close()sing the socket twice will not be a problem,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, once a socket is closed - the file descriptor is available for use elsewhere. And thus a double close could end up closing that - which may cause a problem elsewhere. If you don't like multiple goto handlers, you might prefer moving the close(sock) on line 541 down to 545 before the return. That way there is a close in every path in the function - and no double close.

Copy link
Owner

@giampaolo giampaolo Jun 2, 2016

Choose a reason for hiding this comment

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

I like that. Can you update the PR also including changes for the HISTORY.rst file?

@hardikar
Copy link
Contributor Author

@giampaolo Sorry for getting back to this after so long - have look at the updated diff.

@@ -19,6 +19,7 @@ Bug tracker at https://github.com/giampaolo/psutil/issues
- #797: [Linux] net_if_stats() may raise OSError for certain NIC cards.
- #813: Process.as_dict() should ignore extraneous attribute names which gets
attached to the Process instance.
- #825: Fix possible double close and use of unopened socket
Copy link
Owner

Choose a reason for hiding this comment

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

Please specify the platform ([Linux]) and the function which was affected (the high level, end-user psutil function, not the C function)

@giampaolo
Copy link
Owner

Just fix this https://github.com/giampaolo/psutil/pull/825/files#r66893740 and update CREDITS file then it's good to go.

@giampaolo giampaolo merged commit 95aea67 into giampaolo:master Sep 22, 2016
@giampaolo giampaolo added the bug label Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants