bpo-33871: Fix os.sendfile(), os.writev(), os.readv(), etc. #7931

serhiy-storchaka · 2018-06-26T16:46:54Z

Fix integer overflow in os.readv(), os.writev(), os.preadv()
and os.pwritev() and in os.sendfile() with headers or trailers
arguments (on BSD-based OSes and MacOS).
Fix sending the part of the file in os.sendfile() on MacOS.
Using the trailers argument could cause sending more bytes from
the input file than was specified.

Thanks Ned Deily for testing on 32-bit MacOS.

https://bugs.python.org/issue33871

* Fix integer overflow in os.readv(), os.writev(), os.preadv() and os.pwritev() and in os.sendfile() with headers or trailers arguments (on BSD-based OSes and MacOS). * Fix sending the part of the file in os.sendfile() on MacOS. Using the trailers argument could cause sending more bytes from the input file than was specified. Thanks Ned Deily for testing on 32-bit MacOS.

ned-deily

I did not review the code other than the NEWS entries, someone else more familiar with sendfile should. The tests seem to run without error on 32-bit macOS.

ned-deily · 2018-06-26T19:00:15Z

Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst

+Fixed integer overflow in :func:`os.readv`, :func:`os.writev`,
+:func:`os.preadv` and :func:`os.pwritev` and in :func:`os.sendfile` with
+*headers* or *trailers* arguments (on BSD-based OSes and MacOS).
+Thanks Ned Deily for testing on 32-bit MacOS.


MacOS -> macOS. Also, thanks for the "thanks" but that is not needed for a NEWS entry.

ned-deily · 2018-06-26T19:01:49Z

Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst

@@ -0,0 +1,3 @@
+Fixed sending the part of the file in :func:`os.sendfile` on MacOS.  Using
+the *trailers* argument could cause sending more bytes from the input file
+than was specified.  Thanks Ned Deily for testing on 32-bit MacOS.


Same two comments as above.

bedevere-bot · 2018-06-26T19:04:28Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka · 2018-06-26T20:17:30Z

Modules/posixmodule.c

-                sbytes += i;
+                for (i = 0; i < sf.hdr_cnt; i++) {
+                    Py_ssize_t blen = sf.headers[i].iov_len;
+# define OFF_T_MAX 0x7fffffffffffffff


I'm not sure about this. Is off_t always 64-bit on macOS?

According to pymacconfig.h, off_t is equal to long long on 32-bit, and long on 64-bit. E.g. it is always 64-bit.

I've checked the SDK headers to be sure, and off_t is indeed always 64-bit (off_t is a typedef for __darwin_off_t, and that in turn is a typedef for __int64_t).

The manpage for sendfile(2) doesn't mention this, but the one for writev(2) says:

[EINVAL] The sum of the iov_len values in the iov array overflows a 32-bit integer.

Furthermore we have had problems with write(2) in the past when writing more than INT_MAX bytes at a time (see the implementation of _Py_write_impl). I think it is therefore better to limit the maximum size to INT_MAX, unless we explicitly test that sendfile can send more than INT_MAX bytes in one go.

BTW. Instead of OFF_T_MAX you could use OFF_MAX here, that should be defined to the maximum value of off_t.

Is OFF_MAX a macOS specific constant?

We have not just the sum of the iov_len values, but also added the length of the data sent from a file. The total sum can exceed 32-bit limit even if the sum of the iov_len values is in 32-bit limit.

I think it is better to use a 64-bit limit and allow OS to return an error if this is not supported. We should guard only from integer overflows occurred on the border between Python and OS.

In _PyWrite_impl we explicitly guard against overflowing a 32-bit integer to avoid problems. IIRC this was done because this gives a nicer user experience (and hoping that Apple will fix this in the kernel is not really useful).

I'll see if I can create a simple test to check if sendfile suffers from the same limitation on 10.13 and 10.14.

In _PyWrite_impl there is a different situation. We guard against overflowing int on Windows because write() accepts only unsigned int as the count argument on Windows, and because it is allowed to write less bytes then requested in write(). This is a border between Python and OS.

readv(), writev() and sendfile() accept size_t as the length of every chunk on all supported OSes even if OS doesn't support writing too large total size. I'm not sure that it is better to silently truncate the output than raise OSError. At least sendfile() with headers and trailers will produce corrupted output in this case: headers, than truncated content, than trailers.

_Py_write_impl does indeed not limit writes, I was confused because I have a local patch for issue 24658 that I never pushed...

sendfile like write can write less bytes than requested (at least, that's what the API suggests as it returns the amount of bytes written), limiting the amount of data written to avoid misbehaviour of the OS shouldn't be a problem.

That said: limiting is only necessary when there is a reason to do so. I guess I'll really have to write a test program that checks what the system does when trying to write more than 2GB of data.

Please test sending 2 GiB with headers and trailers.

os.sendfile(sockno, fileno, 0, 2**31-1, headers=[b"[begin]"], trailers=[b"[end]"])

If truncate the total size to INT_MAX, then this can send b"[begin]", then 2**31-1-7 bytes from the file (instead of expected 2**31-1 bytes) and then b"[end]". This is a wrong result.

ned-deily · 2018-06-26T20:21:49Z

Thanks for the changes, @serhiy-storchaka. @ronaldoussoren Do you have time to review this?

ronaldoussoren

Other than the comment about worries w.r.t. the actual limit below (OFF_MAX vs. INT_MAX) the patch looks good.

ronaldoussoren · 2018-07-01T19:23:54Z

Modules/posixmodule.c

-                sbytes += i;
+                for (i = 0; i < sf.hdr_cnt; i++) {
+                    Py_ssize_t blen = sf.headers[i].iov_len;
+# define OFF_T_MAX 0x7fffffffffffffff


I've checked the SDK headers to be sure, and off_t is indeed always 64-bit (off_t is a typedef for __darwin_off_t, and that in turn is a typedef for __int64_t).

The manpage for sendfile(2) doesn't mention this, but the one for writev(2) says:

[EINVAL] The sum of the iov_len values in the iov array overflows a 32-bit integer.

Furthermore we have had problems with write(2) in the past when writing more than INT_MAX bytes at a time (see the implementation of _Py_write_impl). I think it is therefore better to limit the maximum size to INT_MAX, unless we explicitly test that sendfile can send more than INT_MAX bytes in one go.

BTW. Instead of OFF_T_MAX you could use OFF_MAX here, that should be defined to the maximum value of off_t.

serhiy-storchaka · 2018-07-31T06:13:08Z

@ronaldoussoren, how to move this issue forward?

ronaldoussoren · 2018-07-31T07:18:49Z

@serhiy-storchaka: I propose merging as is, and creating a followup patch if it turns out that my size concerns are valid.

The reason I propose this is that the basic patch looks fine and I don't know how soon I'll have time to write (C) code to test the behaviour of the sendfile syscall w.r.t. buffer sizes.

serhiy-storchaka · 2018-07-31T07:23:37Z

Thanks!

miss-islington · 2018-07-31T07:24:57Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-07-31T07:25:14Z

GH-8583 is a backport of this pull request to the 3.7 branch.

…-7931) * Fix integer overflow in os.readv(), os.writev(), os.preadv() and os.pwritev() and in os.sendfile() with headers or trailers arguments (on BSD-based OSes and MacOS). * Fix sending the part of the file in os.sendfile() on MacOS. Using the trailers argument could cause sending more bytes from the input file than was specified. Thanks Ned Deily for testing on 32-bit MacOS. (cherry picked from commit 9d57273) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

miss-islington · 2018-07-31T07:25:59Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 9d5727326af53ddd91016d98e16ae7cf829caa95 3.6

miss-islington · 2018-07-31T07:26:59Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 9d5727326af53ddd91016d98e16ae7cf829caa95 2.7

…thonGH-7931) * Fix integer overflow in os.readv(), os.writev(), os.preadv() and os.pwritev() and in os.sendfile() with headers or trailers arguments (on BSD-based OSes and MacOS). * Fix sending the part of the file in os.sendfile() on MacOS. Using the trailers argument could cause sending more bytes from the input file than was specified. Thanks Ned Deily for testing on 32-bit MacOS.. (cherry picked from commit 9d57273) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-bot · 2018-07-31T07:38:39Z

GH-8584 is a backport of this pull request to the 3.6 branch.

* Fix integer overflow in os.readv(), os.writev(), os.preadv() and os.pwritev() and in os.sendfile() with headers or trailers arguments (on BSD-based OSes and MacOS). * Fix sending the part of the file in os.sendfile() on MacOS. Using the trailers argument could cause sending more bytes from the input file than was specified. Thanks Ned Deily for testing on 32-bit MacOS. (cherry picked from commit 9d57273) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…-7931) (GH-8584) * Fix integer overflow in os.readv(), os.writev() and in os.sendfile() with headers or trailers arguments (on BSD-based OSes and MacOS). * Fix sending the part of the file in os.sendfile() on MacOS. Using the trailers argument could cause sending more bytes from the input file than was specified. Thanks Ned Deily for testing on 32-bit MacOS. (cherry picked from commit 9d57273)

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 type-security A security issue labels Jun 26, 2018

serhiy-storchaka requested a review from ned-deily June 26, 2018 16:46

the-knights-who-say-ni added the CLA signed label Jun 26, 2018

bedevere-bot added the awaiting merge label Jun 26, 2018

ned-deily requested changes Jun 26, 2018

View reviewed changes

bedevere-bot removed the awaiting merge label Jun 26, 2018

bedevere-bot added the awaiting changes label Jun 26, 2018

Update news entries.

b208466

serhiy-storchaka commented Jun 26, 2018

View reviewed changes

ronaldoussoren requested changes Jul 1, 2018

View reviewed changes

ronaldoussoren approved these changes Jul 31, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jul 31, 2018

serhiy-storchaka merged commit 9d57273 into python:master Jul 31, 2018

bedevere-bot removed the awaiting merge label Jul 31, 2018

serhiy-storchaka deleted the sendfile-macos-overflow branch July 31, 2018 07:25

bedevere-bot removed the needs backport to 3.7 label Jul 31, 2018

miss-islington assigned serhiy-storchaka Jul 31, 2018

bedevere-bot removed the needs backport to 3.6 label Jul 31, 2018

serhiy-storchaka removed the needs backport to 2.7 label Sep 22, 2018

serhiy-storchaka removed their assignment Sep 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-33871: Fix os.sendfile(), os.writev(), os.readv(), etc. #7931

bpo-33871: Fix os.sendfile(), os.writev(), os.readv(), etc. #7931

serhiy-storchaka commented Jun 26, 2018 •

edited by bedevere-bot

ned-deily left a comment

ned-deily Jun 26, 2018

ned-deily Jun 26, 2018

bedevere-bot commented Jun 26, 2018

serhiy-storchaka Jun 26, 2018

serhiy-storchaka Jun 30, 2018

ronaldoussoren Jul 1, 2018

serhiy-storchaka Jul 2, 2018

serhiy-storchaka Jul 2, 2018

ronaldoussoren Jul 2, 2018

serhiy-storchaka Jul 3, 2018

ronaldoussoren Jul 4, 2018

serhiy-storchaka Jul 4, 2018

ned-deily commented Jun 26, 2018

ronaldoussoren left a comment

ronaldoussoren Jul 1, 2018

serhiy-storchaka commented Jul 31, 2018

ronaldoussoren commented Jul 31, 2018

serhiy-storchaka commented Jul 31, 2018

miss-islington commented Jul 31, 2018

bedevere-bot commented Jul 31, 2018

miss-islington commented Jul 31, 2018

miss-islington commented Jul 31, 2018

bedevere-bot commented Jul 31, 2018

bpo-33871: Fix os.sendfile(), os.writev(), os.readv(), etc. #7931

bpo-33871: Fix os.sendfile(), os.writev(), os.readv(), etc. #7931

Conversation

serhiy-storchaka commented Jun 26, 2018 • edited by bedevere-bot

ned-deily left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ned-deily commented Jun 26, 2018

ronaldoussoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Jul 31, 2018

ronaldoussoren commented Jul 31, 2018

serhiy-storchaka commented Jul 31, 2018

miss-islington commented Jul 31, 2018

bedevere-bot commented Jul 31, 2018

miss-islington commented Jul 31, 2018

miss-islington commented Jul 31, 2018

bedevere-bot commented Jul 31, 2018

serhiy-storchaka commented Jun 26, 2018 •

edited by bedevere-bot