classification
Title: Zlib compress/decompress functions returning bytearray
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.0
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: alexandre.vassalotti, amaury.forgeotdarc, gregory.p.smith, jcea, ncoghlan, pitrou, pythonhacker
Priority: deferred blocker Keywords: easy, needs review, patch

Created on 2008-08-02 10:15 by pythonhacker, last changed 2008-09-08 05:43 by pythonhacker. This issue is now closed.

Files
File name Uploaded Description Edit
zlibmodule.patch pythonhacker, 2008-08-03 19:48
zlibmodule_svn_diff.patch pythonhacker, 2008-08-04 04:04
test_zlib_svn_diff.patch pythonhacker, 2008-08-04 04:24
zlib-and-zipimport-bytes-gps01.patch gregory.p.smith, 2008-09-05 00:07 fixed patch that covers zlib and zipimport
Messages (19)
msg70625 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-08-02 10:15
>>> 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.
msg70659 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-08-03 19:48
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.
msg70669 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2008-08-04 00:13
Could you submit unified diff--i.e., with 'diff -u' or 'svn diff'?

Also, could you add tests for this fix?
msg70683 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-08-04 04:04
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.
msg70685 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-08-04 04:24
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 .
msg70781 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-08-06 11:16
Any updates ? The py3k list is also very silent since the week-end...Thanks!
msg70933 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-09 16:30
> 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.
msg71078 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-08-13 09:33
Thanks. Will this make into beta3 ?
msg72483 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-09-04 05:15
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.
msg72491 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-09-04 11:13
+1 for committing.
msg72494 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-09-04 11:29
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:
- Modules/_dbmmodule.c
- Modules/mmapmodule.c
- Modules/ossaudiodev.c
- Modules/zipimport.c (as noted above)
- PC/winreg.c
- Python/marshal.c
msg72559 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-09-05 00:07
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.
msg72571 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-09-05 05:50
Does py3k list/barry have this bug in their radar for rc2 ?
msg72572 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-09-05 05:56
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:
> - Modules/_dbmmodule.c
> - Modules/mmapmodule.c
> - Modules/ossaudiodev.c
> - Modules/zipimport.c (as noted above)
> - PC/winreg.c
> - Python/marshal.c

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.
msg72580 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-09-05 10:25
Patch looks good to me (I've only looked at the patch - not the other
possible misuses of PyByteArray_ that Amaury noted)
msg72653 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-09-06 00:19
We must definitely clean up other uses of bytearray in the extension
modules - see #3790 for an example.
msg72686 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-09-06 20:15
fixed in r66266 along with #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.
msg72694 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-09-06 20:50
issue #3797 has been opened to track the other files mentioned.
msg72764 - (view) Author: Anand B Pillai (pythonhacker) Date: 2008-09-08 05:43
Hi Gregory,

      Let me know if I can help out some way in testing the #3797
patches on various platforms.

Regards,

--Anand
History
Date User Action Args
2008-09-08 05:43:00pythonhackersetmessages: + msg72764
2008-09-06 20:50:24gregory.p.smithsetstatus: open -> closed
messages: + msg72694
2008-09-06 20:15:51gregory.p.smithsetpriority: release blocker -> deferred blocker
2008-09-06 20:15:05gregory.p.smithsetresolution: fixed
messages: + msg72686
2008-09-06 00:19:36pitrousetmessages: + msg72653
2008-09-05 16:06:05jceasetnosy: + jcea
2008-09-05 10:25:31ncoghlansetnosy: + ncoghlan
messages: + msg72580
2008-09-05 05:56:36pythonhackersetmessages: + msg72572
2008-09-05 05:50:48pythonhackersetmessages: + msg72571
2008-09-05 00:07:57gregory.p.smithsetfiles: + zlib-and-zipimport-bytes-gps01.patch
messages: + msg72559
2008-09-04 11:29:05amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg72494
2008-09-04 11:13:10pitrousetmessages: + msg72491
2008-09-04 05:15:37gregory.p.smithsetpriority: release blocker
assignee: gregory.p.smith
messages: + msg72483
keywords: + easy, needs review
nosy: + gregory.p.smith
2008-08-13 09:33:10pythonhackersetmessages: + msg71078
2008-08-09 16:30:12pitrousetnosy: + pitrou
messages: + msg70933
2008-08-06 11:16:55pythonhackersetmessages: + msg70781
2008-08-04 04:24:58pythonhackersetfiles: + test_zlib_svn_diff.patch
messages: + msg70685
2008-08-04 04:04:05pythonhackersetfiles: + zlibmodule_svn_diff.patch
messages: + msg70683
2008-08-04 00:13:45alexandre.vassalottisetnosy: + alexandre.vassalotti
messages: + msg70669
2008-08-03 19:48:28pythonhackersetfiles: + zlibmodule.patch
keywords: + patch
messages: + msg70659
2008-08-02 10:15:58pythonhackercreate