(Apologies for potential duplicate message, just realised I'd used the
wrong From address and so ended up in the moderator's bin)
Hi all,
Found a bug in which lib/rdaudioconvert.cpp ->
RDAudioConvert::Stage1Mpeg uses a statically defined 2500 byte buffer,
but may overflow it when trying to import certain MP3s with (perhaps
malformed?) tags that confuse libmad. This causes import to fail because
rdxport.cgi dies with a stack protection fault (when built with
-fstack-protector), or presumably causes undefined behaviour otherwise.
Here's a test case file (Chase & Status - Alive:
http://cs448.user.srcf.net/alive.mp3). Try to import the file using
rdimport: you should find rdimport reports an unspecified audio
conversion error (it failed to parse Apache's default 500 error
document), whilst there will be a *** buffer overflow detected *** error
in your apache error log reporting rdxport.cgi's death.
For that particular test file the cause was an APE tag at the end of the
file, causing libmad to report MAD_ERROR_LOSTSYNC. This led to the
Stage1Mpeg function immediately bailing from the decode loop for that
frame, then trying to place the remaining data in its buffer at line:
> memmove(buffer,mad_stream.next_frame,left_over)
left_over for this file was over 2500 bytes, causing the overflow.
I attach my workaround patch, which (a) continues to try to decode
frames when recoverable errors occur, and (b) checks that it will not
overflow the buffer, returning a format error if it would. Other buffer
handling in that function may also suspect, but this patch fixes this
particular problem for me.
My environment:
Patch applies against rivendell-2.8.1.
Distro is Ubuntu 12.04 x86_64
Libmad version installed: 0.15.1b-7ubuntu1 (12.04 repository latest)
Chris
--- rivendell-2.8.1/lib/rdaudioconvert.cpp 2014-02-18 18:04:57.000000000
+0000
+++ rivendell-2.8.1-hacked/lib/rdaudioconvert.cpp 2014-03-19
23:42:11.661219069 +0000
@@ -520,6 +520,7 @@
#endif // HAVE_VORBIS
}
+#define STAGE1BUFSIZE 2500
RDAudioConvert::ErrorCode RDAudioConvert::Stage1Mpeg(const QString &dstfile,
RDWaveFile *wave)
@@ -533,7 +534,7 @@
int left_over=0;
int fsize;
int n;
- unsigned char buffer[2500];
+ unsigned char buffer[STAGE1BUFSIZE];
float sf_buffer[1152*2];
sf_count_t start=0;
sf_count_t end=wave->getSampleLength();
@@ -581,11 +582,16 @@
}
mad_stream_buffer(&mad_stream,buffer,n+left_over);
//printf("mad err: %d\n",mad_stream.error);
- if(mad_stream.error==MAD_ERROR_LOSTSYNC) { // FIXME: try to recover!!!
- // return RDAudioConvert::ErrorFormatError;
- //break;
- }
- while(mad_frame_decode(&mad_frame,&mad_stream)==0) {
+ while(1) {
+
+ int thiserr=mad_frame_decode(&mad_frame,&mad_stream);
+ if(thiserr!=0) {
+ if(!MAD_RECOVERABLE(mad_stream.error))
+ break;
+ else
+ continue;
+ }
+
//printf("decoding...\n");
mad_synth_frame(&mad_synth,&mad_frame);
for(int i=0;i<mad_synth.pcm.length;i++) {
@@ -625,9 +631,18 @@
}
}
frames+=mad_synth.pcm.length;
+
}
left_over=mad_stream.bufend-mad_stream.next_frame;
+
+ // Prevent buffer overflow on malformed files.
+ // The amount checked for should match the maximum amount that may be read
+ // by the next top-of-loop wave->readWave call.
+ if(left_over + fsize + 1 > STAGE1BUFSIZE)
+ return RDAudioConvert::ErrorFormatError;
+
memmove(buffer,mad_stream.next_frame,left_over);
+
}
memset(buffer+left_over,0,MAD_BUFFER_GUARD);
mad_stream_buffer(&mad_stream,buffer,MAD_BUFFER_GUARD+left_over);
_______________________________________________
Rivendell-dev mailing list
[email protected]
http://caspian.paravelsystems.com/mailman/listinfo/rivendell-dev
