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.

Sign up for GitHub

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

Jump to bottom

Zlib compress/decompress functions returning bytearray #47742

Closed
pythonhacker mannequin opened this issue Aug 2, 2008 · 19 comments
Closed

Zlib compress/decompress functions returning bytearray #47742

pythonhacker mannequin opened this issue Aug 2, 2008 · 19 comments
Assignees
@gpshead
Labels
deferred-blocker easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@pythonhacker
Copy link
Mannequin

pythonhacker mannequin commented Aug 2, 2008

BPO 3492
Nosy @gpshead, @jcea, @amauryfa, @ncoghlan, @pitrou, @avassalotti
Files
  • zlibmodule.patch
  • zlibmodule_svn_diff.patch
  • test_zlib_svn_diff.patch
  • zlib-and-zipimport-bytes-gps01.patch: fixed patch that covers zlib and zipimport
    		•

    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/gpshead'
closed_at = <Date 2008-09-06.20:50:24.980>
created_at = <Date 2008-08-02.10:15:58.076>
labels = ['easy', 'deferred-blocker', 'type-bug', 'library']
title = 'Zlib compress/decompress functions returning bytearray'
updated_at = <Date 2008-09-08.05:43:00.798>
user = 'https://bugs.python.org/pythonhacker'

    bugs.python.org fields:

    activity = <Date 2008-09-08.05:43:00.798>
actor = 'pythonhacker'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2008-09-06.20:50:24.980>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2008-08-02.10:15:58.076>
creator = 'pythonhacker'
dependencies = []
files = ['11047', '11049', '11051', '11381']
hgrepos = []
issue_num = 3492
keywords = ['patch', 'easy', 'needs review']
message_count = 19.0
messages = ['70625', '70659', '70669', '70683', '70685', '70781', '70933', '71078', '72483', '72491', '72494', '72559', '72571', '72572', '72580', '72653', '72686', '72694', '72764']
nosy_count = 7.0
nosy_names = ['gregory.p.smith', 'jcea', 'amaury.forgeotdarc', 'ncoghlan', 'pitrou', 'alexandre.vassalotti', 'pythonhacker']
pr_nums = []
priority = 'deferred blocker'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue3492'
versions = ['Python 3.0']
    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Aug 2, 2008 

    >>> import zlib
>>> s='This is a string'
>>> sc=zlib.compress(s)
>>> sc
bytearray(b'x\x9c\x0b\xc9\xc8,V\x00\xa2D\x85\xe2\x92\xa2\xcc\xbct\x00/\xc2\x05\xcd')
>>> zlib.decompress(sc)
bytearray(b'This is a string')
>>>

    This is wrong behavior as compress functions should return byte
    ,not a bytearray. Same for decompress.

    @pythonhacker pythonhacker mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Aug 2, 2008
    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Aug 3, 2008

    Hi, I have a patch ready for this to be applied to zlibmodule.c. The
    patch is attached. I have tested it and it is working fine.

    @avassalotti
    Copy link
    Member

    Could you submit unified diff--i.e., with 'diff -u' or 'svn diff'?

    Also, could you add tests for this fix?

    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Aug 4, 2008

    Uploading svn diff for zlibmodule.c. Btw, how do I add unit tests for a
    fix ? You want me to create a simple test file and upload it here, or is
    there a standard procedure for this ? Please advise.

    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Aug 4, 2008

    Ok. I added two tests for checking the type of the returned object for
    zlib.compress and zlib.decompress in test_zlib.py in the py3k branch.

    Btw, my code uses assertEqual(type(...), bytes). Is this the proper way
    of type checking in py3k ? I could not see any formal type for "bytes"
    type in the types module.

    Attaching the svn diff for test_zlib.py .

    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Aug 6, 2008

    Any updates ? The py3k list is also very silent since the week-end...Thanks!

    @pitrou
    Copy link
    Member

    pitrou commented Aug 9, 2008

    Any updates ? The py3k list is also very silent since the
    week-end...Thanks!

    Your two patches look good, I suppose either Alexandre or I will commit
    them soon.
    You shouldn't to worry when you don't get a reply immediately, people
    react simply when they have time to do so. And as for the mailing-list
    activity, we are in the beginning of August which I guess implies many
    people are on holidays.

    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Aug 13, 2008

    Thanks. Will this make into beta3 ?

    @gpshead
    Copy link
    Member

    gpshead commented Sep 4, 2008

    this is a very simple patch and makes sense to me. marking it a release
    blocker and asking about it on the mailing list. if anyone disagrees
    with this one please speak up.

    in general IO input functions elsewhere return bytes(). zlib should as
    well. this fixes that.

    @gpshead gpshead self-assigned this Sep 4, 2008
    @pitrou
    Copy link
    Member

    pitrou commented Sep 4, 2008

    +1 for committing.

    @amauryfa
    Copy link
    Member

    amauryfa commented Sep 4, 2008

    Two remarks:

    1. Some functions in Modules/zipimport.c (get_data) will now receive
      PyBytes, but zipimporter_get_source handle them as PyByteArray. Does
      zipimport still work at all with this patch?

    2. There are other places where PyString_FromString was (incorrectly
      IMO) replaced with PyByteArray_FromString:

    @gpshead
    Copy link
    Member

    gpshead commented Sep 5, 2008

    Correct, zipimport required fixing in order for this to work. The newly
    attached zlib-and-zipimport-gps01 patch.

    review at http://codereview.appspot.com/4454

    I haven't had a chance to look at the other modules Amaury mentioned but
    on general principal I agree, they should return bytes.

    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Sep 5, 2008

    Does py3k list/barry have this bug in their radar for rc2 ?

    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Sep 5, 2008

    On Thu, Sep 4, 2008 at 4:59 PM, Amaury Forgeot d'Arc
    <report@bugs.python.org> wrote:

    Amaury Forgeot d'Arc <amauryfa@gmail.com> added the comment:

    Two remarks:

    1. Some functions in Modules/zipimport.c (get_data) will now receive
      PyBytes, but zipimporter_get_source handle them as PyByteArray. Does
      zipimport still work at all with this patch?

    2. There are other places where PyString_FromString was (incorrectly
      IMO) replaced with PyByteArray_FromString:

    Hmmm...but AFAIK zlib changes only affect zipimport directly. I wonder
    whether these other instances have bugs reported and are being tracked
    for the release.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 5, 2008

    Patch looks good to me (I've only looked at the patch - not the other
    possible misuses of PyByteArray_ that Amaury noted)

    @pitrou
    Copy link
    Member

    pitrou commented Sep 6, 2008

    We must definitely clean up other uses of bytearray in the extension
    modules - see bpo-3790 for an example.

    @gpshead
    Copy link
    Member

    gpshead commented Sep 6, 2008

    fixed in r66266 along with bpo-3790.

    leaving this open and assigned to me while i investigate the other uses
    of ByteArray in the Modules/ directory.

    IMHO, its fine if we fix any remaining bytearray uses up for rc2.

    @gpshead
    Copy link
    Member

    gpshead commented Sep 6, 2008

    issue bpo-3797 has been opened to track the other files mentioned.

    @gpshead gpshead closed this as completed Sep 6, 2008
    @pythonhacker
    Copy link
    Mannequin Author

    pythonhacker mannequin commented Sep 8, 2008

    Hi Gregory,

      Let me know if I can help out some way in testing the bpo-3797

    patches on various platforms.

    Regards,

    --Anand

    @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
    Assignees

    @gpshead gpshead

    Labels
    deferred-blocker easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @gpshead @avassalotti @amauryfa @ncoghlan @pitrou