From 175f3b51e528f72068d6e167d4f7892bc132a380 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Sun, 9 Sep 2018 22:16:05 -0700 Subject: [PATCH] Ensure that current uses of BaseException are required * In some cases, it appears that Exception should have been used instead as there's no need to catch sys.exit KeyboardInterrupt and similar. * In a few cases, it appears that BaseException is used because a library we depend on calls sys.exit() contrary to good coding design. Comment those so that we know that those have been audited and found to be correct and change to use (Exception, SystemExit) instead. --- contrib/inventory/zabbix.py | 4 +++- lib/ansible/module_utils/vmware.py | 4 ++-- .../cloud/amazon/rds_snapshot_facts.py | 2 +- .../cloud/univention/udm_dns_record.py | 4 ++-- .../modules/cloud/univention/udm_share.py | 4 ++-- .../monitoring/zabbix/zabbix_maintenance.py | 21 ++++++++++++------- lib/ansible/plugins/callback/logdna.py | 8 +++---- test/sanity/import/importer.py | 2 ++ .../cloudvision/test_cv_server_provision.py | 2 +- 9 files changed, 31 insertions(+), 20 deletions(-) diff --git a/contrib/inventory/zabbix.py b/contrib/inventory/zabbix.py index 8de20ea582f..43a539a0067 100755 --- a/contrib/inventory/zabbix.py +++ b/contrib/inventory/zabbix.py @@ -173,7 +173,9 @@ class ZabbixInventory(object): try: api = ZabbixAPI(server=self.zabbix_server, validate_certs=self.validate_certs) api.login(user=self.zabbix_username, password=self.zabbix_password) - except BaseException as e: + # zabbix_api tries to exit if it cannot parse what the zabbix server returned + # so we have to use SystemExit here + except (Exception, SystemExit) as e: print("Error: Could not login to Zabbix server. Check your zabbix.ini.", file=sys.stderr) sys.exit(1) diff --git a/lib/ansible/module_utils/vmware.py b/lib/ansible/module_utils/vmware.py index 87d4c5ff51e..6b4f96260e7 100644 --- a/lib/ansible/module_utils/vmware.py +++ b/lib/ansible/module_utils/vmware.py @@ -321,7 +321,7 @@ def gather_vm_facts(content, vm): for item in vm.layout.disk: for disk in item.diskFile: facts['hw_files'].append(disk) - except BaseException: + except Exception: pass facts['hw_folder'] = PyVmomi.get_vm_path(content, vm) @@ -968,7 +968,7 @@ class PyVmomi(object): folder_name = fp.name + '/' + folder_name try: fp = fp.parent - except BaseException: + except Exception: break folder_name = '/' + folder_name return folder_name diff --git a/lib/ansible/modules/cloud/amazon/rds_snapshot_facts.py b/lib/ansible/modules/cloud/amazon/rds_snapshot_facts.py index bfad0502a66..81df78014f6 100644 --- a/lib/ansible/modules/cloud/amazon/rds_snapshot_facts.py +++ b/lib/ansible/modules/cloud/amazon/rds_snapshot_facts.py @@ -289,7 +289,7 @@ from ansible.module_utils.ec2 import AWSRetry, boto3_tag_list_to_ansible_dict, c try: import botocore -except BaseException: +except Exception: pass # caught by imported HAS_BOTO3 diff --git a/lib/ansible/modules/cloud/univention/udm_dns_record.py b/lib/ansible/modules/cloud/univention/udm_dns_record.py index 5ad7b90b441..ee5cf65384a 100644 --- a/lib/ansible/modules/cloud/univention/udm_dns_record.py +++ b/lib/ansible/modules/cloud/univention/udm_dns_record.py @@ -159,7 +159,7 @@ def main(): obj.create() else: obj.modify() - except BaseException as e: + except Exception as e: module.fail_json( msg='Creating/editing dns entry {} in {} failed: {}'.format(name, container, e) ) @@ -170,7 +170,7 @@ def main(): if not module.check_mode: obj.remove() changed = True - except BaseException as e: + except Exception as e: module.fail_json( msg='Removing dns entry {} in {} failed: {}'.format(name, container, e) ) diff --git a/lib/ansible/modules/cloud/univention/udm_share.py b/lib/ansible/modules/cloud/univention/udm_share.py index a90e8382328..0d5928f5f39 100644 --- a/lib/ansible/modules/cloud/univention/udm_share.py +++ b/lib/ansible/modules/cloud/univention/udm_share.py @@ -513,7 +513,7 @@ def main(): obj.create() elif changed: obj.modify() - except BaseException as err: + except Exception as err: module.fail_json( msg='Creating/editing share {} in {} failed: {}'.format( name, @@ -528,7 +528,7 @@ def main(): if not module.check_mode: obj.remove() changed = True - except BaseException as err: + except Exception as err: module.fail_json( msg='Removing share {} in {} failed: {}'.format( name, diff --git a/lib/ansible/modules/monitoring/zabbix/zabbix_maintenance.py b/lib/ansible/modules/monitoring/zabbix/zabbix_maintenance.py index ad896e73642..1d7472bb61c 100644 --- a/lib/ansible/modules/monitoring/zabbix/zabbix_maintenance.py +++ b/lib/ansible/modules/monitoring/zabbix/zabbix_maintenance.py @@ -152,7 +152,8 @@ def create_maintenance(zbx, group_ids, host_ids, start_time, maintenance_type, p }] } ) - except BaseException as e: + # zabbix_api can call sys.exit() so we need to catch SystemExit here + except (Exception, SystemExit) as e: return 1, None, str(e) return 0, None, None @@ -176,7 +177,8 @@ def update_maintenance(zbx, maintenance_id, group_ids, host_ids, start_time, mai }] } ) - except BaseException as e: + # zabbix_api can call sys.exit() so we need to catch SystemExit here + except (Exception, SystemExit) as e: return 1, None, str(e) return 0, None, None @@ -193,7 +195,8 @@ def get_maintenance(zbx, name): "selectHosts": "extend" } ) - except BaseException as e: + # zabbix_api can call sys.exit() so we need to catch SystemExit here + except (Exception, SystemExit) as e: return 1, None, str(e) for maintenance in maintenances: @@ -207,7 +210,8 @@ def get_maintenance(zbx, name): def delete_maintenance(zbx, maintenance_id): try: zbx.maintenance.delete([maintenance_id]) - except BaseException as e: + # zabbix_api can call sys.exit() so we need to catch SystemExit here + except (Exception, SystemExit) as e: return 1, None, str(e) return 0, None, None @@ -225,7 +229,8 @@ def get_group_ids(zbx, host_groups): } } ) - except BaseException as e: + # zabbix_api can call sys.exit() so we need to catch SystemExit here + except (Exception, SystemExit) as e: return 1, None, str(e) if not result: @@ -249,7 +254,8 @@ def get_host_ids(zbx, host_names): } } ) - except BaseException as e: + # zabbix_api can call sys.exit() so we need to catch SystemExit here + except (Exception, SystemExit) as e: return 1, None, str(e) if not result: @@ -308,7 +314,8 @@ def main(): zbx = ZabbixAPI(server_url, timeout=timeout, user=http_login_user, passwd=http_login_password, validate_certs=validate_certs) zbx.login(login_user, login_password) - except BaseException as e: + # zabbix_api can call sys.exit() so we need to catch SystemExit here + except (Exception, SystemExit) as e: module.fail_json(msg="Failed to connect to Zabbix server: %s" % e) changed = False diff --git a/lib/ansible/plugins/callback/logdna.py b/lib/ansible/plugins/callback/logdna.py index 56db71f862e..9d267528524 100644 --- a/lib/ansible/plugins/callback/logdna.py +++ b/lib/ansible/plugins/callback/logdna.py @@ -84,12 +84,12 @@ def get_hostname(): def get_ip(): try: return socket.gethostbyname(get_hostname()) - except BaseException: + except Exception: s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) try: s.connect(('10.255.255.255', 1)) IP = s.getsockname()[0] - except BaseException: + except Exception: IP = '127.0.0.1' finally: s.close() @@ -101,7 +101,7 @@ def isJSONable(obj): try: json.dumps(obj, sort_keys=True, cls=AnsibleJSONEncoder) return True - except BaseException: + except Exception: return False @@ -165,7 +165,7 @@ class CallbackModule(CallbackBase): def sanitizeJSON(self, data): try: return json.loads(json.dumps(data, sort_keys=True, cls=AnsibleJSONEncoder)) - except BaseException: + except Exception: return {'warnings': ['JSON Formatting Issue', json.dumps(data, sort_keys=True, cls=AnsibleJSONEncoder)]} def flush(self, log, options): diff --git a/test/sanity/import/importer.py b/test/sanity/import/importer.py index e594759d16e..c8ffbe93e3b 100755 --- a/test/sanity/import/importer.py +++ b/test/sanity/import/importer.py @@ -99,6 +99,8 @@ def test_python_module(path, base_dir, messages, ansible_module): except ImporterAnsibleModuleException: # module instantiated AnsibleModule without raising an exception pass + # We truly want to catch anything the plugin might do here, including call sys.exit() so we + # catch BaseException except BaseException as ex: # pylint: disable=locally-disabled, broad-except capture_report(path, capture, messages) diff --git a/test/units/modules/network/cloudvision/test_cv_server_provision.py b/test/units/modules/network/cloudvision/test_cv_server_provision.py index 0131352fe52..f9017f14583 100644 --- a/test/units/modules/network/cloudvision/test_cv_server_provision.py +++ b/test/units/modules/network/cloudvision/test_cv_server_provision.py @@ -23,7 +23,7 @@ sys.modules['cvprac.cvp_client_errors'] = Mock() from ansible.modules.network.cloudvision import cv_server_provision -class MockException(BaseException): +class MockException(Exception): pass