From e839e4ea191a0eef15661d1a164bee6302d8f800 Mon Sep 17 00:00:00 2001
From: Carl Suster <carl@contraflo.ws>
Date: Tue, 2 Apr 2019 09:39:07 +1100
Subject: [PATCH] bpd: improve exception handling

Check function signature instead of using TypeError to crudely guess
that the wrong number of arguments were provided.

Prevent bpd from crashing when trying to log a traceback. The
`traceback.format_exc` function takes an optional argument which is
supposed to be an integer restricting the length of the backtrace to
show. Instead we were passing the exception object to this function and
causing a new exception to be raised.
---
 beetsplug/bpd/__init__.py | 29 +++++++++++++++++++----------
 test/test_player.py       | 12 +++++++++++-
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py
index 930710e8b..52619e7e1 100644
--- a/beetsplug/bpd/__init__.py
+++ b/beetsplug/bpd/__init__.py
@@ -31,7 +31,7 @@ import beets
 from beets.plugins import BeetsPlugin
 import beets.ui
 from beets import vfs
-from beets.util import bluelet
+from beets.util import bluelet, inspect
 from beets.library import Item
 from beets import dbcore
 from beets.mediafile import MediaFile
@@ -613,12 +613,17 @@ class BaseServer(object):
         index = self._id_to_index(track_id)
         return self.cmd_seek(conn, index, pos)
 
+    # Debugging/testing commands that are not part of the MPD protocol.
+
     def cmd_profile(self, conn):
         """Memory profiling for debugging."""
         from guppy import hpy
         heap = hpy().heap()
         print(heap)
 
+    def cmd_crash_TypeError(self, conn):
+        'a' + 2
+
 
 class Connection(object):
     """A connection between a client and the server. Handles input and
@@ -744,6 +749,17 @@ class Command(object):
             raise BPDError(ERROR_UNKNOWN,
                            u'unknown command "{}"'.format(self.name))
         func = getattr(conn.server, func_name)
+        argspec = inspect.getargspec(func)
+        max_args = len(argspec.args) - 1
+        min_args = max_args
+        if argspec.defaults:
+            min_args -= len(argspec.defaults)
+        wrong_num = (len(self.args) > max_args) or (len(self.args) < min_args)
+        if wrong_num and not argspec.varargs:
+            raise BPDError(ERROR_ARG,
+                           u'wrong number of arguments for "{}"'
+                           .format(self.name),
+                           self.name)
 
         # Ensure we have permission for this command.
         if conn.server.password and \
@@ -758,13 +774,6 @@ class Command(object):
                 for data in results:
                     yield conn.send(data)
 
-        except TypeError:
-            # The client provided too many arguments.
-            raise BPDError(ERROR_ARG,
-                           u'wrong number of arguments for "{}"'
-                           .format(self.name),
-                           self.name)
-
         except BPDError as e:
             # An exposed error. Set the command name and then let
             # the Connection handle it.
@@ -776,9 +785,9 @@ class Command(object):
             # it on the Connection.
             raise
 
-        except Exception as e:
+        except Exception:
             # An "unintentional" error. Hide it from the client.
-            conn.server._log.error('{}', traceback.format_exc(e))
+            conn.server._log.error('{}', traceback.format_exc())
             raise BPDError(ERROR_SYSTEM, u'server error', self.name)
 
 
diff --git a/test/test_player.py b/test/test_player.py
index ecf08c7b3..98fd13f63 100644
--- a/test/test_player.py
+++ b/test/test_player.py
@@ -354,9 +354,19 @@ class BPDTest(BPDTestHelper):
 
     def test_unexpected_argument(self):
         with self.run_bpd() as client:
-            response = client.send_command('clearerror', 'extra argument')
+            response = client.send_command('ping', 'extra argument')
         self._assert_failed(response, bpd.ERROR_ARG)
 
+    def test_missing_argument(self):
+        with self.run_bpd() as client:
+            response = client.send_command('add')
+        self._assert_failed(response, bpd.ERROR_ARG)
+
+    def test_system_error(self):
+        with self.run_bpd() as client:
+            response = client.send_command('crash_TypeError')
+        self._assert_failed(response, bpd.ERROR_SYSTEM)
+
 
 class BPDQueryTest(BPDTestHelper):
     test_implements_query = implements({