From b460b44d109d56ceaab115ee1561dbc2bc7a202c Mon Sep 17 00:00:00 2001 From: "Bjarni R. Einarsson" Date: Wed, 10 Jan 2018 11:32:12 +0100 Subject: [PATCH] Stop using assert() inappropriately. Fixes #1981 This patch also enables the Python optimizer (-OO) and hash randomization (-R) within the setup-test-sh script, just so those alternate code-paths get used and tested. Added .pyo files to .gitignore and `make clean`. --- .gitignore | 1 + Makefile | 1 + mailpile/auth.py | 11 +++++----- mailpile/config/base.py | 2 +- mailpile/crypto/records.py | 3 ++- mailpile/crypto/state.py | 7 +++--- mailpile/crypto/streamer.py | 4 ++-- mailpile/httpd.py | 6 ++++-- mailpile/i18n.py | 4 ++-- mailpile/mail_source/imap.py | 6 +++--- mailpile/mailutils/__init__.py | 4 ++-- mailpile/plugins/__init__.py | 8 +++---- mailpile/plugins/contacts.py | 4 ++-- mailpile/plugins/core.py | 2 +- mailpile/plugins/keylookup/__init__.py | 4 ++-- mailpile/plugins/setup_magic.py | 30 ++++++++++++++------------ mailpile/safe_popen.py | 9 ++++---- mailpile/security.py | 2 +- mailpile/util.py | 10 +++++++++ mailpile/vcard.py | 2 +- mailpile/vfs.py | 5 +++-- scripts/setup-test.sh | 3 ++- shared-data/contrib/gui/gui-o-matic.py | 6 ++++-- shared-data/contrib/hints/hints.py | 6 +++--- 24 files changed, 82 insertions(+), 58 deletions(-) diff --git a/.gitignore b/.gitignore index 509c8e66a..68ddea467 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ *.pyc +*.pyo *~ *.po *.debhelper diff --git a/Makefile b/Makefile index ead17f3a5..1668ff637 100644 --- a/Makefile +++ b/Makefile @@ -107,6 +107,7 @@ pytests: clean: @rm -f `find . -name \\*.pyc` \ + `find . -name \\*.pyo` \ `find . -name \\*.mo` \ mailpile-tmp.py mailpile.py \ ChangeLog AUTHORS \ diff --git a/mailpile/auth.py b/mailpile/auth.py index 73b0d0168..eb754b02e 100644 --- a/mailpile/auth.py +++ b/mailpile/auth.py @@ -40,8 +40,9 @@ def VerifyAndStorePassphrase(config, passphrase=None, sps=None, sps = SecurePassphraseStorage(passphrase) passphrase = 'this probably does not really overwrite :-( ' - assert(sps is not None) - assert(config.load_master_key(sps)) + safe_assert( + (sps is not None) and + (config.load_master_key(sps))) # Fun side effect: changing the passphrase invalidates the message cache import mailpile.mailutils @@ -141,7 +142,7 @@ def _do_redirect(self): # arbitrary URLs on the Internet. if path: url = urlparse(path) - assert(not url.scheme and not url.netloc) + safe_assert(not url.scheme and not url.netloc) if (path and not path[1:].startswith(DeAuthenticate.SYNOPSIS[2] or '!') and @@ -443,7 +444,7 @@ def command(self): if self.data.get('_method', None) == 'GET': return self._success(pass_prompt, result) - assert(fingerprint is not None) + safe_assert(fingerprint is not None) fingerprint = fingerprint.lower() if fingerprint in config.secrets: if config.secrets[fingerprint].policy == 'protect': @@ -461,7 +462,7 @@ def command(self): update_ms = update_mr = (account is not None) if update_ms or update_mr: - assert(account is not None) + safe_assert(account is not None) if not pass_check(password, account=account, fingerprint=fingerprint): result['error'] = _('Password incorrect! Try again?') diff --git a/mailpile/config/base.py b/mailpile/config/base.py index 6614a4791..b15b34c10 100644 --- a/mailpile/config/base.py +++ b/mailpile/config/base.py @@ -265,7 +265,7 @@ def reset(self, rules=True, data=True): raise Exception(_('Please override this method')) def set_rules(self, rules): - assert(isinstance(rules, dict)) + safe_assert(isinstance(rules, dict)) self.reset() for key, rule in rules.iteritems(): self.add_rule(key, rule) diff --git a/mailpile/crypto/records.py b/mailpile/crypto/records.py index a7cdaadf7..c73df96a1 100644 --- a/mailpile/crypto/records.py +++ b/mailpile/crypto/records.py @@ -191,7 +191,8 @@ def __init__(self, fn, key, if not self._parse_header(): self._write_header() - assert(max_bytes <= self._max_bytes) + if max_bytes > self._max_bytes: + raise AssertionError('max_bytes mismatch') def _calculate_constants(self): # Calculate our constants! Magic numbers: diff --git a/mailpile/crypto/state.py b/mailpile/crypto/state.py index 30510082d..496040004 100644 --- a/mailpile/crypto/state.py +++ b/mailpile/crypto/state.py @@ -48,16 +48,17 @@ def __init__(self, parent=None, copy=None, bubbly=True): def _set_status(self, value): if value not in self.STATUSES: print 'Bogus status for %s: %s' % (type(self), value) - assert(value in self.STATUSES) + raise ValueError('Invalid status: %s' % value) self._status = value self.mix_bubbles() def __setitem__(self, item, value): - assert(item in self.KEYS) + if item not in self.KEYS: + raise KeyError('Invalid key: %s' % item) if item == "status": if value not in self.STATUSES: print 'Bogus status for %s: %s' % (type(self), value) - assert(value in self.STATUSES) + raise ValueError('Invalid value for %s: %s' % (key, value)) if self._status is None: # Capture initial value self._status = value dict.__setitem__(self, item, value) diff --git a/mailpile/crypto/streamer.py b/mailpile/crypto/streamer.py index 9be1cbfbe..3076fd6d7 100644 --- a/mailpile/crypto/streamer.py +++ b/mailpile/crypto/streamer.py @@ -44,7 +44,7 @@ from mailpile.i18n import ngettext as _n from mailpile.crypto.gpgi import GPG_BINARY from mailpile.safe_popen import Popen, PIPE -from mailpile.util import CryptoLock, safe_remove +from mailpile.util import CryptoLock, safe_remove, safe_assert from mailpile.util import sha512b64 as genkey from mailpile.crypto.aes_utils import getrandbits @@ -952,7 +952,7 @@ def _mk_command(self): elif self.decryptor is not None: return None elif self.state == self.STATE_PGP_DATA: - assert(self.gpgi is not None) + safe_assert(self.gpgi is not None) if self.gpg_pass: return self.gpgi.common_args(will_send_passphrase=True) else: diff --git a/mailpile/httpd.py b/mailpile/httpd.py index dc6c63f48..3e2adcbd6 100644 --- a/mailpile/httpd.py +++ b/mailpile/httpd.py @@ -83,10 +83,12 @@ class HttpRequestHandler(SimpleXMLRPCRequestHandler): _HTML_RE = re.compile('[<>\'\"]+') def assert_no_newline(self, data): - assert(re.search(self._NEWLINE_RE, str(data) or '') is None) + if re.search(self._NEWLINE_RE, str(data) or '') is not None: + raise ValueError() def assert_no_html(self, data): - assert(re.search(self._HTML_RE, data or '') is None) + if re.search(self._HTML_RE, data or '') is not None: + raise ValueError() def send_header(self, hdr, value): self.assert_no_newline(value) diff --git a/mailpile/i18n.py b/mailpile/i18n.py index e77210fbb..d9651f2b2 100644 --- a/mailpile/i18n.py +++ b/mailpile/i18n.py @@ -22,8 +22,8 @@ def _fmt_safe(translation, original): return FORMAT_CHECKED[translation] if '%' in original: try: - assert(len([c for c in translation if c == '%']) - == len([c for c in original if c == '%'])) + safe_assert(len([c for c in translation if c == '%']) + == len([c for c in original if c == '%'])) bogon = translation % 1 FORMAT_CHECKED[translation] = translation except TypeError: diff --git a/mailpile/mail_source/imap.py b/mailpile/mail_source/imap.py index 00329fc14..46c949eff 100644 --- a/mailpile/mail_source/imap.py +++ b/mailpile/mail_source/imap.py @@ -177,7 +177,7 @@ def __init__(self, session, conn, idle_mailbox=None, idle_callback=None): def _mk_proxy(self, method): def proxy_method(*args, **kwargs): try: - assert(self._lock.locked()) + safe_assert(self._lock.locked()) if 'mailbox' in kwargs: # We're sharing this connection, so all mailbox methods # need to tell us which mailbox they're operating on. @@ -227,7 +227,7 @@ def _shutdown(self): self._update_name() def close(self): - assert(self._lock.locked()) + safe_assert(self._lock.locked()) self._selected = None if '(closed)' not in self.name: self.name += ' (closed)' @@ -237,7 +237,7 @@ def select(self, mailbox='INBOX', readonly=False): # This routine caches the SELECT operations, because we will be # making lots and lots of superfluous ones "just in case" as part # of multiplexing one IMAP connection for multiple mailboxes. - assert(self._lock.locked()) + safe_assert(self._lock.locked()) if self._selected and self._selected[0] == (mailbox, readonly): return self._selected[1] elif self._selected: diff --git a/mailpile/mailutils/__init__.py b/mailpile/mailutils/__init__.py index 7766dbc83..6177ef83d 100644 --- a/mailpile/mailutils/__init__.py +++ b/mailpile/mailutils/__init__.py @@ -157,7 +157,7 @@ def MakeGnuPG(*args, **kwargs): try: if not hasattr(fd, 'read'): # Not a file, is it a function? fd = fd() - assert(hasattr(fd, 'read')) + safe_assert(hasattr(fd, 'read')) except (TypeError, AssertionError): return None @@ -287,7 +287,7 @@ def PrepareMessage(config, msg, rcpts = rcpts or [] if bounce: - assert(len(rcpts) > 0) + safe_assert(len(rcpts) > 0) # Iterate through headers to figure out what we want to do... need_rcpts = not rcpts diff --git a/mailpile/plugins/__init__.py b/mailpile/plugins/__init__.py index 26a714393..72fa4145b 100644 --- a/mailpile/plugins/__init__.py +++ b/mailpile/plugins/__init__.py @@ -141,7 +141,7 @@ def discover(self, paths, update=False): try: with open(manifest_filename) as mfd: manifest = json.loads(self._uncomment(mfd.read())) - assert(manifest.get('name') == subdir) + safe_assert(manifest.get('name') == subdir) # FIXME: Need more sanity checks self.DISCOVERED[pname] = (plug_path, manifest) except (ValueError, AssertionError): @@ -167,7 +167,7 @@ def _import(self, full_name, full_path): sys.modules[mp] = imp.new_module(mp) sys.modules[module].__dict__[parent] = sys.modules[mp] module = mp - assert(module == full_name) + safe_assert(module == full_name) # load actual module sys.modules[full_name].__file__ = full_path @@ -715,8 +715,8 @@ def register_slow_periodic_job(self, name, period, callback): def register_worker(self, thread_obj): self._compat_check() - assert(hasattr(thread_obj, 'start')) - assert(hasattr(thread_obj, 'quit')) + safe_assert(hasattr(thread_obj, 'start')) + safe_assert(hasattr(thread_obj, 'quit')) # FIXME: complain about duplicates? self.WORKERS.append(thread_obj) diff --git a/mailpile/plugins/contacts.py b/mailpile/plugins/contacts.py index ca3e4831a..2e623b1ea 100644 --- a/mailpile/plugins/contacts.py +++ b/mailpile/plugins/contacts.py @@ -1287,9 +1287,9 @@ def command(self): session, config = self.session, self.session.config # OK, fetch the VCard. - assert('rid' in self.data and len(self.data['rid']) == 1) + safe_assert('rid' in self.data and len(self.data['rid']) == 1) vcard = config.vcards.get_vcard(self.data['rid'][0]) - assert(vcard) + safe_assert(vcard) if self.data.get('_method') == 'POST': self._update_vcard_from_post(vcard) diff --git a/mailpile/plugins/core.py b/mailpile/plugins/core.py index c067d9a49..a5ce208f9 100644 --- a/mailpile/plugins/core.py +++ b/mailpile/plugins/core.py @@ -476,7 +476,7 @@ class Cleanup(Command): @classmethod def AddTask(cls, task, last=False, first=False): - assert(not (first and last)) + safe_assert(not (first and last)) if (first or last) and not cls.TASKS: cls.TASKS = [lambda: True] if first: diff --git a/mailpile/plugins/keylookup/__init__.py b/mailpile/plugins/keylookup/__init__.py index 779bd07a5..2c1d0f630 100644 --- a/mailpile/plugins/keylookup/__init__.py +++ b/mailpile/plugins/keylookup/__init__.py @@ -255,7 +255,7 @@ def command(self): address = self.data.get('address', [''])[0] fprints = self.data.get('fingerprints', []) origins = self.data.get('origins', []) - assert(address or fprints or origins) + safe_assert(address or fprints or origins) result = lookup_crypto_keys(self.session, address, get=[f.strip() for f in fprints], @@ -308,7 +308,7 @@ def _seen_enough_signatures(self, idx, email, keyinfo): def command(self): emails = set(list(self.args)) | set(self.data.get('email', [])) - assert(emails) + safe_assert(emails) idx = self._idx() gnupg = self._gnupg(dry_run=True) diff --git a/mailpile/plugins/setup_magic.py b/mailpile/plugins/setup_magic.py index ff05e9e7c..267a20894 100644 --- a/mailpile/plugins/setup_magic.py +++ b/mailpile/plugins/setup_magic.py @@ -820,11 +820,12 @@ def _test_login_and_proto(self, email, settings): if settings['protocol'].startswith('smtp'): try: - assert(SendMail(self.session, None, - [(email, - [email, 'test@mailpile.is'], None, - [event])], - test_only=True, test_route=settings)) + safe_assert( + SendMail(self.session, None, + [(email, + [email, 'test@mailpile.is'], None, + [event])], + test_only=True, test_route=settings)) return True, True except (IOError, OSError, AssertionError, SendMailError): pass @@ -1161,7 +1162,7 @@ def setup_command(self, session): if route_id: route = self.session.config.routes[route_id] - assert(route) + safe_assert(route) else: route = {} for k in CONFIG_RULES['routes'][1]: @@ -1178,16 +1179,17 @@ def setup_command(self, session): if not fromaddr or '@' not in fromaddr: fromaddr = '%s@%s' % (route.get('username', 'test'), route.get('host', 'example.com')) - assert(fromaddr) + safe_assert(fromaddr) error_info = {'error': _('Unknown error')} try: - assert(SendMail(self.session, None, - [(fromaddr, - [fromaddr, 'test@mailpile.is'], - None, - [self.event])], - test_only=True, test_route=route)) + safe_assert( + SendMail(self.session, None, + [(fromaddr, + [fromaddr, 'test@mailpile.is'], + None, + [self.event])], + test_only=True, test_route=route)) return self._success(_('Route is working'), result=route) except OSError: @@ -1241,7 +1243,7 @@ def auto_configure_tor(self, session, hostport=None): with ConnBroker.context(need=need_tor) as context: motd = urlopen(MOTD_URL_TOR_ONLY_NO_MARS, data=None, timeout=10).read() - assert(motd.strip().endswith('}')) + safe_assert(motd.strip().endswith('}')) session.config.sys.proxy.protocol = 'tor' message = _('Successfully configured and enabled Tor!') except (IOError, AssertionError): diff --git a/mailpile/safe_popen.py b/mailpile/safe_popen.py index 6ba7bfc85..1397e19c1 100644 --- a/mailpile/safe_popen.py +++ b/mailpile/safe_popen.py @@ -85,10 +85,11 @@ def __init__(self, args, bufsize=0, # platforms, so we don't allow the programmer to configure them # at all. if SERIALIZE_POPEN_STRICT: - assert(preexec_fn is None) - assert(close_fds is None) - assert(startupinfo is None) - assert(creationflags is None) + if not ((preexec_fn is None) and + (close_fds is None) and + (startupinfo is None) and + (creationflags is None)): + raise AssertionError("Unsafe use of POpen API!") # The goal of the following sections is to achieve two things: # diff --git a/mailpile/security.py b/mailpile/security.py index 85ac7acc6..bb55a5bda 100644 --- a/mailpile/security.py +++ b/mailpile/security.py @@ -383,7 +383,7 @@ def __init__(self, sps): self.offset = 0 def seek(self, offset, whence=0): - assert(whence == 0) + safe_assert(whence == 0) self.offset = offset def read(self, ignored_bytecount=None): diff --git a/mailpile/util.py b/mailpile/util.py index 408ce9674..bc6e46b32 100644 --- a/mailpile/util.py +++ b/mailpile/util.py @@ -216,6 +216,10 @@ class AccessError(Exception): pass +class InternalError(AssertionError): + pass + + class UrlRedirectException(Exception): """An exception indicating we need to redirecting to another URL.""" def __init__(self, url): @@ -247,6 +251,12 @@ def __exit__(self, *args, **kwargs): raise raised[0] +def safe_assert(check, *args): + """A safe-to-use assert() replacement that never gets compiled out.""" + if not check: + raise InternalError(*args) + + def thread_context_push(**kwargs): if not hasattr(THREAD_LOCAL, 'context'): THREAD_LOCAL.context = [] diff --git a/mailpile/vcard.py b/mailpile/vcard.py index 5b7e20f67..1bfefa219 100644 --- a/mailpile/vcard.py +++ b/mailpile/vcard.py @@ -947,7 +947,7 @@ def recent_history(self, now=None): return {} def record_history(self, what, when, mid, now=None): - assert(what[0] in ('s', 'r')) + safe_assert(what[0] in ('s', 'r')) with self._lock: try: history_vcl = self.get('x-mailpile-history') diff --git a/mailpile/vfs.py b/mailpile/vfs.py index b1fe20778..ce5d24a45 100644 --- a/mailpile/vfs.py +++ b/mailpile/vfs.py @@ -23,6 +23,7 @@ import posixpath from mailpile.i18n import gettext as _ +from mailpile.util import safe_assert VFS_HANDLERS = [] @@ -36,7 +37,7 @@ def register_handler(prio, obj): def register_alias(name, prefix): global VFS_ALIASES - assert(name[:1] == '/') + safe_assert(name[:1] == '/') VFS_ALIASES[name] = prefix @@ -51,7 +52,7 @@ class FilePath(object): is suitable for writing to a config file or JSON stream. """ def __init__(self, cooked_fp=None, binary_fp=None, flags=None): - assert((cooked_fp or binary_fp) and not (cooked_fp and binary_fp)) + safe_assert((cooked_fp or binary_fp) and not (cooked_fp and binary_fp)) if cooked_fp: if isinstance(cooked_fp, FilePath): self.raw_fp = cooked_fp.raw_fp diff --git a/scripts/setup-test.sh b/scripts/setup-test.sh index e1d0575e0..8f9c28c62 100755 --- a/scripts/setup-test.sh +++ b/scripts/setup-test.sh @@ -42,7 +42,8 @@ if [ "$1" = "--cleanup" ]; then fi [ "$1" = "" ] && IA="--interact" || IA="" -$PYTHON ./mp --set 'sys.debug = log http' \ +$PYTHON -OOR ./mp \ + --set 'sys.debug = log http' \ --set "sys.gpg_binary = $GPG_BINARY" \ --pidfile "$MAILPILE_HOME/mailpile.pid" \ --www 'localhost:33433' \ diff --git a/shared-data/contrib/gui/gui-o-matic.py b/shared-data/contrib/gui/gui-o-matic.py index a6694c2da..30c33b161 100755 --- a/shared-data/contrib/gui/gui-o-matic.py +++ b/shared-data/contrib/gui/gui-o-matic.py @@ -12,6 +12,8 @@ import urllib import webbrowser +from mailpile.util import safe_assert + ##[ Parent Indicator class, interface and common helpers ]#################### @@ -99,7 +101,7 @@ def _get_webview(self): return None def show_url(self, url=None): - assert(url is not None) + safe_assert(url is not None) if not self.config.get('external_browser'): webview = self._get_webview() if webview: @@ -335,7 +337,7 @@ def run(self): def MacOSXIndicator(): - assert(objc is not None) + safe_assert(objc is not None) class MacOSXThing(NSObject): indicator = None diff --git a/shared-data/contrib/hints/hints.py b/shared-data/contrib/hints/hints.py index 0b75ab4d7..94f4b7812 100644 --- a/shared-data/contrib/hints/hints.py +++ b/shared-data/contrib/hints/hints.py @@ -13,7 +13,7 @@ import random from mailpile.config.defaults import APPVER from mailpile.commands import Command -from mailpile.util import md5_hex +from mailpile.util import md5_hex, safe_assert TIMESTAMPS = None @@ -163,14 +163,14 @@ def command(self): ctx = self.data.get('context') if 'reset' in self.args: - assert(self.data.get('_method', 'POST') == 'POST') + safe_assert(self.data.get('_method', 'POST') == 'POST') ts = self.timestamps() for k in ts.keys(): del ts[k] ts['initial'] = self._today() elif 'next' in self.args: - assert(self.data.get('_method', 'POST') == 'POST') + safe_assert(self.data.get('_method', 'POST') == 'POST') self.timestamps()['last_displayed'] = 0 self.timestamps()['initial'] -= 30