Bug 93343 - SIGSEGV in libakode when reading mmapped files
Summary: SIGSEGV in libakode when reading mmapped files
Status: RESOLVED FIXED
Alias: None
Product: akodelib
Classification: Miscellaneous
Component: general (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR crash
Target Milestone: ---
Assignee: Allan Sandfeld
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-11-16 02:11 UTC by Julio
Modified: 2004-11-17 00:09 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Julio 2004-11-16 02:11:02 UTC
Version:           kdemultimedia-3.3.1, /usr/lib/libakode.so.1.0.0 (using KDE KDE 3.3.1)
Installed from:    Unlisted Binary Package
Compiler:          unknown N/A
OS:                Linux

There seems to be a problem in libakode when reading mmapped files.  I
suspect the problem occurs if the file system does not support mmap.

Here's the general description of the problem and scenarios. Below you
will find a debugger trace for artsd (the test application in this
example). Towards the end there is a code excerpt where I believe the
problem is.  Sorry, no diff.

Here's the setup:
OS: Linux 2.4.26
KDE 3.3.1, compiled by "The KDE-RedHat Project" http://kde-redhat.sf.net.
arts version: 1.3.1
kdemultimedia version: 3.3.1
akode version: /usr/lib/libakode.so.1.0.0


These are 2 scenarios where things go bad:

Scenario 1.
Result: system freeze
Description:

The music files, more specifically in MP3 format, are located in a NTFS
partition.  Assume the artsd daemon was launched by the kde scripts at the
start ot the session.  If one tries to play one of such files using juk,
artsplay, noatun, etc., the whole machine freezes.  For example, it does
not accept any additional incoming ssh connections.  However, it responds
to pings.

Scenario 2.
Result: application (artsd) crashes with SIGSEGV

Description:

Now the arts daemon is launched within a debugger.  The music files, more
specifically in MP3 format, are located in a NTFS partition.  If one tries
to play one of such files using juk, artsplay, noatun, etc., artsd crashes
with SIGSEGV.  Trace provided below.

Here's a scenario where things go well.

Scenario 3.
Result: files are played succesfully.
Play the file from a different partition, e.g., iso9660 or ext2.
The file is played with no problems.

Here's the stack trace produced by gdb for artsd:

-------------------------------------------------
[New Thread 32769 (LWP 924)]
[New Thread 16386 (LWP 925)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 16386 (LWP 925)]
0x407ed54a in memcpy () from /lib/i686/libc.so.6
(gdb) where
#0  0x407ed54a in memcpy () from /lib/i686/libc.so.6
#1  0x40a5c95d in aKode::MMapFile::read(char*, long) () from /usr/lib/libakode.so.1
#2  0x40a63acc in aKode::MPEGDecoder::skipID3v2() ()
   from /usr/lib/libakode_mpeg_decoder.so
#3  0x40a63f52 in aKode::MPEGDecoder::prepare() () from /usr/lib/libakode_mpeg_decoder.so
#4  0x40a642d9 in aKode::MPEGDecoder::readFrame(aKode::AudioFrame*) ()
   from /usr/lib/libakode_mpeg_decoder.so
#5  0x40a5db9f in aKode::StreamToFrameDecoder::audioConfiguration() ()
   from /usr/lib/libakode.so.1
#6  0x40632a21 in pthread_start_thread () from /lib/i686/libpthread.so.0
#7  0x40632b25 in pthread_start_thread_event () from /lib/i686/libpthread.so.0
(gdb)

---------------------------------------------

Artsd crashes in memcpy inside aKode::MMapFile::read(char*, long).

Now, taking a look at kdemultimedia/akode/lib/mmapfile.cpp, it seems that
the problem lies in the openRO method, please look at the comments added
inside the code excerpts:

bool MMapFile::openRO() {
    if(handle) return true;
    struct stat stat;

    fd = ::open(filename, O_RDONLY);
    if (fstat(fd, &stat) < 0) return false;
    len = stat.st_size;
    pos = 0;

    handle = mmap(0, len, PROT_READ, MAP_SHARED, fd, 0);
    if (handle == MAP_FAILED) {  /* <------ HERE !!! */

       /* MAP_FAILED is -1 and handle is never set to NULL,
        * which MMapFile::read checks for later
        */

        ::close(fd);
        return false;
    }

    return true;
}


long MMapFile::read(char* ptr, long num) {

    /* Notice that handle was not set NULL if the mmap failed in
     * openRO, thus the following check will succeed even if handle
     * is not valid.
     */

    if(!handle) return -1;

    if (pos+num > len) num = len-pos;

    /* and of course, this address (handle+pos) won't be valid
     * if mmap failed
     */

    memcpy(ptr, (char*)handle+pos, num);
    pos += num;

    return num;
}

--------------------------------

Assuming this is the real problem, the first fix is to make sure 'handle' is
asigned NULL in openRO when mmap fails.  This would prevent 'MMapFile::read'
from trying to read from an ilegal memory address.

However, the problem is probably deeper than that and requires additional
fixes.  When mmap fails, openRO returns false.  It seems that the
caller of openRO is not checking the return code and proceeded to perform
a read regardless of the fact that the operation failed.  This means that
other modifications are needed where openRO is called.

Finally, I'm not sure about whether this is implemented, but if not, I'd
like to add it to the wish list.  If mmap fails or it is not supported for
some reason, there should be a safe fallback mechanism to old plain file
read(), even if it is slower.  And, please don't tell me that the solution
is to put all my music files in a file system that support mmap.

Finally, I want to include my rant about the bug tracking system ... it's
a pain all the steps one has to go through to submit a bug description,
and the response time (of the bug tracking system) is very low.
Comment 1 Allan Sandfeld 2004-11-17 00:05:24 UTC
CVS commit by carewolf: 

Backport mmap crash-fix
CCBUG: 93343


  M +1 -0      mmapfile.cpp   1.9.2.2


--- kdemultimedia/akode/lib/mmapfile.cpp  #1.9.2.1:1.9.2.2
@@ -66,4 +66,5 @@ bool MMapFile::openRO() {
     if (handle == MAP_FAILED) {
         ::close(fd);
+        handle = 0;
         return false;
     }


Comment 2 Allan Sandfeld 2004-11-17 00:09:36 UTC
CVS commit by carewolf: 

Test if the file can be opened, if not fall back to non-mapped files, then fail.
Thanks to Julio for the _perfect_ bug-report :)
BUG: 93343


  M +11 -1     akodePlayObject_impl.cpp   1.26


--- kdemultimedia/akode/arts_plugin/akodePlayObject_impl.cpp  #1.25:1.26
@@ -35,5 +35,5 @@
 
 #include "audioframe.h"
-//#include "localfile.h"
+#include "localfile.h"
 #include "mmapfile.h"
 #include "framedecoder.h"
@@ -112,4 +112,14 @@ bool akodePlayObject_impl::loadMedia(con
     arts_debug("akode: opening %s", filename.c_str());
     source = new MMapFile(filename.c_str());
+    if (!source->openRO()) {
+        delete source;
+        source = new LocalFile(filename.c_str());
+        if (!source->openRO()) {
+            delete source;
+            source = 0;
+            return false;
+        }
+    }
+    source->close();
     return loadSource();
 }