New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MPD session crashes on too few/many arguments to MPD commands #789

Closed
facunditito opened this Issue Jul 25, 2014 · 11 comments

Comments

5 participants
@facunditito

facunditito commented Jul 25, 2014

Hello, I start this issue because I haven't find any similar. When I start mopidy, I get:

** Message: pygobject_register_sinkfunc is deprecated (GstObject)
INFO     Starting Mopidy 0.19.1
INFO     Loading config from: builtin defaults, /etc/xdg/mopidy/mopidy.conf, /home/facundo/.config/mopidy/mopidy.conf, command line options
INFO     Enabled extensions: mpd, softwaremixer
INFO     Disabled extensions: local, http, stream
INFO     Starting Mopidy mixer: SoftwareMixer
INFO     Mixing using GStreamer software mixing
INFO     Starting Mopidy audio
INFO     Starting Mopidy backends: none
INFO     Starting Mopidy core
INFO     Starting Mopidy frontends: MpdFrontend
INFO     MPD server running at [::ffff:127.0.0.1]:6600
INFO     Audio output set to "autoaudiosink"
INFO     New MPD connection from [::ffff:127.0.0.1]:59691
ERROR    Unhandled exception in MpdSession (urn:uuid:c6862920-30cf-40df-a5a2-701fe40e8795):
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/pykka/actor.py", line 200, in _actor_loop
    response = self._handle_receive(message)
  File "/usr/lib/python2.7/site-packages/pykka/actor.py", line 303, in _handle_receive
    return self.on_receive(message)
  File "/usr/lib/python2.7/site-packages/mopidy/utils/network.py", line 366, in on_receive
    self.on_line_received(line)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/session.py", line 33, in on_line_received
    response = self.dispatcher.handle_request(line)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 46, in handle_request
    return self._call_next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 67, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 75, in _catch_mpd_ack_errors_filter
    return self._call_next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 67, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 88, in _authenticate_filter
    return self._call_next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 67, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 104, in _command_list_filter
    response = self._call_next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 67, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 133, in _idle_filter
    response = self._call_next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 67, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 146, in _add_ok_filter
    response = self._call_next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 67, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 158, in _call_handler_filter
    response = self._format_response(self._call_handler(request))
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/dispatcher.py", line 167, in _call_handler
    return protocol.commands.call(tokens, context=self.context)
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/protocol/__init__.py", line 171, in call
    return self.handlers[tokens[0]](context, *tokens[1:])
  File "/usr/lib/python2.7/site-packages/mopidy/mpd/protocol/__init__.py", line 141, in validate
    callargs = inspect.getcallargs(func, *args, **kwargs)
  File "/usr/lib/python2.7/inspect.py", line 981, in getcallargs
    'arguments' if num_required > 1 else 'argument', num_total))
TypeError: password() takes exactly 2 arguments (1 given)
^CINFO     Interrupted. Exiting...

PC became really slow (Its a Atom CPU). As you can see, I disabled almost all the extension and still get the error. The problem only dissappear when I disable the [MPD] extension. In my other computer with the same OS and same version of the dependences (but other CPU), I have not problem.

Output of mopidy deps

Platform: Linux-3.15.5-2-ARCH-i686-with-glibc2.0
Python: CPython 2.7.8 from /usr/lib/python2.7
Mopidy: 0.19.1 from /usr/lib/python2.7/site-packages
  setuptools: 5.4.1 from /usr/lib/python2.7/site-packages
  Pykka>=1.1: 1.2.0 from /usr/lib/python2.7/site-packages
  tornado>=2.3: 4.0 from /usr/lib/python2.7/site-packages
    certifi: 14.05.14 from /usr/lib/python2.7/site-packages
    backports.ssl-match-hostname: 3.4.0.2 from /usr/lib/python2.7/site-packages
GStreamer: 0.10.36.0 from /usr/lib/python2.7/site-packages/gst-0.10/gst
  Detailed information: 
    Python wrapper: gst-python 0.10.22.0
    Relevant elements:
      Found:
        uridecodebin
        souphttpsrc
        appsrc
        alsasink
        osssink
        oss4sink
        pulsesink
        id3demux
        id3v2mux
        lame
        mad
        vorbisdec
        vorbisenc
        vorbisparse
        oggdemux
        oggmux
        oggparse
        flacdec
        flacparse
        shout2send
      Not found:
        flump3dec
        mp3parse

Output of mopidy config

** Message: pygobject_register_sinkfunc is deprecated (GstObject)
[logging]
color = true
console_format = %(levelname)-8s %(message)s
debug_format = %(levelname)-8s %(asctime)s [%(process)d:%(threadName)s] %(name)s\n  %(message)s
debug_file = mopidy.log
config_file =

[audio]
mixer = software
mixer_volume = 
output = autoaudiosink
visualizer = 

[proxy]
scheme = 
hostname = 
port = 
username = 
password = 

[local]
enabled = false  ; Extension disabled by user config.

[mpd]
enabled = true
hostname = 127.0.0.1
port = 6600
password = 
max_connections = 20
connection_timeout = 60
zeroconf = Mopidy MPD server on $hostname

[softwaremixer]
enabled = true

[http]
enabled = false  ; Extension disabled by user config.

[stream]
enabled = false  ; Extension disabled by user config.

EDIT: With prevoius version (v18), It worked well

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Jul 25, 2014

Try removing "password =" from your [mpd] section?
Why do you have a lot of sections with empty configuration?

@jodal jodal added bug labels Jul 25, 2014

@jodal jodal added this to the v0.19.2 milestone Jul 25, 2014

@jodal

This comment has been minimized.

Member

jodal commented Jul 25, 2014

I can reproduce this with default MPD config by sending the MPD command password without any arguments.

@jodal

This comment has been minimized.

Member

jodal commented Jul 25, 2014

And the same applies for any other command where the wrong number of arguments are supplied, e.g. consume.

@jodal

This comment has been minimized.

Member

jodal commented Jul 25, 2014

This patch fixes the issue, but I'm not sure this is the best place to fix it, @adamcik?

I checked, and all TypeError exceptions raised by inspect.getcallargs() are related to number of arguments.

This will also catch the TypeError when commands receive too many arguments. I noticed that the original MPD server has a slightly different error message for too many arguments to some commands, like play 1 2 3. This patch does not address this. The error code is the same, but the error message differs on the level of "wrong number of arguments" vs. "too many arguments".

diff --git a/mopidy/mpd/protocol/__init__.py b/mopidy/mpd/protocol/__init__.py
index 3c501bb..59a547f 100644
--- a/mopidy/mpd/protocol/__init__.py
+++ b/mopidy/mpd/protocol/__init__.py
@@ -138,7 +138,11 @@ class Commands(object):
             def validate(*args, **kwargs):
                 if varargs:
                     return func(*args, **kwargs)
-                callargs = inspect.getcallargs(func, *args, **kwargs)
+                try:
+                    callargs = inspect.getcallargs(func, *args, **kwargs)
+                except TypeError:
+                    raise exceptions.MpdArgError(
+                        'wrong number of arguments for "%s"' % name)
                 for key, value in callargs.items():
                     default = defaults.get(key, object())
                     if key in validators and value != default:
diff --git a/tests/mpd/protocol/test_authentication.py b/tests/mpd/protocol/test_authentication.py
index 6a39ba8..4937c04 100644
--- a/tests/mpd/protocol/test_authentication.py
+++ b/tests/mpd/protocol/test_authentication.py
@@ -19,6 +19,12 @@ class AuthenticationActiveTest(protocol.BaseTestCase):
         self.assertFalse(self.dispatcher.authenticated)
         self.assertEqualResponse('ACK [3@0] {password} incorrect password')

+    def test_authentication_without_password_fails(self):
+        self.sendRequest('password')
+        self.assertFalse(self.dispatcher.authenticated)
+        self.assertEqualResponse(
+            'ACK [2@0] {password} wrong number of arguments for "password"')
+
     def test_anything_when_not_authenticated_should_fail(self):
         self.sendRequest('any request at all')
         self.assertFalse(self.dispatcher.authenticated)

@jodal jodal changed the title from Error when mopidy start to MPD session crashes on too few/many arguments to MPD commands Jul 25, 2014

@facunditito

This comment has been minimized.

facunditito commented Jul 25, 2014

Just more information: this is my mopidy.conf, all the rest is the default configuration:

[mpd]
hostname = ::

[spotify]
enabled = false
username = alice
password = mysecret

[local]
enabled = false

[http]
enabled = false

[stream]
enabled = false

/etc/xdg/mopidy/mopidy.conf doesn't exist. And if I configure MPD with a password, the same bug happen

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Jul 25, 2014

@facunditito Could you please get the log from that last test again, and run mopidy as "mopidy -vv"?
Are you running mopidy as a service or as a user?

@jodal

This comment has been minimized.

Member

jodal commented Jul 25, 2014

@ZenithDK If you see my comments from earlier today, this is very reproducible :-)

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Jul 25, 2014

@jodal Okay - but in the last config he posted there are no empty options, I thought you reproducing it was with empty parameters?

@jodal

This comment has been minimized.

Member

jodal commented Jul 25, 2014

It can be reproduced with any config, AFAIK. It crashes in MPD request handling, before using the password config in any way.

@trygveaa

This comment has been minimized.

Member

trygveaa commented Jul 25, 2014

but in the last config he posted there are no empty options, I thought you reproducing it was with empty parameters?

The empty options is just printed by mopidy config if they are not set in the config. He never had those in his config.

@GraemeLion

This comment has been minimized.

GraemeLion commented Jul 26, 2014

Also experiencing this. And it really bogs it down. Running Arch, on an I5.

@jodal jodal closed this in 93bf3ea Jul 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment