Fix types for dev_t processing in posix module: use uint64_t (unsigned) instead of int64_t (signed)

@serhiy-storchaka

BPO	45767
Nosy	@serhiy-storchaka, @koobs, @AMDmi3
PRs	gh-89928: consume dev_t with PyLong_FromUnsignedLongLong #29494 gh-89928: Fix integer conversion of device numbers. #31794
Files	posixmodule.c.patch: Patch

^{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 = None
created_at = <Date 2021-11-09.17:13:09.157>
labels = ['extension-modules', 'type-bug', '3.9', '3.10', '3.11']
title = 'Fix types for dev_t processing in posix module'
updated_at = <Date 2022-03-10.14:54:37.748>
user = 'https://github.com/AMDmi3'

bugs.python.org fields:

activity = <Date 2022-03-10.14:54:37.748>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules', 'FreeBSD']
creation = <Date 2021-11-09.17:13:09.157>
creator = 'AMDmi3'
dependencies = []
files = ['50433']
hgrepos = []
issue_num = 45767
keywords = ['patch']
message_count = 5.0
messages = ['406030', '406051', '411022', '411036', '414851']
nosy_count = 3.0
nosy_names = ['serhiy.storchaka', 'koobs', 'AMDmi3']
pr_nums = ['29494', '31794']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue45767'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

So, I was investigating a test failure of python 3.11 and 3.10 on FreeBSD (but it likely applies to all python versions):

======================================================================
FAIL: test_makedev (test.test_posix.PosixTester)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/usr/ports/lang/python311/work/Python-3.11.0a1/Lib/test/test_posix.py", line 686, in test_makedev
    self.assertGreaterEqual(dev, 0)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: -5228656221359548839 not greater than or equal to 0

The test checks that posix.stat(somepath).st_dev >= 0, but negative value was returned.

Python uses PyLong_FromLongLong to convert from dev_t C type which st_dev is:

https://github.com/python/cpython/blob/main/Modules/posixmodule.c#L2410
https://github.com/python/cpython/blob/main/Modules/posixmodule.c#L901

POSIX does not seem to define signedness of dev_t,

https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/basedefs/sys_types.h.html

only saying it shall be an "integer type". However on practice on both FreeBSD and Linux it's unsigned:

FreeBSD:
typedef __dev_t dev_t; // sys/types.h
typedef __uint64_t __dev_t; // sys/_types.h

Linux (Ubuntu 18.04):
typedef __dev_t dev_t;             // sys/stat.h
__STD_TYPE __DEV_T_TYPE __dev_t;   // sys/types.h
#define __DEV_T_TYPE            __UQUAD_TYPE;  // sys/typesizes.h

So I suggest the attached patch to switch _PyLong_FromDev to PyLong_FromUnsignedLongLong which also makes it consistent with _Py_Dev_Converter which converts the other way with PyLong_AsUnsignedLongLong.

This change fixes the mentioned test, but another test failure is unmasked:

======================================================================
ERROR: test_makedev (test.test_posix.PosixTester)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/usr/ports/lang/python311/work/Python-3.11.0a1/Lib/test/test_posix.py", line 704, in test_makedev
    self.assertEqual(posix.makedev(major, minor), dev)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: Python int too large to convert to C int

This problem needs couple more trivial changes, but I'm not sure how to make them correctly (and I'm also confused by how this file relates with clinic/posixmodule.c).

The problems are that:

os_major_impl/os_minor_impl and os_makedev_impl are inconsistent in their argument/return types for major/minor dev numbers: the former use unsigned int, while the latter uses int.
the correct type is platform dependent, for instance Linux uses unsigned int and FreeBSD uses int (from man makedev).

I guess that to fix this failure one needs to add a macro/typedef for the type for minor/major dev numbers like

#if defined(__FreeBSD__)
#define DEVICE_MAJORMINOR_T int
#else
#define DEVICE_MAJORMINOR_T unsigned int
#endif

and use it in the named functions:

static DEVICE_MAJORMINOR_T os_major_impl(PyObject *module, dev_t device)
static DEVICE_MAJORMINOR_T os_minor_impl(PyObject *module, dev_t device)

static dev_t os_makedev_impl(PyObject *module, DEVICE_MAJORMINOR_T major, DEVICE_MAJORMINOR_T minor)

I closed the FreeBSD issue #113078 as a duplicate of this issue.

gh-89928: Fix integer conversion of device numbers (GH-31794)

pythongh-89928: Fix integer conversion of device numbers (pythonGH-31794

[3.12] pythongh-89928: Fix integer conversion of device numbers (pyth…

@serhiy-storchaka

Thanks for the fix @serhiy-storchaka!

[3.12] gh-89928: Fix integer conversion of device numbers (GH-31794) (G…

[3.13] gh-89928: Fix integer conversion of device numbers (GH-31794) (G…

pythongh-89928: Fix integer conversion of device numbers (pythonGH-31794

Navigation Menu

Fix types for dev_t processing in posix module: use uint64_t (unsigned) instead of int64_t (signed) #89928

Linked PRs

10 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Fix types for dev_t processing in posix module: use uint64_t (unsigned) instead of int64_t (signed) #89928

Description

Linked PRs

Activity

AMDmi3 commented on Nov 9, 2021

10 remaining items

vstinner commented on May 24, 2024

vstinner commented on Jun 4, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions