Skip to content

[2.7] bpo-32304: Fix distutils upload for tar files ending with b'\r' (GH-5264) #5331

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

Merged
merwok merged 4 commits into from
Jan 29, 2018
Merged

[2.7] bpo-32304: Fix distutils upload for tar files ending with b'\r' (GH-5264) #5331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85bb01b
[2.7] bpo-32304: Fix distutils upload for sdists ending with \x0d (GH…
bbayles Jan 26, 2018
0297ab2
bpo-32304: Fix up test
bbayles Jan 26, 2018
233d544
bpo-32304: Port whatsnew
bbayles Jan 26, 2018
29a9e7a
Explicitly check for \n
bbayles Jan 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Doc/whatsnew/2.7.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1218,6 +1218,11 @@ changes, or look through the Subversion logs for all the details.
created some new files that should be included.
(Fixed by Tarek Ziadé; :issue:`8688`.)

The ``upload`` command now longer tries to change CR end-of-line characters
to CRLF. This fixes a corruption issue with sdists that ended with a byte
equivalent to CR.
(Contributed by Bo Bayles in :issue:`32304`.)

* The :mod:`doctest` module's :const:`IGNORE_EXCEPTION_DETAIL` flag
will now ignore the name of the module containing the exception
being tested. (Patch by Lennart Regebro; :issue:`7490`.)
Expand Down
2 changes: 0 additions & 2 deletions Lib/distutils/command/upload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,6 @@ def upload_file(self, command, pyversion, filename):
body.write(fn)
body.write("\r\n\r\n")
body.write(value)
if value and value[-1] == '\r':
body.write('\n') # write an extra newline (lurve Macs)
body.write(end_boundary)
body = body.getvalue()

Expand Down
26 changes: 26 additions & 0 deletions Lib/distutils/tests/test_upload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,32 @@ def test_upload(self):
auth = self.last_open.req.headers['Authorization']
self.assertNotIn('\n', auth)

# bpo-32304: archives whose last byte was b'\r' were corrupted due to
# normalization intended for Mac OS 9.
def test_upload_correct_cr(self):

@merwok merwok Jan 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the test failure: https://travis-ci.org/python/cpython/jobs/333550360#L2640-L2647

@bbayles bbayles Jan 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see what's going on - the cmd.show_response isn't the same in this version. There are some other differences in the test I'll smooth out shortly.

# content that ends with \r should not be modified.
tmp = self.mkdtemp()
path = os.path.join(tmp, 'xxx')
self.write_file(path, content='yy\r')
command, pyversion, filename = 'xxx', '2.6', path
dist_files = [(command, pyversion, filename)]
self.write_file(self.rc, PYPIRC_LONG_PASSWORD)

# other fields that ended with \r used to be modified, now are
# preserved.
pkg_dir, dist = self.create_dist(
dist_files=dist_files,
description='long description\r'
)
cmd = upload(dist)
cmd.ensure_finalized()
cmd.run()

headers = dict(self.last_open.req.headers)
self.assertEqual(headers['Content-length'], '2170')
self.assertIn(b'long description\r', self.last_open.req.data)

@merwok merwok Jan 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test would not catch a regression, i.e. fail if the request data contains b'long description\r\n'.

@bbayles bbayles Jan 28, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The size check earlier would signal the error, but I've added a new line to make it explicit.

@merwok merwok Jan 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

self.assertNotIn(b'long description\r\n', self.last_open.req.data)

def test_upload_fails(self):
self.next_msg = "Not Found"
self.next_code = 404
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2018-01-21-16-33-53.bpo-32304.TItrNv.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
distutils' upload command no longer corrupts tar files ending with a CR byte,
and no longer tries to convert CR to CRLF in any of the upload text fields.