From 20d920b07625ec5780b3785035a0280c3b317ba4 Mon Sep 17 00:00:00 2001
From: Accela Zhao <accelazh@gmail.com>
Date: Wed, 3 Feb 2016 18:11:09 -0800
Subject: [PATCH] Fix variable scope issue in try-except

Below piece of code in cinder.backup.api.API.create does not handle
the scope of variable 'backup' properly. In general variable used in
except clause has larger scope than what is in try clause.

```
    try:
        ...
        backup = objects.Backup(context=context, **kwargs)
        backup.create()
        ...
    except Exception:
            ...
            backup.destroy()
            ...
```

The first problem: if object.Backup.__init__ raises exception,
variable 'backup' won't be assigned. However the cleanup code invokes
backup.destroy(), which results in an UnboundLocalError. The original
exception of __init__ is dropped, instead UnboundLocalError is raised
up-level. I know that for now objects.Backup.__init__ doesn't have
chance to raise exception, but when writing code we should not rely
on such assumption.

The second problem: backup.create() invokes DB. If an DB exception is
raised, backup.id won't be assigned. Any access to backup.id will
result in an "NotImplementedError: Cannot load 'id' in the base
class". However, when cleaning up, the backup.destroy() does access
backup.id. So we have the similar problem as the first one, and the
original exception is overrode by NotImplementedError.

This patch fixes it by adding a first-assignment outside try clause,
and add a safe check in except clause. The added test cases, if run
on the original code, it will show how the above problem occurs.

Closes-Bug: #1541745
Change-Id: I0e55c342ac489ff1ef27c0597b6dcba588778a04
---
 cinder/backup/api.py             |  5 +++-
 cinder/tests/unit/test_backup.py | 49 ++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/cinder/backup/api.py b/cinder/backup/api.py
index 258d56d05..c39004222 100644
--- a/cinder/backup/api.py
+++ b/cinder/backup/api.py
@@ -271,6 +271,8 @@ class API(base.Base):
         self.db.volume_update(context, volume_id,
                               {'status': 'backing-up',
                                'previous_status': previous_status})
+
+        backup = None
         try:
             kwargs = {
                 'user_id': context.user_id,
@@ -295,7 +297,8 @@ class API(base.Base):
         except Exception:
             with excutils.save_and_reraise_exception():
                 try:
-                    backup.destroy()
+                    if backup and 'id' in backup:
+                        backup.destroy()
                 finally:
                     QUOTAS.rollback(context, reservations)
 
diff --git a/cinder/tests/unit/test_backup.py b/cinder/tests/unit/test_backup.py
index 18d0c507e..28e91bac0 100644
--- a/cinder/tests/unit/test_backup.py
+++ b/cinder/tests/unit/test_backup.py
@@ -20,6 +20,7 @@ import uuid
 
 import mock
 from oslo_config import cfg
+from oslo_db import exception as db_exc
 from oslo_utils import importutils
 from oslo_utils import timeutils
 
@@ -32,6 +33,7 @@ from cinder import objects
 from cinder.objects import fields
 from cinder import test
 from cinder.tests.unit.backup import fake_service_with_verify as fake_service
+from cinder.tests.unit import utils
 from cinder.volume.drivers import lvm
 
 
@@ -1224,3 +1226,50 @@ class BackupAPITestCase(BaseBackupTest):
         mock_backuplist.get_all_by_project.assert_called_once_with(
             ctxt, ctxt.project_id, {'key': 'value'}, None, None, None, None,
             None)
+
+    @mock.patch.object(api.API, '_is_backup_service_enabled',
+                       return_value=True)
+    @mock.patch.object(db, 'backup_create',
+                       side_effect=db_exc.DBError())
+    def test_create_when_failed_to_create_backup_object(
+            self, mock_create,
+            mock_service_enabled):
+        volume_id = utils.create_volume(self.ctxt)['id']
+        self.ctxt.user_id = 'user_id'
+        self.ctxt.project_id = 'project_id'
+
+        # The opposite side of this test case is a "NotImplementedError:
+        # Cannot load 'id' in the base class" being raised.
+        # More detailed, in the try clause, if backup.create() failed
+        # with DB exception, backup.id won't be assigned. However,
+        # in the except clause, backup.destroy() is invoked to do cleanup,
+        # which internally tries to access backup.id.
+        self.assertRaises(db_exc.DBError, self.api.create,
+                          context=self.ctxt,
+                          name="test_backup",
+                          description="test backup description",
+                          volume_id=volume_id,
+                          container='volumebackups')
+
+    @mock.patch.object(api.API, '_is_backup_service_enabled',
+                       return_value=True)
+    @mock.patch.object(objects.Backup, '__init__',
+                       side_effect=exception.InvalidInput(
+                           reason='Failed to new'))
+    def test_create_when_failed_to_new_backup_object(self, mock_new,
+                                                     mock_service_enabled):
+        volume_id = utils.create_volume(self.ctxt)['id']
+        self.ctxt.user_id = 'user_id'
+        self.ctxt.project_id = 'project_id'
+
+        # The opposite side of this test case is that a "UnboundLocalError:
+        # local variable 'backup' referenced before assignment" is raised.
+        # More detailed, in the try clause, backup = objects.Backup(...)
+        # raises exception, so 'backup' is not assigned. But in the except
+        # clause, 'backup' is referenced to invoke cleanup methods.
+        self.assertRaises(exception.InvalidInput, self.api.create,
+                          context=self.ctxt,
+                          name="test_backup",
+                          description="test backup description",
+                          volume_id=volume_id,
+                          container='volumebackups')
-- 
2.45.2