Valgrind reports invalid write in ds_free_packs

[dmolnar@…]

* Overview: I found a test case mp3 file for mplayer where valgrind 3.2 reports an invalid write of 4 bytes inside an area of memory previously free'd.

The test case is "90-snippet3.mp3" available at the URL
​http://www.cs.berkeley.edu/~dmolnar/90-snippet3.mp3

Please let me know if I omitted something or could add more detail to this report.

* Mplayer version: Compiled from source of mplayer-checkout-2007-11-14. I've attempted to reproduce on mplayer-checkout-2007-12-18, but I encounter a compile error with that snapshot.

* To reproduce:

1) Play 90-snippet3.mp3 in mplayer under Valgrind 3.2

* My OS:
Red Hat Enterprise Linux 4, Pentium D, uname -a:
Linux koschei.eecs.berkeley.edu 2.6.9-5.ELsmp #1 SMP Wed Jan 5 19:30:39 EST 2005 i686 i686 i386 GNU/Linux

* mplayer output:
[dmolnar@koschei mplayertest5]$ /home/dmolnar/mplayer/inst/bin/mplayer 90-snippet3.mp3 < /dev/null
/home/dmolnar/mplayer/inst/bin/mplayer: /usr/lib/libtheora.so.0: no version information available (required by /home/dmolnar/mplayer/inst/bin/mplayer)
MPlayer dev-SVN-r25045-3.4.5 (C) 2000-2007 MPlayer Team
CPU: Intel(R) Pentium(R) D CPU 3.00GHz (Family: 15, Model: 6, Stepping: 4)
CPUflags: MMX: 1 MMX2: 1 3DNow: 0 3DNow2: 0 SSE: 1 SSE2: 1
Compiled for x86 CPU with extensions: MMX MMX2 SSE SSE2

Playing 90-snippet3.mp3.
Audio file file format detected.
==========================================================================
Opening audio decoder: [mp3lib] MPEG layer-2, layer-3
AUDIO: 44100 Hz, 2 ch, s16le, 128.0 kbit/9.07% (ratio: 16000->176400)
Selected audio codec: [mp3] afm: mp3lib (mp3lib MPEG layer-2, layer-3)
==========================================================================
AO: [oss] 44100Hz 2ch s16le (2 bytes per sample)
Video: no video
Starting playback...
A: 1.2 (01.2) of 43750.0 (12:09:10.0) 0.7%
[mplayer appears to hang here, press control-C or wait ~5 mins to finish]

Valgrind output, from command
valgrind --tool=memcheck --log-file-exactly=debug /home/dmolnar/mplayer/inst/bin/mplayer 90-snippet3.mp3 < /dev/null

==1045== Invalid write of size 4
==1045== at 0x81521BB: ds_free_packs (demuxer.h:262)
==1045== by 0x8152253: free_demuxer_stream (demuxer.c:139)
==1045== by 0x8152967: free_demuxer (demuxer.c:313)
==1045== by 0x808B722: uninit_player (mplayer.c:599)
==1045== by 0x808B761: exit_player_with_rc (mplayer.c:669)
==1045== by 0x808B883: exit_player (mplayer.c:707)
==1045== by 0x1288E7: (within /lib/tls/libc-2.3.4.so)
==1045== by 0x81B2CF0: stream_fill_buffer (stream.c:268)
==1045== by 0x81570EB: stream_read (stream.h:209)
==1045== by 0x815A5E0: demux_audio_fill_buffer (demux_audio.c:562)
==1045== by 0x8152DDB: ds_fill_buffer (demuxer.c:423)
==1045== by 0x8153048: demux_read_data (demuxer.c:442)

==1045== Address 0x4632EA4 is 44 bytes inside a block of size 56 free'd
==1045== at 0x4005255: free (vg_replace_malloc.c:233)
==1045== by 0x8152D35: ds_fill_buffer (demuxer.h:271)
==1045== by 0x8153048: demux_read_data (demuxer.c:442)
==1045== by 0x813578B: mplayer_audio_read (ad_mp3lib.c:28)
==1045== by 0x84BA583: MP3_DecodeFrame (sr1.c:63)
==1045== by 0x80F7545: decode_audio (dec_audio.c:373)
==1045== by 0x8091D84: main (mplayer.c:1794)

[reimar]

If you press CTRL+C the signal handler will try to clean up, but this is not possible cleanly in the general way. So the valgrind report is not unexpected (and in general not avoidable) in that case.
I made a change that greatly reduces the likelyhood of this case though.

[daw-bugzilla@…]

(In reply to comment #1)

If you press CTRL+C the signal handler will try to clean up, but this is not
possible cleanly in the general way. So the valgrind report is not unexpected
(and in general not avoidable) in that case.

In this case the bug is that mplayer is accessing memory after it has been freed. For what it's worth, I'll point out that accessing memory after it has been freed can introduce security vulnerabilities, and mplayer is often used on files obtained from unknown sources on the network, so in general it seems like it would be safest to ensure that mplayer never does that (not even in rare corner cases, since if attackers can choose a file that triggers those rare corner cases, one may be in trouble). I would have thought that it would be possible to ensure that mplayer never accesses memory after it has already been freed (there are a number of general ways to do that), though I'm not familiar with the mplayer codebase.

[reimar]

While you have a point, you also miss some other critical points.
Firstly, a file can not trigger it by itself, at the very least the user must press CTRL+C at the very same place.
Secondly, the problem is in a signal handler, thus the solutions are only: 1) do not do any cleanup in signal handlers (might be a possibility but has some issues with vidix and network stuff) or 2) make all code fully thread-safe without using any blocking locks (because those would make the signal handler hang).
2) can be considered impossible IMO.
I would not mind a good solution, but so far the best I can offer is: "use kill -9 to stop playback of an untrusted source" - which will leave the terminal in an unusable state.

[daw-bugzilla@…]

(In reply to comment #3)

While you have a point, you also miss some other critical points.
Firstly, a file can not trigger it by itself, at the very least the user must
press CTRL+C at the very same place.

When mplayer appears to have "hung", pressing CTRL+C is a pretty natural thing for a user to do. If that action can cause the user's account to be taken over by malicious code, that does not seem like a desirable outcome.

Secondly, the problem is in a signal handler, thus the solutions are only: 1)
do not do any cleanup in signal handlers

Hmm. This discussion makes it sound like the mplayer code may have more systemic problems that go beyond this particular bug report. You're not supposed to call non-reentrant functions (like free()) from signal handlers anyway. For instance, the POSIX standard says not to do that, and many references on secure programming warn that violating this rule can lead to security vulnerabilities [1,2,3,4,5,6]. And, of course, double-freeing memory is a serious security risk as well [7,8]. So it seems to me that mplayer is already doing something dubious that likely ought to be fixed.

I realize that this is easy for me to say, as I am blithely ignorant of the costs of fixing the issue (for instance, I have no clue about the nature of the vidix or network issues). And I recognize that fixing this may be decidedly non-trivial. On the other hand, attackers don't care about how easy or hard it would be to fix a security vulnerability; they only care whether such a vulnerability exists.

For these reasons, leaving the code unchanged seems risky to me. Of course, you'll have to make your own cost/benefit/risk assessment; you're presumably in the best position to do that.

I guess I'm probably telling you things you already know. I apologize that I'm not able to be of any help.

[1] ​http://lcamtuf.coredump.cx/signals.txt
[2] ​http://www.gnu.org/software/libc/manual/html_node/Nonreentrancy.html
[3] ​http://www.owasp.org/index.php/Race_condition_in_signal_handler
[4] ​http://www.owasp.org/index.php/Unsafe_function_call_from_a_signal_handler
[5] ​http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/signals.html
[6] ​https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers

[7] ​http://www.owasp.org/index.php/Doubly_freeing_memory
[8] ​https://www.securecoding.cert.org/confluence/display/seccode/MEM31-C.+Free+dynamically+allocated+memory+exactly+once

[reimar]

Hmm... I don't know why I closed this, I intended to have a further look at it anyway, I was only in a hurry to give at least some solution while I was away for the holidays...

[reimar]

Should be mostly fixed in SVN r25651, though some of the code is still not nice for a signal handler. Not sure how it could be improved without loosing functionality.