MPD crashes on windows when large input is submitted

[Polochon-street]

Bug report

Describe the bug

When sending 1. large input and 2. that would trigger the logging of an error on MPD on windows, MPD just segfaults. For instance, sending listplaylist <400 times 'A'> swiftly makes MPD segfaults.

The error probably stems from https://github.com/MusicPlayerDaemon/MPD/blob/master/src/system/Error.hxx#L94-L95 , and the fact that snprintf returns the number of bytes which would have been written to the final string if enough space had been available (https://linux.die.net/man/3/snprintf), and not the number of bytes actually written.
I think changing it by a min(return_value, 512) would work and I'd gladly submit a PR if that's the way to go :)

It also means that it's possible to write 0x3a20 (which corresponds to the : manually written) about anywhere on the stack, which leads to crashes when it's written to, for instance, rip, but could probably be used for more malicious purposes.

Expected Behavior

MPD does not crash when sent arbitrary big input.

Actual Behavior

MPD crashes when sent big payloads.

Version

PS C:\Users\Paul\Downloads> .\mpd.exe --version
Music Player Daemon 0.23.10 (0.23.10)
Copyright 2003-2007 Warren Dukes <warren.dukes@gmail.com>
Copyright 2008-2021 Max Kellermann <max.kellermann@gmail.com>
This is free software; see the source for copying conditions.  There is NO
warranty; not even MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Database plugins:
 simple proxy

Storage plugins:
 local nfs curl


Decoders plugins:
 [vorbis] ogg oga
 [oggflac] ogg oga
 [flac] flac
 [opus] opus ogg oga
 [dsdiff] dff
 [dsf] dsf
 [hybrid_dsd] m4a
 [openmpt] mptm mod s3m xm it 669 amf ams c67 dbm digi dmf dsm dtm far imf ice j2b m15 mdl med mms mt2 mtm nst okt plm psm pt36 ptm sfx sfx2 st26 stk stm stp ult wow gdm mo3 oxm umx xpk ppm mmcmp
 [modplug] 669 amf ams dbm dfm dsm far it med mdl mod mtm mt2 okt s3m stm ult umx xm
 [wildmidi] mid
 [ffmpeg] 16sv 3g2 3gp 4xm 8svx aa3 aac ac3 adx afc aif aifc aiff al alaw amr anim apc ape asf atrac au aud avi avm2 avs bap bfi c93 cak cin cmv cpk daud dct divx dts dv dvd dxa eac3 film flac flc fli fll flx flv g726 gsm gxf iss m1v m2v m2t m2ts m4a m4b m4v mad mj2 mjpeg mjpg mka mkv mlp mm mmf mov mp+ mp1 mp2 mp3 mp4 mpc mpeg mpg mpga mpp mpu mve mvi mxf nc nsv nut nuv oga ogm ogv ogx oma ogg omg opus psp pva qcp qt r3d ra ram rl2 rm rmvb roq rpl rvc shn smk snd sol son spx str swf tak tgi tgq tgv thp ts tsp tta xa xvid uv uv2 vb vid vob voc vp6 vmd wav webm wma wmv wsaud wsvga wv wve
 [gme] ay gbs gym hes kss nsf nsfe rsn sap spc vgm vgz
 [pcm]

Filters:


Tag plugins:


Output plugins:
 null jack httpd snapcast recorder winmm wasapi

Encoder plugins:
 null vorbis opus lame wave flac

Input plugins:
 file curl ffmpeg nfs

Playlist plugins:
 extm3u m3u pls xspf asx rss flac cue embcue

Protocols:
 http:// https:// nfs://

Other features:
 ipv6 tcp
PS C:\Users\Paul\Downloads>

Configuration

music_directory		"C:/Users/Paul/Downloads/Music"
playlist_directory		"C:/Users/Paul/Downloads/playlists"
log_file			"C:/Users/Paul/Downloads/log"

audio_output {
	type		"null"
	name		"My Null Output"
}

database {
plugin "simple"
path "C:/Users/Paul/Downloads/database"
}

Log

The backtrace is misleading, since we're dealing with stack frame corruptions here, but added a few excerpts, depending on the length of the payload:

PS C:\Users\Paul\Downloads> gdb --args mpd --stderr --no-daemon --verbose mpd.conf
GNU gdb (GDB) (Cygwin 11.2-1) 11.2
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-pc-cygwin".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from mpd...
(gdb) run
Starting program: /cygdrive/c/Users/Paul/Downloads/mpd --stderr --no-daemon --verbose mpd.conf
[New Thread 17592.0x512c]
[New Thread 17592.0x4248]
[New Thread 17592.0x2140]
warning: Invalid parameter passed to C runtime function.
config_file: loading file mpd.conf
warning: Invalid parameter passed to C runtime function.
vorbis: Xiph.Org libVorbis 1.3.7
opus: libopus 1.3.1
hybrid_dsd: The Hybrid DSD decoder is disabled because it was not explicitly enabled
decoder: Decoder plugin 'wildmidi' is unavailable: configuration file does not exist: /etc/timidity/timidity.cfg
simple_db: reading DB
curl: version 7.85.0
curl: with Schannel
input: Input plugin 'ffmpeg' is unavailable: No protocol
[New Thread 17592.0x28a4]
[New Thread 17592.0x1834]
client: [0] opened from 192.168.0.60:15032
client: [0] process command "listplaylist AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"

Thread 1 received signal SIGSEGV, Segmentation fault.
0x0000203a560f6c54 in ?? ()
(gdb) bt
#0  0x0000203a560f6c54 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb)

or

(gdb) run mpd.conf
Starting program: /cygdrive/c/Users/Paul/Downloads/mpd mpd.conf
[New Thread 6764.0x4328]
[New Thread 6764.0xb08]
[New Thread 6764.0x204c]
warning: Invalid parameter passed to C runtime function.
warning: Invalid parameter passed to C runtime function.
Nov 27 23:28 : decoder: Decoder plugin 'wildmidi' is unavailable: configuration file does not exist: /etc/timidity/timidity.cfg
[New Thread 6764.0x448c]
[New Thread 6764.0x18fc]

Thread 1 received signal SIGSEGV, Segmentation fault.
0x00007ff77f0d4166 in GetFullMessage[abi:cxx11](std::exception const&, char const*, char const*) (e=..., fallback=fallback@entry=0x7ff77f3d9be3 "Unknown exception", separator=separator@entry=0x7ff77f3d9be0 "; ") at ../../src/util/Exception.cxx:60
60      ../../src/util/Exception.cxx: No such file or directory.
(gdb) bt
#0  0x00007ff77f0d4166 in GetFullMessage[abi:cxx11](std::exception const&, char const*, char const*) (e=...,
    fallback=fallback@entry=0x7ff77f3d9be3 "Unknown exception", separator=separator@entry=0x7ff77f3d9be0 "; ")
    at ../../src/util/Exception.cxx:60
#1  0x00007ff77f0d4102 in GetFullMessage[abi:cxx11](std::__exception_ptr::exception_ptr, char const*, char const*) (
    ep=..., fallback=fallback@entry=0x7ff77f3d9be3 "Unknown exception", separator=separator@entry=0x7ff77f3d9be0 "; ")
    at ../../src/util/Exception.cxx:72
#2  0x00007ff77f0ef783 in Log (level=level@entry=LogLevel::ERROR, ep=...) at ../../src/Log.cxx:44
#3  0x00007ff77f0e33e8 in LogError (ep=...) at ../../src/Log.hxx:141
#4  playlist_open_path_suffix (mutex=..., path=...) at ../../src/playlist/PlaylistStream.cxx:50
#5  playlist_open_path (path=..., mutex=...) at ../../src/playlist/PlaylistStream.cxx:62
#6  0x00007ff77f0e698a in playlist_open_in_playlist_dir (mutex=..., uri=0x29a3fe864f5 'A' <repeats 200 times>...)
    at ../../src/util/StringPointer.hxx:54
#7  playlist_mapper_open (uri=0x29a3fe864f5 'A' <repeats 200 times>..., storage=0x29a416f3dc0, mutex=...)
    at ../../src/playlist/PlaylistMapper.cxx:79
#8  0x00007ff77f0dfba9 in playlist_open_any (located_uri=..., storage=<optimized out>, mutex=...)
    at ../../src/playlist/PlaylistAny.cxx:46
#9  0x00007ff77f0e55c9 in playlist_file_print (r=..., partition=..., loader=..., uri=..., detail=detail@entry=false)
    at ../../src/playlist/Print.cxx:71
#10 0x00007ff77f0e4f02 in handle_listplaylist (client=..., args=..., r=...) at ../../src/client/Client.hxx:255
#11 0x00007ff77f0d8e33 in command_process (client=..., num=num@entry=0, line=line@entry=0x29a3fe864e8 "listplaylist")
    at ../../src/command/AllCommands.cxx:432
#12 0x00007ff77f18864d in Client::ProcessLine (this=this@entry=0x29a3fe864d0, line=<optimized out>,
    line@entry=0x29a3fe864e8 "listplaylist") at ../../src/client/Process.cxx:137
#13 0x00007ff77f188e53 in Client::OnSocketInput (length=<optimized out>, data=0x29a3fe864e8, this=0x29a3fe864d0)
    at ../../src/client/Read.cxx:49
#14 Client::OnSocketInput (this=0x29a3fe864d0, data=0x29a3fe864e8, length=<optimized out>)
    at ../../src/client/Read.cxx:29
#15 0x00007ff77f129b95 in BufferedSocket::ResumeInput (this=this@entry=0x29a3fe864d0)
    at ../../src/event/BufferedSocket.cxx:76
#16 0x00007ff77f129dc9 in BufferedSocket::OnSocketReady (this=0x29a3fe864d0, flags=<optimized out>)
    at ../../src/event/BufferedSocket.cxx:113
#17 0x00007ff77f19c552 in EventLoop::Run (this=this@entry=0xe4983fd378) at ../../src/event/Loop.cxx:362
#18 0x00007ff77f0f8bf3 in MainConfigured (options=..., raw_config=...) at ../../src/Main.cxx:576
#19 0x00007ff77f0f4478 in MainOrThrow (argc=argc@entry=2, argv=argv@entry=0x29a3fe81850) at ../../src/Main.cxx:691
#20 0x00007ff77f0f1e19 in mpd_main (argc=argc@entry=2, argv=argv@entry=0x29a3fe81850) at ../../src/Main.cxx:697
#21 0x00007ff77f0cd60a in win32_main (argc=argc@entry=2, argv=argv@entry=0x29a3fe81850)
    at ../../src/win32/Win32Main.cxx:137
#22 0x00007ff77f38075e in main (argc=2, argv=0x29a3fe81850) at ../../src/Main.cxx:707
(gdb)

[MaxKellermann]

The error probably stems from https://github.com/MusicPlayerDaemon/MPD/blob/master/src/system/Error.hxx#L94-L95 , and the fact that snprintf returns the number of bytes which would have been written to the final string if enough space had been available (https://linux.die.net/man/3/snprintf), and not the number of bytes actually written.

Oh no, that's indeed a stupid mistake!

[Polochon-street]

it's definitely a footgun - now that I think about it, there are probably other places where that happens, I should look into it :)

[MaxKellermann]

There are few places where the return value is really used, and it looks like those are used correctly (to allocate a buffer).
MPD is being migrated away from those poor C APIs; the transition to libfmt is going on in the master branch, though this mistake was stil present over there.

[Polochon-street]

oh, alright. Awesome, and thanks for the quick fix!

[macmpi]

FWIW this also hits linux, particularly on AudioCD playback.
0.23.10 had very frequent IO crashes (0.23.9 was mostly fine): 0.23.11 apparently fixes it (not windows-only fix).
Thanks!

[macmpi]

@MaxKellermann
While it's better, but not fully solved yet: have some remaining IO struggles crashing mpd thread when reading Audio CD.
Thoughts?
What/how can I provide meaningful logs?

[MaxKellermann]

What/how can I provide meaningful logs?

https://mpd.readthedocs.io/en/stable/user.html#mpd-crashes

[macmpi]

Thanks for the tip.
While I'm trying to rebuild (on Alpine/musl) with debub symbols for use with gdb, I noticed a bunch of compile warnings for src/decoder/plugins/libdecoder_plugins.a.p/FfmpegIo.cxx.o in case this may be related:
https://pastebin.com/3Sz02H8b

[MaxKellermann]

I don't see any compiler warnings in your paste

[macmpi]

If those are no concerning messages, then fine.

For some reason, under gdb can't get pass:
Failed to decode cdda:///1; Unable find or access a CD-ROM drive with an audio CD in it.
https://pastebin.com/76qWTZ3d

While launched regularly it does access CD and then hangs. (IO stuff I'd like to debug)
in both cases mpd process seems launched as mpd user as expected, which is part of cdrom and audio groups... 👀

[macmpi]

I see there are many fmt buffer-related patches in master since 0.23.11, some related to cdio, so thought I would try with master.
However, build stumbles upon:

ninja: job failed: /usr/bin/sphinx-build -q -b html -d doc/doctrees /builds/macmpi/aports/community/mpd/src/MPD-8b1ff3f0055295b50f29a7af09809772484ef645/doc doc/html
WARNING: sphinx_rtd_theme (< 0.3.0) found. It will not be available since Sphinx-6.0
Theme error:
no theme named 'sphinx_rtd_theme' found (missing theme.conf?)
/builds/macmpi/aports/community/mpd/src/MPD-8b1ff3f0055295b50f29a7af09809772484ef645/doc/user.rst:720: ERROR: Unexpected indentation.
/builds/macmpi/aports/community/mpd/src/MPD-8b1ff3f0055295b50f29a7af09809772484ef645/doc/user.rst:723: ERROR: Unexpected indentation.
ninja: subcommand failed

Thoughts?

[macmpi]

Can't get much out of gdb: it's not a crash here, but IO that ramps-up up at 90% and process that seems stuck (D flag in htop).
gdb would not give me a promt to do any backtrace.
Same with current master (reverting 6913148 to build)