py_getrandom() uses an int for syscall() result

[vstinner]

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]

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]

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]

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]

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]

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]

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]

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]

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.