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

py_getrandom() uses an int for syscall() result #71465

Closed
vstinner opened this issue Jun 9, 2016 · 8 comments
Closed

py_getrandom() uses an int for syscall() result #71465

vstinner opened this issue Jun 9, 2016 · 8 comments

Comments

@vstinner
Copy link
Member

vstinner commented Jun 9, 2016

BPO 27278
Nosy @vstinner, @vadmium

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-06-16.22:04:15.922>
created_at = <Date 2016-06-09.07:42:45.893>
labels = []
title = 'py_getrandom() uses an int for syscall() result'
updated_at = <Date 2016-06-16.22:04:15.921>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2016-06-16.22:04:15.921>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2016-06-16.22:04:15.922>
closer = 'vstinner'
components = []
creation = <Date 2016-06-09.07:42:45.893>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 27278
keywords = []
message_count = 8.0
messages = ['267969', '267986', '267989', '268559', '268560', '268595', '268702', '268703']
nosy_count = 3.0
nosy_names = ['vstinner', 'python-dev', 'martin.panter']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue27278'
versions = ['Python 3.5', 'Python 3.6']

@vstinner
Copy link
Member Author

vstinner commented Jun 9, 2016

syscall() result type is long.

Moreover, long type might can smaller than the size type, so we may need to add:

n = Py_MIN(size, LONG_MAX);

@vadmium
Copy link
Member

vadmium commented Jun 9, 2016

According to <http://man7.org/linux/man-pages/man2/getrandom.2.html\>, getrandom() returns no more than 32 MiB as an int on Linux. Doesn’t that mean you can rely on syscall()’s long return value fitting in an int? Maybe just cast n = (int)sycall(...) to be explicit.

But it does make sense to cap n to LONG_MAX just in case there is some strange platform where it matters :)

@vadmium
Copy link
Member

vadmium commented Jun 9, 2016

Make that INT_MAX. Or change n from an int to a Py_ssize_t. Both Linux and Solaris versions or getrandom() are documented as accepting size_t buflen.

@python-dev
Copy link
Mannequin

python-dev mannequin commented Jun 14, 2016

New changeset e028e86a5b73 by Victor Stinner in branch '3.5':
Fix os.urandom() using getrandom() on Linux
https://hg.python.org/cpython/rev/e028e86a5b73

New changeset 0d39bd9028e8 by Victor Stinner in branch 'default':
Merge 3.5 (os.urandom, issue bpo-27278)
https://hg.python.org/cpython/rev/0d39bd9028e8

@vstinner
Copy link
Member Author

Martin: What do you think of my change? Is it enough? Or would you prefer an explicit cast on syscall() result?

I hesitated to use a wider type since the manual page shows an "int" type, not long.

@vadmium
Copy link
Member

vadmium commented Jun 15, 2016

Yeah I think your change is enough.

Adding a cast would solve this compiler warning:

Python/random.c:168:17: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
n = syscall(SYS_getrandom, dest, n, flags);
^

But on the other hand, when I just recompiled Python with -Wconversion, I got hundreds of other warnings, so maybe there is a good reason we don’t enable that warning.

@python-dev
Copy link
Mannequin

python-dev mannequin commented Jun 16, 2016

New changeset 193f50babfa4 by Victor Stinner in branch '3.5':
py_getrandom(): use long type for the syscall() result
https://hg.python.org/cpython/rev/193f50babfa4

@vstinner
Copy link
Member Author

Adding a cast would solve this compiler warning:

I changed the code to use the long type. It should fix the warning, even if it was no more a real bug: the original bug was already fixed.

I close the issue, it's now solved.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants