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

The pwd module implementation incorrectly sets some attributes to None #76214

Closed
xdegaye mannequin opened this issue Nov 15, 2017 · 13 comments
Closed

The pwd module implementation incorrectly sets some attributes to None #76214

xdegaye mannequin opened this issue Nov 15, 2017 · 13 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@xdegaye
Copy link
Mannequin

xdegaye mannequin commented Nov 15, 2017

BPO 32033
Nosy @vstinner, @tiran, @xdegaye, @serhiy-storchaka, @ZackerySpytz, @miss-islington
PRs
  • bpo-32033: Fix test_pwd failures on Android #19502
  • [3.8] bpo-32033: Fix test_pwd failures on Android (GH-19502) #19518
  • [3.7] bpo-32033: Fix test_pwd failures on Android (GH-19502) #19519
  • bpo-32033: Finalize WASI configure options (GH-32053) #32053
  • 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 2020-04-14.18:16:47.711>
    created_at = <Date 2017-11-15.10:34:19.480>
    labels = ['extension-modules', '3.8', 'type-bug', '3.7', '3.9']
    title = 'The pwd module implementation incorrectly sets some attributes to None'
    updated_at = <Date 2022-03-22.17:42:26.595>
    user = 'https://github.com/xdegaye'

    bugs.python.org fields:

    activity = <Date 2022-03-22.17:42:26.595>
    actor = 'christian.heimes'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-04-14.18:16:47.711>
    closer = 'vstinner'
    components = ['Extension Modules']
    creation = <Date 2017-11-15.10:34:19.480>
    creator = 'xdegaye'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32033
    keywords = ['patch']
    message_count = 13.0
    messages = ['306261', '306262', '306263', '306286', '306290', '306707', '306715', '306729', '366418', '366420', '366425', '366426', '415800']
    nosy_count = 6.0
    nosy_names = ['vstinner', 'christian.heimes', 'xdegaye', 'serhiy.storchaka', 'ZackerySpytz', 'miss-islington']
    pr_nums = ['19502', '19518', '19519', '32053']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue32033'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Nov 15, 2017

    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

    @xdegaye xdegaye mannequin added tests Tests in the Lib/test dir 3.7 (EOL) end of life extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Nov 15, 2017
    @vstinner
    Copy link
    Member

    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)

    @vstinner
    Copy link
    Member

    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.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Nov 15, 2017

    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.

    @xdegaye xdegaye mannequin removed the tests Tests in the Lib/test dir label Nov 15, 2017
    @xdegaye xdegaye mannequin changed the title The pwd test test_values fails on Android The pwd module implementation incorrectly sets some attributes to None Nov 15, 2017
    @serhiy-storchaka
    Copy link
    Member

    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).

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Nov 22, 2017

    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.

    @serhiy-storchaka
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

    And at least for pw_shell the behavior for NULL and "" are different.

    What is the difference between the two?

    @ZackerySpytz ZackerySpytz mannequin added 3.8 only security fixes 3.9 only security fixes labels Apr 13, 2020
    @vstinner
    Copy link
    Member

    New changeset 96515e9 by Zackery Spytz in branch 'master':
    bpo-32033: Fix test_pwd failures on Android (GH-19502)
    96515e9

    @vstinner
    Copy link
    Member

    It's now fixed in master and backports to 3.7 and 3.8 will be merged as soon as the CI pass.

    @miss-islington
    Copy link
    Contributor

    New changeset 1e1dbdf by Miss Islington (bot) in branch '3.8':
    bpo-32033: Fix test_pwd failures on Android (GH-19502)
    1e1dbdf

    @miss-islington
    Copy link
    Contributor

    New changeset 8821200 by Miss Islington (bot) in branch '3.7':
    bpo-32033: Fix test_pwd failures on Android (GH-19502)
    8821200

    @tiran
    Copy link
    Member

    tiran commented Mar 22, 2022

    New changeset 4aea656 by Christian Heimes in branch 'main':
    bpo-32033: Finalize WASI configure options (GH-32053)
    4aea656

    @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
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants