Skip to content

Commit 9d57273

Browse files
serhiy-storchakaserhiy-storchaka
authored
bpo-33871: Fix os.sendfile(), os.writev(), os.readv(), etc. (GH-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.
1 parent f1d36d8 commit 9d57273

5 files changed

Lines changed: 114 additions & 32 deletions

File tree

‎Lib/test/test_os.py‎

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2532,12 +2532,14 @@ class Handler(asynchat.async_chat):
25322532
def __init__(self, conn):
25332533
asynchat.async_chat.__init__(self, conn)
25342534
self.in_buffer = []
2535+
self.accumulate = True
25352536
self.closed = False
25362537
self.push(b"220 ready\r\n")
25372538

25382539
def handle_read(self):
25392540
data = self.recv(4096)
2540-
self.in_buffer.append(data)
2541+
if self.accumulate:
2542+
self.in_buffer.append(data)
25412543

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

26222626
@classmethod
26232627
def setUpClass(cls):
@@ -2648,17 +2652,13 @@ def tearDown(self):
26482652
self.server.stop()
26492653
self.server = None
26502654

2651-
def sendfile_wrapper(self, sock, file, offset, nbytes, headers=[], trailers=[]):
2655+
def sendfile_wrapper(self, *args, **kwargs):
26522656
"""A higher level wrapper representing how an application is
26532657
supposed to use sendfile().
26542658
"""
2655-
while 1:
2659+
while True:
26562660
try:
2657-
if self.SUPPORT_HEADERS_TRAILERS:
2658-
return os.sendfile(sock, file, offset, nbytes, headers,
2659-
trailers)
2660-
else:
2661-
return os.sendfile(sock, file, offset, nbytes)
2661+
return os.sendfile(*args, **kwargs)
26622662
except OSError as err:
26632663
if err.errno == errno.ECONNRESET:
26642664
# disconnected
@@ -2749,20 +2749,22 @@ def test_keywords(self):
27492749
@requires_headers_trailers
27502750
def test_headers(self):
27512751
total_sent = 0
2752+
expected_data = b"x" * 512 + b"y" * 256 + self.DATA[:-1]
27522753
sent = os.sendfile(self.sockno, self.fileno, 0, 4096,
2753-
headers=[b"x" * 512])
2754+
headers=[b"x" * 512, b"y" * 256])
2755+
self.assertLessEqual(sent, 512 + 256 + 4096)
27542756
total_sent += sent
27552757
offset = 4096
2756-
nbytes = 4096
2757-
while 1:
2758+
while total_sent < len(expected_data):
2759+
nbytes = min(len(expected_data) - total_sent, 4096)
27582760
sent = self.sendfile_wrapper(self.sockno, self.fileno,
27592761
offset, nbytes)
27602762
if sent == 0:
27612763
break
2764+
self.assertLessEqual(sent, nbytes)
27622765
total_sent += sent
27632766
offset += sent
27642767

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

27802782
with open(TESTFN2, 'rb') as f:
2781-
os.sendfile(self.sockno, f.fileno(), 0, len(file_data),
2782-
trailers=[b"1234"])
2783+
os.sendfile(self.sockno, f.fileno(), 0, 5,
2784+
trailers=[b"123456", b"789"])
27832785
self.client.close()
27842786
self.server.wait()
27852787
data = self.server.handler_instance.get_data()
2786-
self.assertEqual(data, b"abcdef1234")
2788+
self.assertEqual(data, b"abcde123456789")
2789+
2790+
@requires_headers_trailers
2791+
@requires_32b
2792+
def test_headers_overflow_32bits(self):
2793+
self.server.handler_instance.accumulate = False
2794+
with self.assertRaises(OSError) as cm:
2795+
os.sendfile(self.sockno, self.fileno, 0, 0,
2796+
headers=[b"x" * 2**16] * 2**15)
2797+
self.assertEqual(cm.exception.errno, errno.EINVAL)
2798+
2799+
@requires_headers_trailers
2800+
@requires_32b
2801+
def test_trailers_overflow_32bits(self):
2802+
self.server.handler_instance.accumulate = False
2803+
with self.assertRaises(OSError) as cm:
2804+
os.sendfile(self.sockno, self.fileno, 0, 0,
2805+
trailers=[b"x" * 2**16] * 2**15)
2806+
self.assertEqual(cm.exception.errno, errno.EINVAL)
27872807

27882808
@requires_headers_trailers
27892809
@unittest.skipUnless(hasattr(os, 'SF_NODISKIO'),

‎Lib/test/test_posix.py‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@
2020
_DUMMY_SYMLINK = os.path.join(tempfile.gettempdir(),
2121
support.TESTFN + '-dummy-symlink')
2222

23+
requires_32b = unittest.skipUnless(sys.maxsize < 2**32,
24+
'test is only meaningful on 32-bit builds')
25+
2326
class PosixTester(unittest.TestCase):
2427

2528
def setUp(self):
@@ -284,6 +287,7 @@ def test_preadv(self):
284287
finally:
285288
os.close(fd)
286289

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

302+
@unittest.skipUnless(hasattr(posix, 'preadv'), "test needs posix.preadv()")
303+
@requires_32b
304+
def test_preadv_overflow_32bits(self):
305+
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
306+
try:
307+
buf = [bytearray(2**16)] * 2**15
308+
with self.assertRaises(OSError) as cm:
309+
os.preadv(fd, buf, 0)
310+
self.assertEqual(cm.exception.errno, errno.EINVAL)
311+
self.assertEqual(bytes(buf[0]), b'\0'* 2**16)
312+
finally:
313+
os.close(fd)
314+
298315
@unittest.skipUnless(hasattr(posix, 'pwrite'), "test needs posix.pwrite()")
299316
def test_pwrite(self):
300317
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
@@ -320,6 +337,7 @@ def test_pwritev(self):
320337
finally:
321338
os.close(fd)
322339

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

355+
@unittest.skipUnless(hasattr(posix, 'pwritev'), "test needs posix.pwritev()")
356+
@requires_32b
357+
def test_pwritev_overflow_32bits(self):
358+
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
359+
try:
360+
with self.assertRaises(OSError) as cm:
361+
os.pwritev(fd, [b"x" * 2**16] * 2**15, 0)
362+
self.assertEqual(cm.exception.errno, errno.EINVAL)
363+
finally:
364+
os.close(fd)
365+
337366
@unittest.skipUnless(hasattr(posix, 'posix_fallocate'),
338367
"test needs posix.posix_fallocate()")
339368
def test_posix_fallocate(self):
@@ -435,6 +464,17 @@ def test_writev(self):
435464
finally:
436465
os.close(fd)
437466

467+
@unittest.skipUnless(hasattr(posix, 'writev'), "test needs posix.writev()")
468+
@requires_32b
469+
def test_writev_overflow_32bits(self):
470+
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
471+
try:
472+
with self.assertRaises(OSError) as cm:
473+
os.writev(fd, [b"x" * 2**16] * 2**15)
474+
self.assertEqual(cm.exception.errno, errno.EINVAL)
475+
finally:
476+
os.close(fd)
477+
438478
@unittest.skipUnless(hasattr(posix, 'readv'), "test needs posix.readv()")
439479
def test_readv(self):
440480
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
@@ -457,6 +497,19 @@ def test_readv(self):
457497
finally:
458498
os.close(fd)
459499

500+
@unittest.skipUnless(hasattr(posix, 'readv'), "test needs posix.readv()")
501+
@requires_32b
502+
def test_readv_overflow_32bits(self):
503+
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
504+
try:
505+
buf = [bytearray(2**16)] * 2**15
506+
with self.assertRaises(OSError) as cm:
507+
os.readv(fd, buf)
508+
self.assertEqual(cm.exception.errno, errno.EINVAL)
509+
self.assertEqual(bytes(buf[0]), b'\0'* 2**16)
510+
finally:
511+
os.close(fd)
512+
460513
@unittest.skipUnless(hasattr(posix, 'dup'),
461514
'test needs posix.dup()')
462515
def test_dup(self):

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fixed integer overflow in :func:`os.readv`, :func:`os.writev`,
2+
:func:`os.preadv` and :func:`os.pwritev` and in :func:`os.sendfile` with
3+
*headers* or *trailers* arguments (on BSD-based OSes and macOS).

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fixed sending the part of the file in :func:`os.sendfile` on macOS. Using
2+
the *trailers* argument could cause sending more bytes from the input file
3+
than was specified.

‎Modules/posixmodule.c‎

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8321,12 +8321,13 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length)
83218321
}
83228322

83238323
#if (defined(HAVE_SENDFILE) && (defined(__FreeBSD__) || defined(__DragonFly__) \
8324-
|| defined(__APPLE__))) || defined(HAVE_READV) || defined(HAVE_WRITEV)
8325-
static Py_ssize_t
8324+
|| defined(__APPLE__))) \
8325+
|| defined(HAVE_READV) || defined(HAVE_PREADV) || defined (HAVE_PREADV2) \
8326+
|| defined(HAVE_WRITEV) || defined(HAVE_PWRITEV) || defined (HAVE_PWRITEV2)
8327+
static int
83268328
iov_setup(struct iovec **iov, Py_buffer **buf, PyObject *seq, Py_ssize_t cnt, int type)
83278329
{
83288330
Py_ssize_t i, j;
8329-
Py_ssize_t blen, total = 0;
83308331

83318332
*iov = PyMem_New(struct iovec, cnt);
83328333
if (*iov == NULL) {
@@ -8351,11 +8352,9 @@ iov_setup(struct iovec **iov, Py_buffer **buf, PyObject *seq, Py_ssize_t cnt, in
83518352
}
83528353
Py_DECREF(item);
83538354
(*iov)[i].iov_base = (*buf)[i].buf;
8354-
blen = (*buf)[i].len;
8355-
(*iov)[i].iov_len = blen;
8356-
total += blen;
8355+
(*iov)[i].iov_len = (*buf)[i].len;
83578356
}
8358-
return total;
8357+
return 0;
83598358

83608359
fail:
83618360
PyMem_Del(*iov);
@@ -8652,12 +8651,20 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict)
86528651
}
86538652
if (i > 0) {
86548653
sf.hdr_cnt = (int)i;
8655-
i = iov_setup(&(sf.headers), &hbuf,
8656-
headers, sf.hdr_cnt, PyBUF_SIMPLE);
8657-
if (i < 0)
8654+
if (iov_setup(&(sf.headers), &hbuf,
8655+
headers, sf.hdr_cnt, PyBUF_SIMPLE) < 0)
86588656
return NULL;
86598657
#ifdef __APPLE__
8660-
sbytes += i;
8658+
for (i = 0; i < sf.hdr_cnt; i++) {
8659+
Py_ssize_t blen = sf.headers[i].iov_len;
8660+
# define OFF_T_MAX 0x7fffffffffffffff
8661+
if (sbytes >= OFF_T_MAX - blen) {
8662+
PyErr_SetString(PyExc_OverflowError,
8663+
"sendfile() header is too large");
8664+
return NULL;
8665+
}
8666+
sbytes += blen;
8667+
}
86618668
#endif
86628669
}
86638670
}
@@ -8678,13 +8685,9 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict)
86788685
}
86798686
if (i > 0) {
86808687
sf.trl_cnt = (int)i;
8681-
i = iov_setup(&(sf.trailers), &tbuf,
8682-
trailers, sf.trl_cnt, PyBUF_SIMPLE);
8683-
if (i < 0)
8688+
if (iov_setup(&(sf.trailers), &tbuf,
8689+
trailers, sf.trl_cnt, PyBUF_SIMPLE) < 0)
86848690
return NULL;
8685-
#ifdef __APPLE__
8686-
sbytes += i;
8687-
#endif
86888691
}
86898692
}
86908693
}

0 commit comments

Comments
 (0)