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

uu.py calls os.path.chmod which doesn't exist #77868

Closed
bsdphk mannequin opened this issue May 29, 2018 · 18 comments
Closed

uu.py calls os.path.chmod which doesn't exist #77868

bsdphk mannequin opened this issue May 29, 2018 · 18 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@bsdphk
Copy link
Mannequin

bsdphk mannequin commented May 29, 2018

BPO 33687
Nosy @vstinner, @tiran, @bitdancer, @berkerpeksag, @vadmium, @serhiy-storchaka, @zhangyangyu, @timofurrer
PRs
  • bpo-33687: uu: fix call to os.chmod #7282
  • [3.7] bpo-33687: Fix call to os.chmod() in uu.decode() (GH-7282) #11592
  • 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 2019-01-17.14:35:37.533>
    created_at = <Date 2018-05-29.22:02:07.813>
    labels = ['3.7', '3.8', 'type-bug', 'library', 'easy']
    title = "uu.py calls os.path.chmod which doesn't exist"
    updated_at = <Date 2019-01-17.14:35:37.531>
    user = 'https://bugs.python.org/bsdphk'

    bugs.python.org fields:

    activity = <Date 2019-01-17.14:35:37.531>
    actor = 'berker.peksag'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-01-17.14:35:37.533>
    closer = 'berker.peksag'
    components = ['Library (Lib)']
    creation = <Date 2018-05-29.22:02:07.813>
    creator = 'bsdphk'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33687
    keywords = ['patch', 'easy']
    message_count = 18.0
    messages = ['318119', '318151', '318286', '318293', '318810', '318814', '318815', '319208', '319211', '319273', '319275', '319276', '321552', '321554', '321557', '333871', '333874', '333875']
    nosy_count = 9.0
    nosy_names = ['vstinner', 'christian.heimes', 'r.david.murray', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'bsdphk', 'xiang.zhang', 'tuxtimo']
    pr_nums = ['7282', '11592']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue33687'
    versions = ['Python 2.7', 'Python 3.7', 'Python 3.8']

    @bsdphk
    Copy link
    Mannequin Author

    bsdphk mannequin commented May 29, 2018

    Library file uu.py on at least 2.7 and 3.6 contains:

            try:
                os.path.chmod(out_file, mode)
            except AttributeError:
                pass
    

    As far as I can tell, os.path.chmod does not exist, so this always raises AttributeError and becomes a no-op.

    Is suspect the proper fix is to remove the ".path" ?

    @bsdphk bsdphk mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 29, 2018
    @serhiy-storchaka
    Copy link
    Member

    Looks as the proper fix to me.

    @serhiy-storchaka serhiy-storchaka added easy 3.7 (EOL) end of life 3.8 only security fixes labels May 30, 2018
    @serhiy-storchaka
    Copy link
    Member

    Would be nice to add a test.

    @timofurrer
    Copy link
    Mannequin

    timofurrer mannequin commented May 31, 2018

    I've added a test and updated the PR.

    @vstinner
    Copy link
    Member

    vstinner commented Jun 6, 2018

    Is the uu module still maintained? Christian Heimes wants to remove the module:
    https://github.com/tiran/peps/blob/oldbatteries/pep-9999.rst

    Xiang Zhang made a change in uu last year to add a new feature: new backtick optional parameter, bpo-30103.

    @bitdancer
    Copy link
    Member

    The email module uses it, so I would object to its being removed. It may not be used often (probably only when working with old email archives), but there's no good reason I can see to break something that currently works.

    @zhangyangyu
    Copy link
    Member

    I modified it for the feature, not maintain it. I remember at that time I get the feeling it's somewhat strange there are two APIs, with similar functionality resides in two modules, need to be updated.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 10, 2018

    What is your use case, Poul-Henning?

    It looks like the module has never set the file mode, at least since it was added to Python in 1994. I suggest just remove the dead code and fix the documentation. At least you shouldn’t make this change in bug fix releases. It is just as likely to break people’s code as fix it.

    If you do want to change the behaviour, I suggest using “os.open” to set the mode when the file is created. Otherwise you risk errors and messing up the filesystem when using special files e.g. uu.decode(..., "/dev/null").

    @vstinner
    Copy link
    Member

    It looks like the module has never set the file mode, at least since it was added to Python in 1994. I suggest just remove the dead code and fix the documentation.

    You cannot just remove the mode parameter without a deprecation period. Right now, the flag is documented:

    https://docs.python.org/dev/library/uu.html
    "mode is used to set the permission bits if the file must be created"

    I agree with Martin that I am not comfortable to fix the bug in stable branches (2.7, 3.6, 3.7).

    I'm +0 to fix the uu module in master (future 3.8).

    If we fix the bug in master, we should document that mode is ignored in 2.7, 3.6 and 3.7 (update their documentation).

    @bsdphk
    Copy link
    Mannequin Author

    bsdphk mannequin commented Jun 11, 2018

    I was just playing with it in a prototype and noticed that it didn't work.

    @serhiy-storchaka
    Copy link
    Member

    I concur with Martin. If this feature never worked, there is a risk of breaking user code when fix it. Let consider this as adding a new feature in 3.8. For older versions it should be documented that the mode of the output file is not set.

    And I agree that it would be more safe to use os.open(). Either call os.open() and pass the file descriptor to the builtin open() (but be careful to not leak it) or use the opener argument of open().

        def opener(path, flags):
            return os.open(path, flags, mode)
        fp = open(out_file, 'wb', opener=opener)

    It would be worth also to use the 'xb' opening mode instead of 'wb'.

    @serhiy-storchaka
    Copy link
    Member

    But 'xb' should be used only if out_file is not specified.

    @vstinner
    Copy link
    Member

    Is there really an use case which requires to set the permission of the file created by uu.decode()? It is already possible to call uu.decode() with an open file which has been created with the expected permission. Moreover, it's also possible to explicitly call chmod() *after* uu.decode().

    I would suggest to deprecate the mode parameter in Python 3.7 and remove it from Python 3.8.

    @bsdphk
    Copy link
    Mannequin Author

    bsdphk mannequin commented Jul 12, 2018

    Please note that the mode is not just a parameter, it is also a data field inside the encoded input.

    See: https://en.wikipedia.org/wiki/Uuencoding

    (search for "mode")

    @vstinner
    Copy link
    Member

    Please note that the mode is not just a parameter, it is also a data field inside the encoded input.

    Oh... right...

    "<mode> is the file's Unix file permissions as three octal digits (e.g. 644, 744). This is typically only significant to unix-like operating systems."

    Ok, in that case, it's maybe better to fix the bug instead of removing the parameter.

    @berkerpeksag
    Copy link
    Member

    New changeset 17f05bb by Berker Peksag (Timo Furrer) in branch 'master':
    bpo-33687: Fix call to os.chmod() in uu.decode() (GH-7282)
    17f05bb

    @berkerpeksag
    Copy link
    Member

    New changeset a261b73 by Berker Peksag (Miss Islington (bot)) in branch '3.7':
    bpo-33687: Fix call to os.chmod() in uu.decode() (GH-7282)
    a261b73

    @berkerpeksag
    Copy link
    Member

    Thank you for the report, Poul-Henning and thank you for the PR, Timo!

    @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 easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants