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

Hashlib/blake2* missing 'data' keyword argument #77910

Closed
JuusoLehtivarjo mannequin opened this issue Jun 1, 2018 · 22 comments
Closed

Hashlib/blake2* missing 'data' keyword argument #77910

JuusoLehtivarjo mannequin opened this issue Jun 1, 2018 · 22 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error

Comments

@JuusoLehtivarjo
Copy link
Mannequin

JuusoLehtivarjo mannequin commented Jun 1, 2018

BPO 33729
Nosy @gpshead, @vstinner, @tiran, @ned-deily, @serhiy-storchaka, @tirkarthi
PRs
  • bpo-33729: Fix issues with arguments parsing in hashlib. #8346
  • [3.7] bpo-33729: Fix issues with arguments parsing in hashlib. (GH-8346) #8581
  • [3.6] bpo-33729: Fix issues with arguments parsing in hashlib. (GH-8346) (GH-8581) #9657
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = None
    created_at = <Date 2018-06-01.08:36:11.487>
    labels = ['3.8', 'type-bug', '3.7', 'docs']
    title = "Hashlib/blake2* missing 'data' keyword argument"
    updated_at = <Date 2018-10-13.21:58:53.976>
    user = 'https://bugs.python.org/JuusoLehtivarjo'

    bugs.python.org fields:

    activity = <Date 2018-10-13.21:58:53.976>
    actor = 'ned.deily'
    assignee = 'serhiy.storchaka'
    closed = False
    closed_date = None
    closer = None
    components = ['Documentation']
    creation = <Date 2018-06-01.08:36:11.487>
    creator = 'Juuso Lehtivarjo'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33729
    keywords = ['patch']
    message_count = 22.0
    messages = ['318370', '321414', '321455', '321945', '321948', '321949', '321955', '322732', '322738', '322740', '322741', '322744', '322781', '322798', '327212', '327256', '327287', '327303', '327304', '327305', '327516', '327675']
    nosy_count = 8.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'christian.heimes', 'ned.deily', 'docs@python', 'serhiy.storchaka', 'Juuso Lehtivarjo', 'xtreak']
    pr_nums = ['8346', '8581', '9657']
    priority = None
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue33729'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @JuusoLehtivarjo
    Copy link
    Mannequin Author

    JuusoLehtivarjo mannequin commented Jun 1, 2018

    In python 3.6.5: hashlib blake2b/blake2s constructors do not recognize 'data' keyword. Try the following:

    from hashlib import blake2b
    print (blake2b(b"foobar").hexdigest()) # works
    print (blake2b(data=b"foobar").hexdigest()) # TypeError: 'data' is an invalid keyword argument for this function

    @JuusoLehtivarjo JuusoLehtivarjo mannequin added the type-crash A hard crash of the interpreter, possibly with a core dump label Jun 1, 2018
    @serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Jul 11, 2018
    @tiran
    Copy link
    Member

    tiran commented Jul 11, 2018

    None of the hashlib functions are taking keyword arguments for data:

    >>> hashlib.sha256(data=b'foo')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: openssl_sha256() takes no keyword arguments
    >>> hashlib.blake2b(data=b'foo')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: 'data' is an invalid keyword argument for this function

    @tiran tiran added 3.7 (EOL) end of life 3.8 only security fixes labels Jul 11, 2018
    @vstinner
    Copy link
    Member

    None of the hashlib functions are taking keyword arguments for data: ...

    So it's just a documentation issue, no?

    https://docs.python.org/dev/library/hashlib.html#creating-hash-objects

    Juuso Lehtivarjo: do you want to write a pull request to fix the documentation?

    @vstinner vstinner added the docs Documentation in the Doc dir label Jul 11, 2018
    @tirkarthi
    Copy link
    Member

    This was introduced as part of https://hg.python.org/cpython/rev/4969f6d343b1 . In addition to the signature there is also a line at https://docs.python.org/dev/library/hashlib.html#simple-hashing which could be removed

    As a shortcut, you can pass the first chunk of data to update directly to the constructor as the first argument (or as data keyword argument)

    Thanks

    @serhiy-storchaka
    Copy link
    Member

    hashlib.blake2b() and some other constructors accept the first chunk of data as the "string" keyword argument.

    >>> hashlib.blake2b(string=b'')
    <_blake2.blake2b object at 0x7f2847a9c430>
    >>> hashlib.blake2s(string=b'')
    <_blake2.blake2s object at 0x7f28468f6290>
    >>> hashlib.sha3_224(string=b'')
    <_sha3.sha3_224 object at 0x7f28468f6608>
    >>> hashlib.sha3_256(string=b'')
    <_sha3.sha3_256 object at 0x7f28468f6290>
    >>> hashlib.sha3_384(string=b'')
    <_sha3.sha3_384 object at 0x7f28468f6608>
    >>> hashlib.sha3_512(string=b'')
    <_sha3.sha3_512 object at 0x7f28468f6290>
    >>> hashlib.shake_128(string=b'')
    <_sha3.shake_128 object at 0x7f28468f6608>
    >>> hashlib.shake_256(string=b'')
    <_sha3.shake_256 object at 0x7f28468f6290>

    @tiran
    Copy link
    Member

    tiran commented Jul 19, 2018

    Please treat the first argument as positional-only argument. I don't want to standardize on 'string'.

    @serhiy-storchaka
    Copy link
    Member

    I take this issue because there are many other issues with handling arguments in the hashlib module.

    @serhiy-storchaka
    Copy link
    Member

    New changeset f1d36d8 by Serhiy Storchaka in branch 'master':
    bpo-33729: Fix issues with arguments parsing in hashlib. (GH-8346)
    f1d36d8

    @serhiy-storchaka
    Copy link
    Member

    New changeset 47957da by Serhiy Storchaka in branch '3.7':
    [3.7] bpo-33729: Fix issues with arguments parsing in hashlib. (GH-8346) (GH-8581)
    47957da

    @tiran
    Copy link
    Member

    tiran commented Jul 31, 2018

    Your PR changed way to many aspects of the code in one commit. I also don't like the fact, that you pushed such a big change without waiting for my feedback. For the past two weeks I have been travelling to conferences and had no time to review your PR. There was no need to rush it.

    @tiran
    Copy link
    Member

    tiran commented Jul 31, 2018

    The backport to 3.6 and 3.7 are breaking backwards compatibility and compatibility with PEP-247.

    @serhiy-storchaka
    Copy link
    Member

    Sorry. Do you prefer to revert the whole changes or just some parts?

    @vstinner
    Copy link
    Member

    I have no opinion on the change in the master branch, but I agree with Christian that the 3.7 change should be reverted since it breaks the backward compatibility.

    Serhiy modified int() in Python 3.7 to convert its first parameter to positional only parameter. IMHO it's ok to do such change. Moreover, the change has been documented in What's New in Python 3.7:
    https://docs.python.org/dev/whatsnew/3.7.html#api-and-feature-removals

    If you decide to keep the change in the master branch, it should be documented in What's New in Python 3.8, no?

    @serhiy-storchaka
    Copy link
    Member

    In case of int() the name of it's first argument was documented, in both the module documentation, and in interactive help. But the documented name of the first blake2b() argument was "data", and it never worked. Since help() was not worked for blake2b, the user had weak chance to know that the actual name is "string". Thus there is much less chance of breaking the user code by making this parameter a positional-only than in case of int().

    But I think Christian has other concerns.

    @ned-deily
    Copy link
    Member

    What's the status of this issue for 3.7 and for 3.6? Is everyone OK with what is currently in 3.7, i.e. no revert needed or has it already been reverted elsewhere? Also, there is the open PR for 3.6.

    @tiran
    Copy link
    Member

    tiran commented Oct 6, 2018

    Ned, I'm currently travelling until next weekend. The PR is rather large and I don't have any means or time to review it properly. Perhaps Gregory or Dmitry Chestnykh (original author of pyblake2) are able to assist.

    @ned-deily
    Copy link
    Member

    OK. This is now blocking the release of 3.7.1rc2 and 3.6.7rc2. See also bpo-34922.

    @ned-deily
    Copy link
    Member

    The question remains if Christian's and Victor's concern with 3.7 compatibility have been satisfied by Serihy's response in msg322798. While there is plenty of time to resolve concerns about what was merged into master by PR 8346, we shouldn't release the PR 8581 changes in 3.7.1 if there are still valid compatibility concerns. Since PR 9657 for 3.6 hasn't been reviewed or merged, the only pressing issue for 3.6.7 is ensuring the segfault documented in bpo-34922 is blocked, correct? And PR 8581 prevents the segfault for 3.7 so we would only need a similar fix for it if the PR were reverted, correct?

    @serhiy-storchaka
    Copy link
    Member

    I have reviewed PR 8346 yet one time. No documented working behavior was changed. Some documentation was updated to match the code, some code was updated to match the documentation, and other minor errors were fixed. I don't think this change breaks compatibility.

    @serhiy-storchaka
    Copy link
    Member

    Other variant of the crash in bpo-34922 still is reproducible in 3.7.

    @serhiy-storchaka
    Copy link
    Member

    New changeset f543e18 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-33729: Fix issues with arguments parsing in hashlib. (GH-8346) (GH-8581) (GH-9657)
    f543e18

    @ned-deily
    Copy link
    Member

    Serhiy's fixes (thanks!) are now released in 3.7.0rc2 and 3.6.7rc2 so I'm removing the "release blocker" status. If there is nothing more to be done for this issue, can we close it?

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @gpshead gpshead closed this as completed May 20, 2023
    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 docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants