Skip to content

bpo-41056: Use the fildes converter for fd to please Coverity. #21011

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.

Sign up for GitHub

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

Jump to bottom
Merged
miss-islington merged 2 commits into from
Jun 20, 2020
Merged

bpo-41056: Use the fildes converter for fd to please Coverity. #21011

miss-islington merged 2 commits into from
Jun 20, 2020

Conversation

@gpshead
Copy link
Member

@gpshead gpshead commented Jun 20, 2020 •
edited by miss-islington
Loading

There are a bunch of other fd: int uses in this file, I expect many if not
all of them would be better off using the fildes converter. This particular
one was flagged by Coverity as it presumably flags fpathconf as not accepting
negative fds. I'd expect the other fd's to have been flagged as well
otherwise.

I'm marking this one as skip news as it really is a no-op.

https://bugs.python.org/issue41056

Automerge-Triggered-By: @tiran

@gpshead
Use the fildes converter for fd to please Coverity.
fafe2c2 
there are a bunch of other fd: int uses in this file, I expect many if not
all of them would be better off using the fildes converter.  This particular
one was flagged by Coverity as it presumably flags fpathconf as not accepting
negative fds.  I'd expect the other fd's to have been flagged as well
otherwise.
@gpshead gpshead added type-bug An unexpected behavior, bug, or error needs backport to 3.8 labels Jun 20, 2020
@gpshead gpshead requested a review from serhiy-storchaka June 20, 2020 18:37
@miss-islington miss-islington merged commit 3ccb96c into python:master Jun 20, 2020
@miss-islington
Copy link
Contributor

miss-islington commented Jun 20, 2020

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Jun 20, 2020

Sorry, @gpshead, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 3ccb96c9782480e5ce646a4a130569fb92f2965d 3.9

@miss-islington miss-islington self-assigned this Jun 20, 2020
@miss-islington
Copy link
Contributor

miss-islington commented Jun 20, 2020

Sorry @gpshead, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 3ccb96c9782480e5ce646a4a130569fb92f2965d 3.8

@tiran
Copy link
Member

tiran commented Jun 20, 2020

@gpshead Coverity is generally very picky when it comes to negative ints in file functions. I tend to flag them as invalid because they are harmless and Coverity does not understand our safe guards.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jun 20, 2020

Using the fildes convertor causes the following behavior changes:

  1. Negative integers are rejected.
  2. Accepted objects with the fileno() method.
  3. Objects with the __index__() method are no longer accepted.

I think it is a new feature, and therefore it should not be backported. Before merging it we perhaps need to add support of objects with the __index__() in the convertor to avoid regression.

You can just add an explicit check for negative fd.

@gpshead
Copy link
Member Author

gpshead commented Jun 21, 2020

I'm wondering why anyone would ever pass an object with an __index__ method in as a file descriptor anywhere? os module functions are low level APIs, they take an integer file descriptor. Many already use the fildes conversion, some still use int. Given they're all file descriptors they should probably all use the same fildes conversion unless a particular API ascribes additional meaning to the value such as accepting negative numbers as meaning something on input. I expect that to be rare to non-existent.

anyways agreed about not backporting; this wasn't a major bug given it is the os module.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jun 22, 2020

Would you mind to add to the NEWS entry that os.fpathconf() now accepts objects with the fileno() method?

fasih pushed a commit to fasih/cpython that referenced this pull request Jun 29, 2020
@gpshead @fasihahmad
bpo-41056: Use the fildes converter for fd to please Coverity. (pytho…
1b96d4a 
…nGH-21011)

There are a bunch of other fd: int uses in this file, I expect many if not
all of them would be better off using the fildes converter.  This particular
one was flagged by Coverity as it presumably flags fpathconf as not accepting
negative fds.  I'd expect the other fd's to have been flagged as well
otherwise.

I'm marking this one as skip news as it really is a no-op.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiran tiran tiran approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

@miss-islington miss-islington

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gpshead @miss-islington @tiran @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot