Skip to content

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

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
merged 2 commits into from
Jul 31, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2b1dda
bpo-33871: Fix os.sendfile(), os.writev(), os.readv(), etc.
serhiy-storchaka Jun 26, 2018
b208466
Update news entries.
serhiy-storchaka Jun 26, 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
50 changes: 35 additions & 15 deletions Lib/test/test_os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2532,12 +2532,14 @@ class Handler(asynchat.async_chat):
def __init__(self, conn):
asynchat.async_chat.__init__(self, conn)
self.in_buffer = []
self.accumulate = True
self.closed = False
self.push(b"220 ready\r\n")

def handle_read(self):
data = self.recv(4096)
self.in_buffer.append(data)
if self.accumulate:
self.in_buffer.append(data)

def get_data(self):
return b''.join(self.in_buffer)
Expand Down Expand Up @@ -2618,6 +2620,8 @@ class TestSendfile(unittest.TestCase):
not sys.platform.startswith("sunos")
requires_headers_trailers = unittest.skipUnless(SUPPORT_HEADERS_TRAILERS,
'requires headers and trailers support')
requires_32b = unittest.skipUnless(sys.maxsize < 2**32,
'test is only meaningful on 32-bit builds')

@classmethod
def setUpClass(cls):
Expand Down Expand Up @@ -2648,17 +2652,13 @@ def tearDown(self):
self.server.stop()
self.server = None

def sendfile_wrapper(self, sock, file, offset, nbytes, headers=[], trailers=[]):
def sendfile_wrapper(self, *args, **kwargs):
"""A higher level wrapper representing how an application is
supposed to use sendfile().
"""
while 1:
while True:
try:
if self.SUPPORT_HEADERS_TRAILERS:
return os.sendfile(sock, file, offset, nbytes, headers,
trailers)
else:
return os.sendfile(sock, file, offset, nbytes)
return os.sendfile(*args, **kwargs)
except OSError as err:
if err.errno == errno.ECONNRESET:
# disconnected
Expand Down Expand Up @@ -2749,20 +2749,22 @@ def test_keywords(self):
@requires_headers_trailers
def test_headers(self):
total_sent = 0
expected_data = b"x" * 512 + b"y" * 256 + self.DATA[:-1]
sent = os.sendfile(self.sockno, self.fileno, 0, 4096,
headers=[b"x" * 512])
headers=[b"x" * 512, b"y" * 256])
self.assertLessEqual(sent, 512 + 256 + 4096)
total_sent += sent
offset = 4096
nbytes = 4096
while 1:
while total_sent < len(expected_data):
nbytes = min(len(expected_data) - total_sent, 4096)
sent = self.sendfile_wrapper(self.sockno, self.fileno,
offset, nbytes)
if sent == 0:
break
self.assertLessEqual(sent, nbytes)
total_sent += sent
offset += sent

expected_data = b"x" * 512 + self.DATA
self.assertEqual(total_sent, len(expected_data))
self.client.close()
self.server.wait()
Expand All @@ -2778,12 +2780,30 @@ def test_trailers(self):
create_file(TESTFN2, file_data)

with open(TESTFN2, 'rb') as f:
os.sendfile(self.sockno, f.fileno(), 0, len(file_data),
trailers=[b"1234"])
os.sendfile(self.sockno, f.fileno(), 0, 5,
trailers=[b"123456", b"789"])
self.client.close()
self.server.wait()
data = self.server.handler_instance.get_data()
self.assertEqual(data, b"abcdef1234")
self.assertEqual(data, b"abcde123456789")

@requires_headers_trailers
@requires_32b
def test_headers_overflow_32bits(self):
self.server.handler_instance.accumulate = False
with self.assertRaises(OSError) as cm:
os.sendfile(self.sockno, self.fileno, 0, 0,
headers=[b"x" * 2**16] * 2**15)
self.assertEqual(cm.exception.errno, errno.EINVAL)

@requires_headers_trailers
@requires_32b
def test_trailers_overflow_32bits(self):
self.server.handler_instance.accumulate = False
with self.assertRaises(OSError) as cm:
os.sendfile(self.sockno, self.fileno, 0, 0,
trailers=[b"x" * 2**16] * 2**15)
self.assertEqual(cm.exception.errno, errno.EINVAL)

@requires_headers_trailers
@unittest.skipUnless(hasattr(os, 'SF_NODISKIO'),
Expand Down
53 changes: 53 additions & 0 deletions Lib/test/test_posix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
_DUMMY_SYMLINK = os.path.join(tempfile.gettempdir(),
support.TESTFN + '-dummy-symlink')

requires_32b = unittest.skipUnless(sys.maxsize < 2**32,
'test is only meaningful on 32-bit builds')

class PosixTester(unittest.TestCase):

def setUp(self):
Expand Down Expand Up @@ -284,6 +287,7 @@ def test_preadv(self):
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'preadv'), "test needs posix.preadv()")
@unittest.skipUnless(hasattr(posix, 'RWF_HIPRI'), "test needs posix.RWF_HIPRI")
def test_preadv_flags(self):
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
Expand All @@ -295,6 +299,19 @@ def test_preadv_flags(self):
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'preadv'), "test needs posix.preadv()")
@requires_32b
def test_preadv_overflow_32bits(self):
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
try:
buf = [bytearray(2**16)] * 2**15
with self.assertRaises(OSError) as cm:
os.preadv(fd, buf, 0)
self.assertEqual(cm.exception.errno, errno.EINVAL)
self.assertEqual(bytes(buf[0]), b'\0'* 2**16)
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'pwrite'), "test needs posix.pwrite()")
def test_pwrite(self):
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
Expand All @@ -320,6 +337,7 @@ def test_pwritev(self):
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'pwritev'), "test needs posix.pwritev()")
@unittest.skipUnless(hasattr(posix, 'os.RWF_SYNC'), "test needs os.RWF_SYNC")
def test_pwritev_flags(self):
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
Expand All @@ -334,6 +352,17 @@ def test_pwritev_flags(self):
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'pwritev'), "test needs posix.pwritev()")
@requires_32b
def test_pwritev_overflow_32bits(self):
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
try:
with self.assertRaises(OSError) as cm:
os.pwritev(fd, [b"x" * 2**16] * 2**15, 0)
self.assertEqual(cm.exception.errno, errno.EINVAL)
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'posix_fallocate'),
"test needs posix.posix_fallocate()")
def test_posix_fallocate(self):
Expand Down Expand Up @@ -435,6 +464,17 @@ def test_writev(self):
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'writev'), "test needs posix.writev()")
@requires_32b
def test_writev_overflow_32bits(self):
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
try:
with self.assertRaises(OSError) as cm:
os.writev(fd, [b"x" * 2**16] * 2**15)
self.assertEqual(cm.exception.errno, errno.EINVAL)
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'readv'), "test needs posix.readv()")
def test_readv(self):
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
Expand All @@ -457,6 +497,19 @@ def test_readv(self):
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'readv'), "test needs posix.readv()")
@requires_32b
def test_readv_overflow_32bits(self):
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
try:
buf = [bytearray(2**16)] * 2**15
with self.assertRaises(OSError) as cm:
os.readv(fd, buf)
self.assertEqual(cm.exception.errno, errno.EINVAL)
self.assertEqual(bytes(buf[0]), b'\0'* 2**16)
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'dup'),
'test needs posix.dup()')
def test_dup(self):
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
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).
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
37 changes: 20 additions & 17 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8321,12 +8321,13 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length)
}

#if (defined(HAVE_SENDFILE) && (defined(__FreeBSD__) || defined(__DragonFly__) \
|| defined(__APPLE__))) || defined(HAVE_READV) || defined(HAVE_WRITEV)
static Py_ssize_t
|| defined(__APPLE__))) \
|| defined(HAVE_READV) || defined(HAVE_PREADV) || defined (HAVE_PREADV2) \
|| defined(HAVE_WRITEV) || defined(HAVE_PWRITEV) || defined (HAVE_PWRITEV2)
static int
iov_setup(struct iovec **iov, Py_buffer **buf, PyObject *seq, Py_ssize_t cnt, int type)
{
Py_ssize_t i, j;
Py_ssize_t blen, total = 0;

*iov = PyMem_New(struct iovec, cnt);
if (*iov == NULL) {
Expand All @@ -8351,11 +8352,9 @@ iov_setup(struct iovec **iov, Py_buffer **buf, PyObject *seq, Py_ssize_t cnt, in
}
Py_DECREF(item);
(*iov)[i].iov_base = (*buf)[i].buf;
blen = (*buf)[i].len;
(*iov)[i].iov_len = blen;
total += blen;
(*iov)[i].iov_len = (*buf)[i].len;
}
return total;
return 0;

fail:
PyMem_Del(*iov);
Expand Down Expand Up @@ -8652,12 +8651,20 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict)
}
if (i > 0) {
sf.hdr_cnt = (int)i;
i = iov_setup(&(sf.headers), &hbuf,
headers, sf.hdr_cnt, PyBUF_SIMPLE);
if (i < 0)
if (iov_setup(&(sf.headers), &hbuf,
headers, sf.hdr_cnt, PyBUF_SIMPLE) < 0)
return NULL;
#ifdef __APPLE__
sbytes += i;
for (i = 0; i < sf.hdr_cnt; i++) {
Py_ssize_t blen = sf.headers[i].iov_len;
# define OFF_T_MAX 0x7fffffffffffffff
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is OFF_MAX a macOS specific constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

_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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (sbytes >= OFF_T_MAX - blen) {
PyErr_SetString(PyExc_OverflowError,
"sendfile() header is too large");
return NULL;
}
sbytes += blen;
}
#endif
}
}
Expand All @@ -8678,13 +8685,9 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict)
}
if (i > 0) {
sf.trl_cnt = (int)i;
i = iov_setup(&(sf.trailers), &tbuf,
trailers, sf.trl_cnt, PyBUF_SIMPLE);
if (i < 0)
if (iov_setup(&(sf.trailers), &tbuf,
trailers, sf.trl_cnt, PyBUF_SIMPLE) < 0)
return NULL;
#ifdef __APPLE__
sbytes += i;
#endif
}
}
}
Expand Down