classification
Title: The pwd module implementation incorrectly sets some attributes to None
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ZackerySpytz, miss-islington, serhiy.storchaka, vstinner, xdegaye
Priority: normal Keywords: patch

Created on 2017-11-15 10:34 by xdegaye, last changed 2020-04-14 18:31 by miss-islington. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 19502 merged ZackerySpytz, 2020-04-13 14:47
PR 19518 merged miss-islington, 2020-04-14 18:13
PR 19519 merged miss-islington, 2020-04-14 18:16
Messages (12)
msg306261 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-11-15 10:34
On Android API 24:

$ python -c "import pwd; print(pwd.getpwuid(0))"
pwd.struct_passwd(pw_name='root', pw_passwd='', pw_uid=0, pw_gid=0, pw_gecos=None, pw_dir='/', pw_shell='/system/bin/sh')

The pw_gecos member is None and the test_values test of pwd fails because it expects a string. The fix is either (1) to skip the pw_gecos check in test_values for Android or (2) to modify the sets() function in Modules/pwdmodule.c to set an empty string instead of None when the member of the passwd structure is a NULL pointer.

POSIX [1] does not specify what are the possible values of the members of the struct passwd. GNU libc states that pw_dir and pw_shell may be NULL pointers so it seems that sets() is broken in these two cases.

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/getpwnam.html
[2] https://www.gnu.org/software/libc/manual/html_node/User-Data-Structure.html#User-Data-Structure
msg306262 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-15 10:47
> self.assertIsInstance(e.pw_gecos, str)

This test is wrong: it's perfectly fine to get None here.

Python must not test the OS itself, but only test our own code: make sure that Python converts properly C types to nice Python types, so a string or None.

I propose to use something like:

def check_type(field):
    self.assertTrue(field is None or isinstance(field, str), repr(field))

...
check_type(e.pw_gecos)
msg306263 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-15 10:49
Hum, I changed my mind a little bit :-)

> (2) to modify the sets() function in Modules/pwdmodule.c to set an empty string instead of None when the member of the passwd structure is a NULL pointer.

I checked the doc: pwd doesn't mention None at all :-(
https://docs.python.org/dev/library/pwd.html

For practical reasons, maybe (2) is nicer option. It would avoid to have all existing code just for Android.

I'm not sure that it's very useful to distinguish NULL and an empty char* string.
msg306286 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-11-15 17:08
> I'm not sure that it's very useful to distinguish NULL and an empty char* string.

I agree. An attribute of a ('pwd' Python module) password database entry corresponds to the field of a line in a 'passwd' text file. So it makes sense that when the field is empty in the text file, the corresponding attribute be an empty string and never None if it is not an integer (FWIW Android does not have a 'passwd' file).

Changing the title of the issue.
msg306290 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-15 17:41
I disagree. This is an old API, a thin wrapper around standard POSIX API, and returning an empty string instead of None will make impossible to distinguish NULL from "".

It is easy to convert None in an empty string in Python: `value or ''`.

I would change the test to

    if field is not None:
        self.assertIsInstance(field, str)

or

    self.assertIsInstance(field, (str, type(None)))

(I prefer the former variant).
msg306707 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-11-22 09:31
Changing test_pwd does not correct the fact that the current implementation of the pwd module may break an existing Python application since this (old indeed) API states "The uid and gid items are integers, all others are strings".

> returning an empty string instead of None will make impossible to distinguish NULL from "".

AFAIK in the 50 years since the creation of the unix operating system, there has never been an implementation of pwd that states that a string field may be either an empty string or NULL. And it is doubtful that there will ever be one, since this would break all (all, not just the Python applications) existing applications using pwd.
msg306715 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-22 10:10
On your second link it is documented explicitly that pw_dir and pw_shell might be NULL. And at least for pw_shell the behavior for NULL and "" are different.
msg306729 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-22 13:38
> And at least for pw_shell the behavior for NULL and "" are different.

What is the difference between the two?
msg366418 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-14 18:11
New changeset 96515e9f6785328c52ebc5d4ce60e0087a9adc2d by Zackery Spytz in branch 'master':
bpo-32033: Fix test_pwd failures on Android (GH-19502)
https://github.com/python/cpython/commit/96515e9f6785328c52ebc5d4ce60e0087a9adc2d
msg366420 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-14 18:16
It's now fixed in master and backports to 3.7 and 3.8 will be merged as soon as the CI pass.
msg366425 - (view) Author: miss-islington (miss-islington) Date: 2020-04-14 18:31
New changeset 1e1dbdf23f7a18f53a3257badc3541973831f2c4 by Miss Islington (bot) in branch '3.8':
bpo-32033: Fix test_pwd failures on Android (GH-19502)
https://github.com/python/cpython/commit/1e1dbdf23f7a18f53a3257badc3541973831f2c4
msg366426 - (view) Author: miss-islington (miss-islington) Date: 2020-04-14 18:31
New changeset 8821200d85657ef3bbec78dcb43694449c05e896 by Miss Islington (bot) in branch '3.7':
bpo-32033: Fix test_pwd failures on Android (GH-19502)
https://github.com/python/cpython/commit/8821200d85657ef3bbec78dcb43694449c05e896
History
Date User Action Args
2020-04-14 18:31:42miss-islingtonsetmessages: + msg366426
2020-04-14 18:31:07miss-islingtonsetmessages: + msg366425
2020-04-14 18:16:47vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg366420

stage: patch review -> resolved
2020-04-14 18:16:14miss-islingtonsetpull_requests: + pull_request18869
2020-04-14 18:13:00miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request18868
2020-04-14 18:11:50vstinnersetmessages: + msg366418
2020-04-13 14:49:04ZackerySpytzsetversions: + Python 3.8, Python 3.9, - Python 2.7, Python 3.6
2020-04-13 14:47:33ZackerySpytzsetkeywords: + patch
nosy: + ZackerySpytz

pull_requests: + pull_request18853
stage: needs patch -> patch review
2017-11-25 15:50:09xdegayelinkissue26856 dependencies
2017-11-22 13:38:13vstinnersetmessages: + msg306729
2017-11-22 10:10:40serhiy.storchakasetmessages: + msg306715
2017-11-22 09:31:05xdegayesetmessages: + msg306707
2017-11-15 17:41:29serhiy.storchakasetmessages: + msg306290
2017-11-15 17:08:30xdegayesettitle: The pwd test test_values fails on Android -> The pwd module implementation incorrectly sets some attributes to None
messages: + msg306286
components: - Tests
versions: + Python 2.7, Python 3.6
2017-11-15 10:49:52vstinnersetmessages: + msg306263
2017-11-15 10:47:04vstinnersetmessages: + msg306262
2017-11-15 10:34:19xdegayecreate