Syncplay / syncplay

Client/server to synchronize media playback on mpv/VLC/MPC-HC/MPC-BE on many computers
http://syncplay.pl/
Apache License 2.0
2.15k stars 215 forks source link

UnicodeDecodeError #210

Closed BotoX closed 5 years ago

BotoX commented 5 years ago

My syncplay server crashed while I was not using it, this was in the error log:

builtins.UnicodeDecodeError: 'utf-8' codec can't decode byte 0x89 in position 2: invalid start byte
    line = line.decode('utf-8').strip()
  File "/usr/lib/syncplay/syncplay/protocols.py", line 34, in lineReceived
    why = self.lineReceived(line)
  File "/usr/lib/python3.7/site-packages/twisted/protocols/basic.py", line 572, in dataReceived
    rval = self.protocol.dataReceived(data)
  File "/usr/lib/python3.7/site-packages/twisted/internet/tcp.py", line 249, in _dataReceived
    return self._dataReceived(data)
  File "/usr/lib/python3.7/site-packages/twisted/internet/tcp.py", line 243, in doRead
    why = selectable.doRead()
  File "/usr/lib/python3.7/site-packages/twisted/internet/posixbase.py", line 614, in _doReadOrWrite
--- <exception caught here> ---
    return func(*args,**kw)
  File "/usr/lib/python3.7/site-packages/twisted/python/context.py", line 85, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/usr/lib/python3.7/site-packages/twisted/python/context.py", line 122, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File "/usr/lib/python3.7/site-packages/twisted/python/log.py", line 86, in callWithContext
    return callWithContext({"system": lp}, func, *args, **kw)
  File "/usr/lib/python3.7/site-packages/twisted/python/log.py", line 103, in callWithLogger
Traceback (most recent call last):
Unhandled Error

I guess just move the decode into the try block to fix it? https://github.com/Syncplay/syncplay/blob/master/syncplay/protocols.py#L34

Et0h commented 5 years ago

That sounds like a reasonable guess to me, but I've not tested it with a relevant use case. If it doesn't work then a potential alternative might be "line = line.decode('utf-8', 'ignore').strip()" but that would also need testing with a relevant use case.

xNinjaKittyx commented 5 years ago

Just putting my thoughts here that using 'ignore' would end up completely remove those bytes, which I agree that there should be a relevant scenario that would cause this issue. (It says position 2, which should just be one of the keys of the protocol no?)

Also, the code has

if not line:
    return

after the decode instead of before... I wonder if there was a specific reason for this?

line = line.decode('utf-8', 'backslashreplace').strip() might also be another solution if we want to preserve the bytes. (however it's going to show up as double backslash \\x80, so it wouldn't work the other way normally)

BotoX commented 5 years ago

Could it be that this is just someone (or something, a bot) sending garbage data to the syncplay server which it then tries to interpret and fails? Because my server crashed again and I don't think anyone even tried to connect to it, it also never happens when somebody tries to connect or is in it. I've moved the decode into the try block on my server and will report back if there are any more errors.

xNinjaKittyx commented 5 years ago

If that's the case, syncplay has to handle that situation so it doesn't crash, but just closes the connection with that client.

BotoX commented 5 years ago

yep, my assumption was true, check out all the garbage my syncplay has received in the past few days: https://p.botox.bz/view/raw/122f983b So simply moving everything into the try block does the trick.