segfault for passing a null string to strchr

[fajard01]

My mpd would not update the database and I tried different solutions found online but with no success. I later on discovered that the program crashes during update issuing a segmentation fault. I rebuilt mpd with debugging symbols on and discovered mpd refuses to update due to a malformed flac metadata in one of my albums in my music directory. However it seems the bug lies on mpd passing a null string to strchr.

Here is the coredumpctl gdb output:

[Thread debugging using libthread_db enabled] Using host libthread_db library "/usr/lib/libthread_db.so.1". Core was generated by mpd --verbose --no-daemon /home/dipswitch/.config/mpd/mpd.conf'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007f1086cafc43 in __strchr_sse2 () from /usr/lib/libc.so.6
[Current thread is 1 (Thread 0x7f106d294700 (LWP 3933))]
(gdb) bt full
#0 0x00007f1086cafc43 in __strchr_sse2 () from /usr/lib/libc.so.6
No symbol table info available.
#1 0x0000562d91a443f6 in strchr (__c=, __s=0x0) at /usr/include/string.h:226
No locals.
#2 DivideString::DivideString (this=0x7f106d293490, s=0x0, separator=, strip=) at src/util/DivideString.cxx:28
length =
#3 0x0000562d91a054b3 in flac_scan_comment (entry=0x7f106007fa00, handler=..., handler_ctx=handler_ctx@entry=0x7f106d2936b0) at src/decoder/plugins/FlacMetadata.cxx:99
comment =
split = {first = 0x0, second = 0x7f10600632b6 "Lotus Feet"}
#4 0x0000562d91a05778 in flac_scan_comments (comment=, comment=, handler_ctx=, handler=...)
at src/decoder/plugins/FlacMetadata.cxx:123
i = 2
#5 flac_scan_metadata (block=0x7f1060062aa0, handler=..., handler_ctx=0x7f106d2936b0) at src/decoder/plugins/FlacMetadata.cxx:143
No locals.
#6 0x0000562d91a058ee in FlacMetadataChain::Scan (this=0x7f106d293540, handler=..., handler_ctx=0x7f106d2936b0) at src/decoder/plugins/FlacMetadata.cxx:176
iterator = {iterator = 0x7f106001cf70}
#7 0x0000562d91a0011f in flac_scan_file (path_fs=..., handler=..., handler_ctx=0x7f106d2936b0) at src/decoder/plugins/FlacDecoderPlugin.cxx:83
chain = {chain = 0x7f1060061a50}
#8 0x0000562d919b3c62 in DecoderPlugin::ScanFile (this=, handler_ctx=, handler=..., path_fs=...) at src/decoder/DecoderPlugin.hxx:145
No locals.
#9 TagFileScan::ScanFile (plugin=..., this=0x7f106d2935c0) at src/TagFile.cxx:55
No locals.
#10 TagFileScan::Scan (plugin=..., this=0x7f106d2935c0) at src/TagFile.cxx:83
this = 0x7f106d2935c0
#11 <lambda(const DecoderPlugin&)>::operator() (plugin=..., __closure=) at src/TagFile.cxx:103
No locals.
#12 decoder_plugins_try<tag_file_scan(Path, const TagHandler&, void*)::<lambda(const DecoderPlugin&)> > (f=...) at src/decoder/DecoderList.hxx:60
i = 4
#13 tag_file_scan (path_fs=..., handler=..., handler_ctx=handler_ctx@entry=0x7f106d2936b0) at src/TagFile.cxx:103
PRETTY_FUNCTION = "bool tag_file_scan(Path, const TagHandler&, void*)"
suffix =
suffix_utf8 = "flac"
tfs = {path_fs = {<StringPointer> = {static SENTINEL = ,
value = 0x7f1060061f40 "/home/DataPool/Music/Shakti/Shakti with John McLaughlin/Disc 1 - 02 - Lotus Feet.flac"}, },
suffix = 0x7f106d2935b0 "flac", handler = @0x562d91c9dc00, handler_ctx = 0x7f106d2936b0, mutex = { = {mutex = {__data = {__lock = 0, __count = 0,
__owner = 0, __nusers = 0, __kind = 0, __spins = 0, __elision = 0, __list = {__prev = 0x0, __next = 0x0}}, __size = '\000' <repeats 39 times>,
__align = 0}}, }, cond = { = {cond = {__data = {{__wseq = 0, __wseq32 = {__low = 0, __high = 0}}, {__g1_start = 0, __g1_start32 = {
__low = 0, __high = 0}}, __g_refs = {0, 0}, __g_size = {0, 0}, __g1_orig_size = 0, __wrefs = 0, __g_signals = {0, 0}},
__size = '\000' <repeats 47 times>, __align = 0}}, }, is = std::unique_ptr containing 0x0}
#14 0x0000562d919b3e49 in tag_file_scan (path=..., builder=...) at src/TagFile.cxx:109
No locals.
#15 0x0000562d919b21d9 in Song::UpdateFile (this=this@entry=0x7f1060062090, storage=...) at src/SongUpdate.cxx:87
relative_uri = "Shakti/Shakti with John McLaughlin/Disc 1 - 02 - Lotus Feet.flac"
info = {type = StorageFileInfo::Type::REGULAR, size = 19263536, mtime = 1503678958, device = 47, inode = 147817}
---Type to continue, or q to quit---
tag_builder = {duration = {<std::chrono::duration<int, std::ratio<1, 1000> >> = {__r = 286466}, }, has_playlist = false,
items = std::vector of length 1, capacity 1 = {0x7f1060062109}}
path_fs = {value = "/home/DataPool/Music/Shakti/Shakti with John McLaughlin/Disc 1 - 02 - Lotus Feet.flac"}
#16 0x0000562d919b2307 in Song::LoadFile (storage=..., path_utf8=path_utf8@entry=0x7f1060061d20 "Disc 1 - 02 - Lotus Feet.flac", parent=...) at src/SongUpdate.cxx:51
PRETTY_FUNCTION = "static Song* Song::LoadFile(Storage&, const char*, Directory&)"
song = 0x7f1060062090
#17 0x0000562d919bdff3 in UpdateWalk::UpdateSongFile2 (info=..., suffix=0x7f1060061d39 "flac", name=0x7f1060061d20 "Disc 1 - 02 - Lotus Feet.flac", directory=...,
this=0x562d9342ba50) at src/db/update/UpdateSong.cxx:66
song =
#18 UpdateWalk::UpdateSongFile (this=this@entry=0x562d9342ba50, directory=..., name=name@entry=0x7f1060061d20 "Disc 1 - 02 - Lotus Feet.flac",
suffix=suffix@entry=0x7f1060061d39 "flac", info=...) at src/db/update/UpdateSong.cxx:104
No locals.
#19 0x0000562d919bd5a2 in UpdateWalk::UpdateRegularFile (info=..., name=0x7f1060061d20 "Disc 1 - 02 - Lotus Feet.flac", directory=..., this=0x562d9342ba50)
at src/db/update/Walk.cxx:211
suffix = 0x7f1060061d39 "flac"
#20 UpdateWalk::UpdateDirectoryChild (this=this@entry=0x562d9342ba50, directory=..., exclude_list=..., name=name@entry=0x7f1060061d20 "Disc 1 - 02 - Lotus Feet.flac",
info=...) at src/db/update/Walk.cxx:224
PRETTY_FUNCTION = "void UpdateWalk::UpdateDirectoryChild(Directory&, const ExcludeList&, const char*, const StorageFileInfo&)"
#21 0x0000562d919bd0bc in UpdateWalk::UpdateDirectory (this=this@entry=0x562d9342ba50, directory=..., exclude_list=..., info=...) at src/db/update/Walk.cxx:382
info2 = {type = StorageFileInfo::Type::REGULAR, size = 19263536, mtime = 1503678958, device = 47, inode = 147817}
PRETTY_FUNCTION = "bool UpdateWalk::UpdateDirectory(Directory&, const ExcludeList&, const StorageFileInfo&)"
reader = std::unique_ptr containing 0x7f1060061110
child_exclude_list = {parent = 0x7f106d293aa0, patterns = empty std::forward_list}
name_utf8 = 0x7f1060061d20 "Disc 1 - 02 - Lotus Feet.flac"
#22 0x0000562d919bd557 in UpdateWalk::UpdateDirectoryChild (this=this@entry=0x562d9342ba50, directory=..., exclude_list=...,
name=name@entry=0x7f1060062150 "Shakti with John McLaughlin", info=...) at src/db/update/Walk.cxx:238
PRETTY_FUNCTION = "void UpdateWalk::UpdateDirectoryChild(Directory&, const ExcludeList&, const char*, const StorageFileInfo&)"
#23 0x0000562d919bd0bc in UpdateWalk::UpdateDirectory (this=this@entry=0x562d9342ba50, directory=..., exclude_list=..., info=...) at src/db/update/Walk.cxx:382
info2 = {type = StorageFileInfo::Type::DIRECTORY, size = 5, mtime = 1503679271, device = 47, inode = 148298}
PRETTY_FUNCTION = "bool UpdateWalk::UpdateDirectory(Directory&, const ExcludeList&, const StorageFileInfo&)"
reader = std::unique_ptr containing 0x7f10600631f0
child_exclude_list = {parent = 0x7f106d293c40, patterns = empty std::forward_list}
name_utf8 = 0x7f1060062150 "Shakti with John McLaughlin"
#24 0x0000562d919bd557 in UpdateWalk::UpdateDirectoryChild (this=this@entry=0x562d9342ba50, directory=..., exclude_list=..., name=name@entry=0x7f106001bb00 "Shakti",
info=...) at src/db/update/Walk.cxx:238
PRETTY_FUNCTION = "void UpdateWalk::UpdateDirectoryChild(Directory&, const ExcludeList&, const char*, const StorageFileInfo&)"
#25 0x0000562d919bd0bc in UpdateWalk::UpdateDirectory (this=this@entry=0x562d9342ba50, directory=..., exclude_list=..., info=...) at src/db/update/Walk.cxx:382
info2 = {type = StorageFileInfo::Type::DIRECTORY, size = 4, mtime = 1503679315, device = 47, inode = 147566}
PRETTY_FUNCTION = "bool UpdateWalk::UpdateDirectory(Directory&, const ExcludeList&, const StorageFileInfo&)"
reader = std::unique_ptr containing 0x7f10600008e0
child_exclude_list = {parent = 0x7f106d293d30, patterns = empty std::forward_list}
name_utf8 = 0x7f106001bb00 "Shakti"
#26 0x0000562d919bda50 in UpdateWalk::Walk (this=0x562d9342ba50, root=..., path=, discard=) at src/db/update/Walk.cxx:499
info = {type = StorageFileInfo::Type::DIRECTORY, size = 109, mtime = 1504280730, device = 47, inode = 69636}
---Type to continue, or q to quit---
exclude_list = {parent = 0x0, patterns = empty std::forward_list}
#27 0x0000562d919bb5f8 in UpdateService::Task (this=0x562d933ad670) at src/db/update/Service.cxx:123
No locals.
#28 UpdateService::Task (ctx=0x562d933ad670) at src/db/update/Service.cxx:147
service = @0x562d933ad670: { = {_vptr.DeferredMonitor = 0x562d91c9af48 <vtable for UpdateService+16>, loop = @0x562d933acc88, pending = false}, db =
@0x562d933ad1b0, storage = @0x562d933ad2d0, listener = @0x562d933acc80, modified = false, update_thread = {handle = 139708527625984, defined = true, creating = false,
f = 0x562d919bb590 UpdateService::Task(void*), ctx = 0x562d933ad670}, static update_task_id_max = 32768, update_task_id = 1, queue = {
static MAX_UPDATE_QUEUE_SIZE = 32, Python Exception <class 'ValueError'> Cannot find type std::__cxx11::list<UpdateQueueItem, std::allocator >::_Node:
update_queue = empty std::__cxx11::list}, next = {db = 0x562d933ad1b0, storage = 0x562d933ae2c0, path_utf8 = "", id = 1,
discard = true}, walk = 0x562d9342ba50}
#29 0x0000562d91a3c012 in Thread::ThreadProc (ctx=) at src/thread/Thread.cxx:105
thread =
#30 0x00007f1086fdc049 in start_thread () from /usr/lib/libpthread.so.0
No symbol table info available.
#31 0x00007f1086d1cf0f in clone () from /usr/lib/libc.so.6
No symbol table info available.`

This is on an Arch Linux machine with mpd version 0.20.10. A full discussion of the problem can be found in this thread:
https://bbs.archlinux.org/viewtopic.php?id=229413

[MaxKellermann]

This looks like an impossible situation, according to libFLAC API documentation. Maybe it is a bug in libFLAC due to a corrupted FLAC file? Which libFLAC version are you using? Please send that failing FLAC file (Disc 1 - 02 - Lotus Feet.flac) to me (max.kellermann@gmail.com).

[fajard01]

I have flac 1.3.2.
I'll send a link to the files.

[MaxKellermann]

Your link leads me to HTML+JavaScript hell, and I cannot download anything.

[fajard01]

I'll try again.

[MaxKellermann]

Your second mail contains the same JavaScript hell. I cannot wget this link.
Let's reopen this bug report as soon as you are able to send a proper wgettable download link. (or just the raw file in an email - is that idea too simple for you?)

[fajard01]

Sorry for that. I'll attach it to an email.

[louiz]

Hello, I encountered this exact same issue with a FLAC album ripped with sound-juicer 3.24.0

I have the following installed flac libs:

flac-libs-1.3.2-2.fc26.x86_64
flac-devel-1.3.2-2.fc26.x86_64

Reproduced on master branch of MPD, with the following config:

Archive support:
        (+bzip2) (+ISO9660) (-ZIP) 
Autodiscovery support:
        (+Avahi) (-Bonjour) 
Client support:
        (+IPv6) (+TCP) (+UNIX Domain Sockets) 
Storage support:
        (-NFS) (+SMB) 
File format support:
        (-AAC) (-AdPlug) (+DSD) (-C64 SID) (-FFMPEG) (+FLAC) (-FluidSynth) (-GME) 
        (+libsndfile) (-MikMod) (+MODPLUG) (+MAD) (-MPG123) (-Musepack) 
        (+Opus) (-OggTremor) (+OggVorbis) (+WAVE) (+WavPack) (-WildMidi) 
Other features:
        (+libsamplerate) (+libsoxr) (+libmpdclient) (+inotify) (+SQLite) 
Metadata support:
        (+ID3) 
Playback support:
        (+ALSA) (+FIFO) (-SNDIO) (+File Recorder) (-Haiku) (+HTTP Daemon) (+JACK) 
        (+libao) (+OSS) (+OpenAL) (-OS X) (-Pipeline) 
        (+PulseAudio) (-ROAR) (+SHOUTcast) (-Solaris) (-WinMM) 
Streaming encoder support:
        (+FLAC) (+LAME) (-Shine) (+Ogg Vorbis) (+Opus) (-TwoLAME) (+WAVE) 
Streaming support:
        (+CDIO_PARANOIA) (+CURL) (+SMBCLIENT) (+Soundcloud) 
        (+MMS) 
Event loop:
        epoll

And indeed, in FlacMetadata.cxx line 99, const char* comment is 0x0.

So, I added a simple if (!comment) return;, and then it also segfaults when playing that file, in VorbisComment.cxx line 30. So I replaced this assert with a similar if (!entry) return nullptr;. And this now seems to work.

Also, note: with mpv, when playing that file, I get this output:

[ffmpeg/audio] flac: invalid sync code    
[ffmpeg/audio] flac: invalid frame header 
[ffmpeg/audio] flac: decode_frame() failed
Error decoding audio.                     

However the file successfully plays.

I’ll send you (@MaxKellermann) an e-mail with a link to wget that flac song, to help you reproduce.

[MaxKellermann]

@louz thanks for the file. With it, I could confirm that this is actually a libFLAC bug. Please report it to Xiph/libFLAC.

[louiz]

I’m not sure how libflac is supposed to work. Is that just a bug in the decoding of the file? Or a bug when the file was actually created (and made it corrupt)?

I guess it’s both: it should not create a corrupt file (somehow…), but it also shouldn’t return a nullptr when the API documentation says it can’t (even if the file is corrupt).

Should I just report “here, with this file, some_function_call() returns nullptr while it should not. See that FLAC file that I have”?

[MaxKellermann]

This may be a corrupt file. But no matter how corrupt a file is, the application/library must not crash. And libFLAC doesn't behave correctly: instead of failing to parse the file, it returns malformed data to the application (=MPD).
Adding your suggested NULL check to MPD would be wrong, because that doesn't fix the bug - it only makes the symptom go away by hiding it, and I'm not fond of that kind of approach. This must be fixed in libFLAC.

[louiz]

Yeah, I agree, my fix is just a workaround for myself (to be able to listen to music…), I know it’s not the correct fix.

My question was more about what my bug report was supposed to say exactly. I’ll try anyway. Thank you.

[MaxKellermann]

libFLAC puts NULL values into FLAC__StreamMetadata_VorbisComment_Entry, which according to the API documentation (https://xiph.org/flac/api/structFLAC____StreamMetadata__VorbisComment__Entry.html) must never happen.

[MaxKellermann]

(gdb) p vc
$40 = (const FLAC__StreamMetadata_VorbisComment &) @0x7fffffffe1f0: {vendor_string = {length = 31, entry = 0x555555686030 "GStreamer encoded vorbiscomment"},
num_comments = 12, comments = 0x555555688d00}
(gdb) p *vc.comments@12
$41 = {{length = 27, entry = 0x55555567ae50 "TITLE=March of the Machines"}, {length = 13, entry = 0x55555567a980 "ARTIST=Ayreon"}, {length = 14,
entry = 0x5555556876c0 "TRACKNUMBER=10"}, {length = 13, entry = 0x55555567a0c0 "TRACKTOTAL=10"}, {length = 16, entry = 0x555555688df0 "ALBUM=The Source"}, {
length = 46, entry = 0x555555686970 "COMPOSER=Micheal Mills, Arjen Anthony Lucassen"}, {length = 18, entry = 0x5555556869b0 "ALBUMARTIST=Ayreon"}, {
length = 0, entry = 0x0}, {length = 0, entry = 0x0}, {length = 0, entry = 0x0}, {length = 0, entry = 0x0}, {length = 18, entry = 0x5555556869d0 ""}}

[MaxKellermann]

The last one contains an awful lot of null bytes:

(gdb) p *vc.comments[11].entry@19
$45 = "\000\000\000\006®\000\000\000\000\000\000\rxS\022\000\000\000"