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
Appearance settings

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

Merged
miss-islington merged 2 commits into
python:masterpython/cpython:masterfrom
gpshead:coverity_posixmodgpshead/cpython:coverity_posixmodCopy head branch name to clipboard
Jun 20, 2020
Merged

bpo-41056: Use the fildes converter for fd to please Coverity.#21011
miss-islington merged 2 commits into
python:masterpython/cpython:masterfrom
gpshead:coverity_posixmodgpshead/cpython:coverity_posixmodCopy head branch name to clipboard

Conversation

@gpshead

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

Copy link
Copy Markdown
Member

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

miss-islington commented Jun 20, 2020

Copy link
Copy Markdown
Contributor

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

@miss-islington

miss-islington commented Jun 20, 2020

Copy link
Copy Markdown
Contributor

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

miss-islington commented Jun 20, 2020

Copy link
Copy Markdown
Contributor

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

tiran commented Jun 20, 2020

Copy link
Copy Markdown
Member

@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

serhiy-storchaka commented Jun 20, 2020

Copy link
Copy Markdown
Member

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

gpshead commented Jun 21, 2020

Copy link
Copy Markdown
Member Author

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

serhiy-storchaka commented Jun 22, 2020

Copy link
Copy Markdown
Member

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.