From deffb09871c9460497929c57bd82ca78d1c3177f Mon Sep 17 00:00:00 2001
From: Chris Dent <cdent@anticdent.org>
Date: Thu, 13 Dec 2018 20:56:39 +0000
Subject: [PATCH] Make QuotaImageTagsProxy deep-copyable

Infinite recursion is possible if you try to deepcopy a
QuotaImageTagsProxy object, which is something that happens when using
oslo.policy 1.43.1 and beyond.

By adding a __gettattribute__ within the __getattr__ method we can break
the loop when the name of the attribute is 'tags'.

The added test will fail (with infinite recursion) if the new code is
removed.

For completeness a test is added checking what happens when the tags
attribute is removed.

The thing that is not immediately clear from the __getattr__ code is
that it is not for getting at the tags themselves, it is for getting
at the methods on the set object at self.tags. The non-obviousness
of this led to rather a lot of confusion while trying to get this
code right. It isn't really "right" now, but rather: safe in the
context of its use.

Change-Id: Ic47c9bf8e9b97de5a5a49a35f631753c54e0e2af
Related-Bug: #1808063
---
 glance/quota/__init__.py        | 17 +++++++++--------
 glance/tests/unit/test_quota.py | 22 ++++++++++++++++------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/glance/quota/__init__.py b/glance/quota/__init__.py
index 4560307cd0..32ba9837a1 100644
--- a/glance/quota/__init__.py
+++ b/glance/quota/__init__.py
@@ -165,14 +165,15 @@ class QuotaImageTagsProxy(object):
         return self.tags.__len__(*args, **kwargs)
 
     def __getattr__(self, name):
-        # Use TypeError here, not AttributeError, as the latter is how we
-        # know a tag is not present. TypeError says "this object is not
-        # what it claims to be".
-        try:
-            tags = self.__getattribute__('tags')
-        except AttributeError:
-            raise TypeError('QuotaImageTagsProxy has no tags.')
-        return getattr(tags, name)
+        # Protect against deepcopy, which calls getattr. __getattr__
+        # is only called when an attribute is not "normal", so when
+        # self.tags is called, this is not.
+        if name == 'tags':
+            try:
+                return self.__getattribute__('tags')
+            except AttributeError:
+                return None
+        return getattr(self.tags, name)
 
 
 class ImageMemberFactoryProxy(glance.domain.proxy.ImageMembershipFactory):
diff --git a/glance/tests/unit/test_quota.py b/glance/tests/unit/test_quota.py
index fff5b1daa7..51d0295e10 100644
--- a/glance/tests/unit/test_quota.py
+++ b/glance/tests/unit/test_quota.py
@@ -12,6 +12,7 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
+import copy
 import uuid
 
 import mock
@@ -596,15 +597,24 @@ class TestQuotaImageTagsProxy(test_utils.BaseTestCase):
             items.remove(item)
         self.assertEqual(0, len(items))
 
-    def test_tags_attr_exception(self):
+    def test_tags_attr_no_loop(self):
         proxy = glance.quota.QuotaImageTagsProxy(None)
-        self.assertRaises(AttributeError, lambda: proxy.foo)
+        self.assertEqual(set([]), proxy.tags)
 
-        # Remove tags to cause the object to be broken. If tags
-        # is not there and we weren't raising TypeError, we'd
-        # get an infinite loop when calling 'proxy.foo'.
+    def test_tags_deepcopy(self):
+        proxy = glance.quota.QuotaImageTagsProxy(set(['a', 'b']))
+        proxy_copy = copy.deepcopy(proxy)
+        self.assertEqual(set(['a', 'b']), proxy_copy.tags)
+        self.assertIn('a', proxy_copy)
+        # remove is a found via __getattr__
+        proxy_copy.remove('a')
+        self.assertNotIn('a', proxy_copy)
+
+    def test_tags_delete(self):
+        proxy = glance.quota.QuotaImageTagsProxy(set(['a', 'b']))
+        self.assertEqual(set(['a', 'b']), proxy.tags)
         del proxy.tags
-        self.assertRaises(TypeError, lambda: proxy.foo)
+        self.assertIsNone(proxy.tags)
 
 
 class TestImageMemberQuotas(test_utils.BaseTestCase):