Skip to content

bpo-35193: Fix an off by one error in the RETURN_VALUE case. #10418

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
gpshead merged 6 commits into from
Nov 9, 2018
Merged

bpo-35193: Fix an off by one error in the RETURN_VALUE case. #10418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
92281ce
Fix an off by one error in the RETURN_VALUE case.
gpshead Nov 8, 2018
4b0f93f
Add a NEWS entry.
gpshead Nov 8, 2018
fb357b6
Reword the NEWS entry.
gpshead Nov 9, 2018
9580c5d
Add a unittest that will fail under clang msan.
gpshead Nov 9, 2018
5d79253
Mention the releases that this bug is in.
gpshead Nov 9, 2018
d400140
Shorten the news entry text.
gpshead Nov 9, 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
19 changes: 19 additions & 0 deletions Lib/test/test_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import dis
import math
import os
import unittest
Expand Down Expand Up @@ -621,6 +622,24 @@ def check_same_constant(const):
self.check_constant(f1, frozenset({0}))
self.assertTrue(f1(0))

# This is a regression test for a CPython specific peephole optimizer
# implementation bug present in a few releases. It's assertion verifies
# that peephole optimization was actually done though that isn't an
# indication of the bugs presence or not (crashing is).
@support.cpython_only
def test_peephole_opt_unreachable_code_array_access_in_bounds(self):
"""Regression test for issue35193 when run under clang msan."""
def unused_code_at_end():
return 3
raise RuntimeError("unreachable")
# The above function definition will trigger the out of bounds
# bug in the peephole optimizer as it scans opcodes past the
# RETURN_VALUE opcode. This does not always crash an interpreter.
# When you build with the clang memory sanitizer it reliably aborts.
self.assertEqual(
'RETURN_VALUE',
list(dis.get_instructions(unused_code_at_end))[-1].opname)

def test_dont_merge_constants(self):
# Issue #25843: compile() must not merge constants which are equal
# but have a different type.
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2018-11-08-15-00-58.bpo-35193.HzPS6R.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix an off by one error in the bytecode peephole optimizer where it could read
bytes beyond the end of bounds of an array when removing unreachable code.
This bug was present in every release of Python 3.6 and 3.7 until now.
22 changes: 12 additions & 10 deletions Python/peephole.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ lastn_const_start(const _Py_CODEUNIT *codestr, Py_ssize_t i, Py_ssize_t n)

/* Scans through EXTENDED ARGs, seeking the index of the effective opcode */
static Py_ssize_t
find_op(const _Py_CODEUNIT *codestr, Py_ssize_t i)
find_op(const _Py_CODEUNIT *codestr, Py_ssize_t codelen, Py_ssize_t i)
{
while (_Py_OPCODE(codestr[i]) == EXTENDED_ARG) {
while (i < codelen && _Py_OPCODE(codestr[i]) == EXTENDED_ARG) {
i++;
}
return i;
Expand Down Expand Up @@ -128,8 +128,9 @@ copy_op_arg(_Py_CODEUNIT *codestr, Py_ssize_t i, unsigned char op,
Called with codestr pointing to the first LOAD_CONST.
*/
static Py_ssize_t
fold_tuple_on_constants(_Py_CODEUNIT *codestr, Py_ssize_t c_start,
Py_ssize_t opcode_end, PyObject *consts, int n)
fold_tuple_on_constants(_Py_CODEUNIT *codestr, Py_ssize_t codelen,
Py_ssize_t c_start, Py_ssize_t opcode_end,
PyObject *consts, int n)
{
/* Pre-conditions */
assert(PyList_CheckExact(consts));
Expand All @@ -142,7 +143,7 @@ fold_tuple_on_constants(_Py_CODEUNIT *codestr, Py_ssize_t c_start,

for (Py_ssize_t i = 0, pos = c_start; i < n; i++, pos++) {
assert(pos < opcode_end);
pos = find_op(codestr, pos);
pos = find_op(codestr, codelen, pos);
assert(_Py_OPCODE(codestr[pos]) == LOAD_CONST);

unsigned int arg = get_arg(codestr, pos);
Expand Down Expand Up @@ -265,7 +266,7 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names,
goto exitError;
assert(PyList_Check(consts));

for (i=find_op(codestr, 0) ; i<codelen ; i=nexti) {
for (i=find_op(codestr, codelen, 0) ; i<codelen ; i=nexti) {
opcode = _Py_OPCODE(codestr[i]);
op_start = i;
while (op_start >= 1 && _Py_OPCODE(codestr[op_start-1]) == EXTENDED_ARG) {
Expand Down Expand Up @@ -303,7 +304,8 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names,
if (j > 0 && lastlc >= j) {
h = lastn_const_start(codestr, op_start, j);
if (ISBASICBLOCK(blocks, h, op_start)) {
h = fold_tuple_on_constants(codestr, h, i+1, consts, j);
h = fold_tuple_on_constants(codestr, codelen,
h, i+1, consts, j);
break;
}
}
Expand Down Expand Up @@ -340,7 +342,7 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names,
case JUMP_IF_FALSE_OR_POP:
case JUMP_IF_TRUE_OR_POP:
h = get_arg(codestr, i) / sizeof(_Py_CODEUNIT);
tgt = find_op(codestr, h);
tgt = find_op(codestr, codelen, h);

j = _Py_OPCODE(codestr[tgt]);
if (CONDITIONAL_JUMP(j)) {
Expand Down Expand Up @@ -374,7 +376,7 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names,
case JUMP_FORWARD:
case JUMP_ABSOLUTE:
h = GETJUMPTGT(codestr, i);
tgt = find_op(codestr, h);
tgt = find_op(codestr, codelen, h);
/* Replace JUMP_* to a RETURN into just a RETURN */
if (UNCONDITIONAL_JUMP(opcode) &&
_Py_OPCODE(codestr[tgt]) == RETURN_VALUE) {
Expand Down Expand Up @@ -417,7 +419,7 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names,
}
if (h > i + 1) {
fill_nops(codestr, i + 1, h);
nexti = find_op(codestr, h);
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Nov 8, 2018

Choose a reason for hiding this comment

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

Wouldn't be simpler to just add a condition in this line?

nexti = (h < codelen) ? find_op(codestr, h) : codelen;

Copy link
Copy Markdown
Member

@Yhg1s Yhg1s Nov 8, 2018

Choose a reason for hiding this comment

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

That would leave room for the same bug to be introduced in other calls to find_op.

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In other calls find_op() can't be called with codelen.

Copy link
Copy Markdown
Member

@Yhg1s Yhg1s Nov 8, 2018

Choose a reason for hiding this comment

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

Yes. That's why I said "introduced". this prevents the bug from reappearing when the rest of the code changes.

nexti = find_op(codestr, codelen, h);
}
break;
}
Expand Down