MPD crash (boost assertion failure)

[jacobvosmaer]

Bug report

Describe the bug

Crash while listening to music.

Expected Behavior

Not crashing

Actual Behavior

A crash

Version

Music Player Daemon 0.22~git (v0.21.16-650-g8e7ab9653)
Copyright 2003-2007 Warren Dukes <warren.dukes@gmail.com>
Copyright 2008-2018 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 curl


Decoders plugins:
 [vorbis] ogg oga
 [oggflac] ogg oga
 [flac] flac
 [opus] opus ogg oga
 [sndfile] wav aiff aif au snd paf iff svx sf voc w64 pvf xi htk caf sd2
 [audiofile] wav au aiff aif
 [dsdiff] dff
 [dsf] dsf
 [hybrid_dsd] m4a
 [wavpack] wv
 [modplug] 669 amf ams dbm dfm dsm far it med mdl mod mtm mt2 okt s3m stm ult umx xm
 [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
 [pcm]

Filters:
 soxr

Tag plugins:
 id3tag

Output plugins:
 null fifo pipe ao openal osx httpd recorder

Encoder plugins:
 null vorbis opus twolame wave flac

Archive plugins:
 [bz2] bz2

Input plugins:
 file archive curl ffmpeg

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

Protocols:
 file:// ftp:// ftps:// gopher:// http:// https:// mmsh:// mmst:// rtmp:// rtmps:// rtmpt:// rtmpts:// rtp:// smb:// srtp://

Other features:
 icu ipv6 tcp un

Log

Assertion failed: (( p == this->end() || !this->comp()(*p, value) )), function insert_unique_commit, file /opt/local/include/boost/intrusive/bstree.hpp, line 1331.

[jacobvosmaer]

It looks like an assertion somewhere deep inside Boost. I don't see bstree in the MPD source.

I only recently started using the input cache feature. I have no indication that is the cause though.

[jacobvosmaer]

Not sure if the Boost version matters, but here it is:

boost @1.71.0_1 (devel)
Sub-ports:            boost-numpy
Variants:             clang33, clang34, clang37, clang39, clang40, clang50, clang60,
                      clang70, clang80, clang90, cmake_scripts, debug, mpich, mpich_devel,
                      [+]no_single, [+]no_static, openmpi, openmpi_devel, python26,
                      [+]python27, python33, python34, python35, python36, python37,
                      regex_match_extra, universal

Description:          Boost provides free portable peer-reviewed C++ libraries. The
                      emphasis is on portable libraries which work well with the C++
                      Standard Library.
Homepage:             http://www.boost.org

Library Dependencies: zlib, expat, bzip2, libiconv, icu, python27
Platforms:            darwin
License:              Boost-1
Maintainers:          Email: ryandesign@macports.org, GitHub: ryandesign
                      Email: michaelld@macports.org, GitHub: michaelld
                      Policy: openmaintainer

[jacobvosmaer]

At this point I don't even know how to reproduce this. But this failed assertion suggests there is a bug somewhere, and now we have an issue to talk about it.

[MaxKellermann]

The bstree is used by the intrusive set class. This could be a concurrency problem, maybe an access lacking a mutex lock.

[jacobvosmaer]

Very wild guess:

MPD/src/input/cache/Item.cxx

Line 53 in b449627

// TODO: ensure that OnBufferAvailable() isn't currently running

(Meaning, I'm looking at the cache on a hunch, and this comment looks interesting. I don't know what is actually going on here.)

[jacobvosmaer]

Again, guessing, throwing random pieces of information together. The assertion talks about insert_unique_commit.

I'm not sure if I'm looking at the right documentation here:

https://www.boost.org/doc/libs/1_50_0/doc/html/boost/intrusive/rbtree.html#id1197749-bb

Anyway, I see:

No value should be inserted or erased between the "insert_check" and "insert_commit" calls.

And in the cache manager I see insert_check and insert_commit calls.

[jacobvosmaer]

And in between insert_check and insert_commit I see EvictOldestUnused which maybe removes values?

[jacobvosmaer]

So I previously posted the wrong documentation link (intrusive/bstree instead of intrusive/set) but I see the same requirement in intrusive/set:

https://www.boost.org/doc/libs/1_70_0/doc/html/boost/intrusive/set.html#idm45307364153120-bb

Notes: This function has only sense if a "insert_check" has been previously executed to fill "commit_data". No value should be inserted or erased between the "insert_check" and "insert_commit" calls.

[jacobvosmaer]

@MaxKellermann this all makes me suspect #691 (comment) is the problem.

[MaxKellermann]

I think you're right.

[jacobvosmaer]

Then this would be a fix?

diff --git a/src/input/cache/Manager.cxx b/src/input/cache/Manager.cxx
index 3ff51d88e..38e661064 100644
--- a/src/input/cache/Manager.cxx
+++ b/src/input/cache/Manager.cxx
@@ -112,7 +112,7 @@ InputCacheManager::Get(const char *uri, bool create)
        while (total_size > max_total_size && EvictOldestUnused()) {}
 
        auto *item = new InputCacheItem(std::move(is));
-       items_by_uri.insert_commit(*item, hint);
+       items_by_uri.insert(*item);
        items_by_time.push_back(*item);
 
        return InputCacheLease(*item);

[jacobvosmaer]

Or, move the while eviction loop after the insert_commit.