Merge lp:~gz/brz/bencode_recursion into lp:brz

Proposed by Martin Packman on 2018-08-20
Status: Merged
Approved by: Martin Packman on 2018-08-21
Approved revision: 7062
Merge reported by: The Breezy Bot
Merged at revision: not available
Proposed branch: lp:~gz/brz/bencode_recursion
Merge into: lp:brz
Diff against target: 230 lines (+50/-37)
4 files modified
breezy/_bencode_pyx.pyx (+20/-24)
breezy/python-compat.h (+2/-0)
breezy/tests/test__bencode.py (+26/-13)
python3.passing (+2/-0)
To merge this branch: bzr merge lp:~gz/brz/bencode_recursion
Reviewer Review Type Date Requested Status
Jelmer Vernooij 2018-08-20 Approve on 2018-08-20
Review via email: mp+353451@code.launchpad.net

Commit message

Fix recursion handling in bencode extension

Description of the change

This conflicts with part of the fix in the proposed fix-c-extensions branch:

https://code.launchpad.net/~jelmer/brz/fix-c-extensions/+merge/353369

I think the right change is a mix of the two? Should bare raise but with the (uncovered?) encode site also updated.

To post a comment you must log in.
Jelmer Vernooij (jelmer) wrote :

This seems simpler than my solution and also fix other stuff (e.g. the warnings).

review: Approve
The Breezy Bot (the-breezy-bot) wrote :

Running landing tests failed
https://ci.breezy-vcs.org/job/land-brz/394/

The Breezy Bot (the-breezy-bot) wrote :

Running landing tests failed
https://ci.breezy-vcs.org/job/land-brz/405/

lp:~gz/brz/bencode_recursion updated on 2018-08-21
7062. By Martin Packman on 2018-08-21

Fix recursion check in C bencode implementation

Hard to get Cython to do the right thing but by inverting the
return code can use the standard except handling.

Avoid going through a Python call when encoding, which requires
the encode recursion check to work too.

Adjust tests to use a smaller limit to be more managable.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'breezy/_bencode_pyx.pyx'
2--- breezy/_bencode_pyx.pyx 2017-06-27 01:29:30 +0000
3+++ breezy/_bencode_pyx.pyx 2018-08-21 00:57:35 +0000
4@@ -18,37 +18,37 @@
5
6 from __future__ import absolute_import
7
8+from cpython.bool cimport (
9+ PyBool_Check,
10+ )
11 from cpython.bytes cimport (
12 PyBytes_CheckExact,
13 PyBytes_FromStringAndSize,
14 PyBytes_AS_STRING,
15 PyBytes_GET_SIZE,
16 )
17-from cpython.long cimport (
18- PyLong_CheckExact,
19+from cpython.dict cimport (
20+ PyDict_CheckExact,
21 )
22 from cpython.int cimport (
23 PyInt_CheckExact,
24 PyInt_FromString,
25 )
26-from cpython.tuple cimport (
27- PyTuple_CheckExact,
28- )
29 from cpython.list cimport (
30 PyList_CheckExact,
31 PyList_Append,
32 )
33-from cpython.dict cimport (
34- PyDict_CheckExact,
35- )
36-from cpython.bool cimport (
37- PyBool_Check,
38+from cpython.long cimport (
39+ PyLong_CheckExact,
40 )
41 from cpython.mem cimport (
42 PyMem_Free,
43 PyMem_Malloc,
44 PyMem_Realloc,
45 )
46+from cpython.tuple cimport (
47+ PyTuple_CheckExact,
48+ )
49
50 from libc.stdlib cimport (
51 strtol,
52@@ -57,15 +57,14 @@
53 memcpy,
54 )
55
56+cdef extern from "python-compat.h":
57+ int snprintf(char* buffer, size_t nsize, char* fmt, ...)
58+ # Use wrapper with inverted error return so Cython can propogate
59+ int BrzPy_EnterRecursiveCall(char *) except 0
60+
61 cdef extern from "Python.h":
62- # There is no cython module for ceval.h for some reason
63- int Py_GetRecursionLimit()
64- int Py_EnterRecursiveCall(char *)
65 void Py_LeaveRecursiveCall()
66
67-cdef extern from "python-compat.h":
68- int snprintf(char* buffer, size_t nsize, char* fmt, ...)
69-
70 cdef class Decoder
71 cdef class Encoder
72
73@@ -114,8 +113,7 @@
74 if 0 == self.size:
75 raise ValueError('stream underflow')
76
77- if Py_EnterRecursiveCall("_decode_object"):
78- raise RuntimeError("too deeply nested")
79+ BrzPy_EnterRecursiveCall(" while bencode decoding")
80 try:
81 ch = self.tail[0]
82 if c'0' <= ch <= c'9':
83@@ -129,10 +127,9 @@
84 elif ch == c'd':
85 D_UPDATE_TAIL(self, 1)
86 return self._decode_dict()
87- else:
88- raise ValueError('unknown object type identifier %r' % ch)
89 finally:
90 Py_LeaveRecursiveCall()
91+ raise ValueError('unknown object type identifier %r' % ch)
92
93 cdef int _read_digits(self, char stop_char) except -1:
94 cdef int i
95@@ -342,7 +339,7 @@
96 cdef Py_ssize_t x_len
97 x_len = PyBytes_GET_SIZE(x)
98 self._ensure_buffer(x_len + INT_BUF_SIZE)
99- n = snprintf(self.tail, INT_BUF_SIZE, b'%d:', x_len)
100+ n = snprintf(self.tail, INT_BUF_SIZE, b'%ld:', x_len)
101 if n < 0:
102 raise MemoryError('string %s too big to encode' % x)
103 memcpy(<void *>(self.tail+n), PyBytes_AS_STRING(x), x_len)
104@@ -378,9 +375,8 @@
105 E_UPDATE_TAIL(self, 1)
106 return 1
107
108- def process(self, object x):
109- if Py_EnterRecursiveCall("encode"):
110- raise RuntimeError("too deeply nested")
111+ cpdef object process(self, object x):
112+ BrzPy_EnterRecursiveCall(" while bencode encoding")
113 try:
114 if PyBytes_CheckExact(x):
115 self._encode_string(x)
116
117=== modified file 'breezy/python-compat.h'
118--- breezy/python-compat.h 2018-07-12 01:43:49 +0000
119+++ breezy/python-compat.h 2018-08-21 00:57:35 +0000
120@@ -71,6 +71,8 @@
121
122 #endif
123
124+#define BrzPy_EnterRecursiveCall(where) (Py_EnterRecursiveCall(where) == 0)
125+
126 #if defined(_WIN32) || defined(WIN32)
127 /* Defining WIN32_LEAN_AND_MEAN makes including windows quite a bit
128 * lighter weight.
129
130=== modified file 'breezy/tests/test__bencode.py'
131--- breezy/tests/test__bencode.py 2017-06-10 22:09:37 +0000
132+++ breezy/tests/test__bencode.py 2018-08-21 00:57:35 +0000
133@@ -26,6 +26,21 @@
134 return suite
135
136
137+class RecursionLimit(object):
138+ """Context manager that lowers recursion limit for testing."""
139+
140+ def __init__(self, limit=100):
141+ self._new_limit = limit
142+ self._old_limit = sys.getrecursionlimit()
143+
144+ def __enter__(self):
145+ sys.setrecursionlimit(self._new_limit)
146+ return self
147+
148+ def __exit__(self, *exc_info):
149+ sys.setrecursionlimit(self._old_limit)
150+
151+
152 class TestBencodeDecode(tests.TestCase):
153
154 module = None
155@@ -90,7 +105,8 @@
156 self._check([[b'Alice', b'Bob'], [2, 3]], b'll5:Alice3:Bobeli2ei3eee')
157
158 def test_list_deepnested(self):
159- self._run_check_error(RuntimeError, (b"l" * 10000) + (b"e" * 10000))
160+ with RecursionLimit():
161+ self._run_check_error(RuntimeError, (b"l" * 100) + (b"e" * 100))
162
163 def test_malformed_list(self):
164 self._run_check_error(ValueError, b'l')
165@@ -107,14 +123,9 @@
166 b'd8:spam.mp3d6:author5:Alice6:lengthi100000eee')
167
168 def test_dict_deepnested(self):
169- # The recursion here provokes CPython into emitting a warning on
170- # stderr, "maximum recursion depth exceeded in __subclasscheck__", due
171- # to running out of stack space while evaluating "except (...):" in
172- # _bencode_py. This is harmless, so we temporarily override stderr to
173- # avoid distracting noise in the test output.
174- self.overrideAttr(sys, 'stderr', self._log_file)
175- self._run_check_error(
176- RuntimeError, (b"d0:" * 10000) + b'i1e' + (b"e" * 10000))
177+ with RecursionLimit():
178+ self._run_check_error(
179+ RuntimeError, (b"d0:" * 1000) + b'i1e' + (b"e" * 1000))
180
181 def test_malformed_dict(self):
182 self._run_check_error(ValueError, b'd')
183@@ -184,10 +195,11 @@
184 def test_list_deep_nested(self):
185 top = []
186 l = top
187- for i in range(10000):
188+ for i in range(1000):
189 l.append([])
190 l = l[0]
191- self.assertRaises(RuntimeError, self.module.bencode, top)
192+ with RecursionLimit():
193+ self.assertRaises(RuntimeError, self.module.bencode, top)
194
195 def test_dict(self):
196 self._check(b'de', {})
197@@ -197,10 +209,11 @@
198
199 def test_dict_deep_nested(self):
200 d = top = {}
201- for i in range(10000):
202+ for i in range(1000):
203 d[b''] = {}
204 d = d[b'']
205- self.assertRaises(RuntimeError, self.module.bencode, top)
206+ with RecursionLimit():
207+ self.assertRaises(RuntimeError, self.module.bencode, top)
208
209 def test_bencached(self):
210 self._check(b'i3e', self.module.Bencached(self.module.bencode(3)))
211
212=== modified file 'python3.passing'
213--- python3.passing 2018-08-21 00:20:23 +0000
214+++ python3.passing 2018-08-21 00:57:35 +0000
215@@ -22151,6 +22151,7 @@
216 breezy.tests.test__bencode.TestBencodeDecode.test_decoder_type_error(C)
217 breezy.tests.test__bencode.TestBencodeDecode.test_decoder_type_error(python)
218 breezy.tests.test__bencode.TestBencodeDecode.test_dict(C)
219+breezy.tests.test__bencode.TestBencodeDecode.test_dict_deepnested(C)
220 breezy.tests.test__bencode.TestBencodeDecode.test_dict_deepnested(python)
221 breezy.tests.test__bencode.TestBencodeDecode.test_dict(python)
222 breezy.tests.test__bencode.TestBencodeDecode.test_empty_string(C)
223@@ -22162,6 +22163,7 @@
224 breezy.tests.test__bencode.TestBencodeDecode.test_large_string(C)
225 breezy.tests.test__bencode.TestBencodeDecode.test_large_string(python)
226 breezy.tests.test__bencode.TestBencodeDecode.test_list(C)
227+breezy.tests.test__bencode.TestBencodeDecode.test_list_deepnested(C)
228 breezy.tests.test__bencode.TestBencodeDecode.test_list_deepnested(python)
229 breezy.tests.test__bencode.TestBencodeDecode.test_list(python)
230 breezy.tests.test__bencode.TestBencodeDecode.test_long(C)

Subscribers

People subscribed via source and target branches