1 This upstream patch addresses CVE-2016-0738 which is filed under |
|
2 Launchpad bug 1493303. The issue is addressed in Swift 2.3.1 as well as |
|
3 2.5.1 or later. |
|
4 |
|
5 From a4c1825a026655b7ed21d779824ae7c25318fd52 Mon Sep 17 00:00:00 2001 |
|
6 From: Samuel Merritt <[email protected]> |
|
7 Date: Tue, 8 Dec 2015 16:36:05 -0800 |
|
8 Subject: Fix memory/socket leak in proxy on truncated SLO/DLO GET |
|
9 |
|
10 When a client disconnected while consuming an SLO or DLO GET response, |
|
11 the proxy would leak a socket. This could be observed via strace as a |
|
12 socket that had shutdown() called on it, but was never closed. It |
|
13 could also be observed by counting entries in /proc/<pid>/fd, where |
|
14 <pid> is the pid of a proxy server worker process. |
|
15 |
|
16 This is due to a memory leak in SegmentedIterable. A SegmentedIterable |
|
17 has an 'app_iter' attribute, which is a generator. That generator |
|
18 references 'self' (the SegmentedIterable object). This creates a |
|
19 cyclic reference: the generator refers to the SegmentedIterable, and |
|
20 the SegmentedIterable refers to the generator. |
|
21 |
|
22 Python can normally handle cyclic garbage; reference counting won't |
|
23 reclaim it, but the garbage collector will. However, objects with |
|
24 finalizers will stop the garbage collector from collecting them* and |
|
25 the cycle of which they are part. |
|
26 |
|
27 For most objects, "has finalizer" is synonymous with "has a __del__ |
|
28 method". However, a generator has a finalizer once it's started |
|
29 running and before it finishes: basically, while it has stack frames |
|
30 associated with it**. |
|
31 |
|
32 When a client disconnects mid-stream, we get a memory leak. We have |
|
33 our SegmentedIterable object (call it "si"), and its associated |
|
34 generator. si.app_iter is the generator, and the generator closes over |
|
35 si, so we have a cycle; and the generator has started but not yet |
|
36 finished, so the generator needs finalization; hence, the garbage |
|
37 collector won't ever clean it up. |
|
38 |
|
39 The socket leak comes in because the generator *also* refers to the |
|
40 request's WSGI environment, which contains wsgi.input, which |
|
41 ultimately refers to a _socket object from the standard |
|
42 library. Python's _socket objects only close their underlying file |
|
43 descriptor when their reference counts fall to 0***. |
|
44 |
|
45 This commit makes SegmentedIterable.close() call |
|
46 self.app_iter.close(), thereby unwinding its generator's stack and |
|
47 making it eligible for garbage collection. |
|
48 |
|
49 * in Python < 3.4, at least. See PEP 442. |
|
50 |
|
51 ** see PyGen_NeedsFinalizing() in Objects/genobject.c and also |
|
52 has_finalizer() in Modules/gcmodule.c in Python. |
|
53 |
|
54 *** see sock_dealloc() in Modules/socketmodule.c in Python. See |
|
55 sock_close() in the same file for the other half of the sad story. |
|
56 |
|
57 This closes CVE-2016-0738. |
|
58 |
|
59 Closes-Bug: 1493303 |
|
60 |
|
61 Change-Id: I9b617bfc152dca40d1750131d1d814d85c0a88dd |
|
62 Co-Authored-By: Kota Tsuyuzaki <[email protected]> |
|
63 --- |
|
64 swift/common/request_helpers.py | 6 ++-- |
|
65 test/unit/common/middleware/test_slo.py | 62 +++++++++++++++++++++++++++++++++ |
|
66 2 files changed, 66 insertions(+), 2 deletions(-) |
|
67 |
|
68 diff --git a/swift/common/request_helpers.py b/swift/common/request_helpers.py |
|
69 index 8aa8457..611ee83 100644 |
|
70 --- a/swift/common/request_helpers.py |
|
71 +++ b/swift/common/request_helpers.py |
|
72 @@ -378,6 +378,9 @@ class SegmentedIterable(object): |
|
73 self.logger.exception(_('ERROR: An error occurred ' |
|
74 'while retrieving segments')) |
|
75 raise |
|
76 + finally: |
|
77 + if self.current_resp: |
|
78 + close_if_possible(self.current_resp.app_iter) |
|
79 |
|
80 def app_iter_range(self, *a, **kw): |
|
81 """ |
|
82 @@ -420,5 +423,4 @@ class SegmentedIterable(object): |
|
83 Called when the client disconnect. Ensure that the connection to the |
|
84 backend server is closed. |
|
85 """ |
|
86 - if self.current_resp: |
|
87 - close_if_possible(self.current_resp.app_iter) |
|
88 + close_if_possible(self.app_iter) |
|
89 diff --git a/test/unit/common/middleware/test_slo.py b/test/unit/common/middleware/test_slo.py |
|
90 index 4d483c8..8119b5f 100644 |
|
91 --- a/test/unit/common/middleware/test_slo.py |
|
92 +++ b/test/unit/common/middleware/test_slo.py |
|
93 @@ -1253,6 +1253,68 @@ class TestSloGetManifest(SloTestCase): |
|
94 self.assertEqual(headers['X-Object-Meta-Fish'], 'Bass') |
|
95 self.assertEqual(body, '') |
|
96 |
|
97 + def test_generator_closure(self): |
|
98 + # Test that the SLO WSGI iterable closes its internal .app_iter when |
|
99 + # it receives a close() message. |
|
100 + # |
|
101 + # This is sufficient to fix a memory leak. The memory leak arises |
|
102 + # due to cyclic references involving a running generator; a running |
|
103 + # generator sometimes preventes the GC from collecting it in the |
|
104 + # same way that an object with a defined __del__ does. |
|
105 + # |
|
106 + # There are other ways to break the cycle and fix the memory leak as |
|
107 + # well; calling .close() on the generator is sufficient, but not |
|
108 + # necessary. However, having this test is better than nothing for |
|
109 + # preventing regressions. |
|
110 + leaks = [0] |
|
111 + |
|
112 + class LeakTracker(object): |
|
113 + def __init__(self, inner_iter): |
|
114 + leaks[0] += 1 |
|
115 + self.inner_iter = iter(inner_iter) |
|
116 + |
|
117 + def __iter__(self): |
|
118 + return self |
|
119 + |
|
120 + def next(self): |
|
121 + return next(self.inner_iter) |
|
122 + |
|
123 + def close(self): |
|
124 + leaks[0] -= 1 |
|
125 + self.inner_iter.close() |
|
126 + |
|
127 + class LeakTrackingSegmentedIterable(slo.SegmentedIterable): |
|
128 + def _internal_iter(self, *a, **kw): |
|
129 + it = super( |
|
130 + LeakTrackingSegmentedIterable, self)._internal_iter( |
|
131 + *a, **kw) |
|
132 + return LeakTracker(it) |
|
133 + |
|
134 + status = [None] |
|
135 + headers = [None] |
|
136 + |
|
137 + def start_response(s, h, ei=None): |
|
138 + status[0] = s |
|
139 + headers[0] = h |
|
140 + |
|
141 + req = Request.blank( |
|
142 + '/v1/AUTH_test/gettest/manifest-abcd', |
|
143 + environ={'REQUEST_METHOD': 'GET', |
|
144 + 'HTTP_ACCEPT': 'application/json'}) |
|
145 + |
|
146 + # can't self.call_slo() here since we don't want to consume the |
|
147 + # whole body |
|
148 + with patch.object(slo, 'SegmentedIterable', |
|
149 + LeakTrackingSegmentedIterable): |
|
150 + app_resp = self.slo(req.environ, start_response) |
|
151 + self.assertEqual(status[0], '200 OK') # sanity check |
|
152 + body_iter = iter(app_resp) |
|
153 + chunk = next(body_iter) |
|
154 + self.assertEqual(chunk, 'aaaaa') # sanity check |
|
155 + |
|
156 + app_resp.close() |
|
157 + self.assertEqual(0, leaks[0]) |
|
158 + |
|
159 def test_head_manifest_is_efficient(self): |
|
160 req = Request.blank( |
|
161 '/v1/AUTH_test/gettest/manifest-abcd', |
|
162 -- |
|
163 cgit v0.11.2 |
|
164 |
|