[SRU] ripperX assert failure: *** buffer overflow detected ***: ripperx terminated

[ahornby]

Binary package hint: ripperx

crashed after last track ripped and encoded

ProblemType: Crash
Architecture: amd64
AssertionMessage: *** buffer overflow detected ***: ripperx terminated
Date: Sun Jan 31 00:10:32 2010
DistroRelease: Ubuntu 10.04
ExecutablePath: /usr/bin/ripperX
Package: ripperx 2.7.2-3
ProcCmdline: ripperx
ProcEnviron:
 PATH=(custom, user)
 LANG=en_AU.utf8
 SHELL=/bin/bash
ProcVersionSignature: Ubuntu 2.6.32-10.14-generic
Signal: 6
SourcePackage: ripperx
StacktraceTop:
 *__GI_raise (sig=<value optimized out>)
 *__GI_abort () at abort.c:92
 __libc_message (do_abort=<value optimized out>,
 *__GI___fortify_fail (
 *__GI___chk_fail () at chk_fail.c:29
Title: ripperX assert failure: *** buffer overflow detected ***: ripperx terminated
Uname: Linux 2.6.32-10-generic x86_64
UserGroups: adm admin cdrom dialout lpadmin plugdev sambashare

[Apport retracing service (apport)]

StacktraceTop:
 *__GI_raise (sig=<value optimized out>)
 *__GI_abort () at abort.c:92
 __libc_message (do_abort=<value optimized out>,
 *__GI___fortify_fail (
 *__GI___chk_fail () at chk_fail.c:29

[Arto Jalkanen (ajalkane)]

This is actually as far as I know a bug in libid3tag library. It happens if trying to tag a track with track number of over 9. Apparently it only affects mp3s that are encoded with VBR. As such, the bug also affects every other program using id3tag library.

See here for more on this:

http://sourceforge.net/projects/ripperx/forums/forum/3069/topic/3659737?message=8386189

[fractal13 (cgl-infowest)]

This problem is not caused by libid3tag. RipperX has an array of size 2 to store the track number string into. So, higher than 1 digits, plus the null character cause the buffer to overflow. I imagine other builds do not have problems because they are lucky with stack frame construction.

Attached is a patch. This works when I rebuild the package from sources.

[Tony Mancill (tmancill)]

On 11/05/2010 09:48 PM, fractal13 wrote:
> This problem is not caused by libid3tag. RipperX has an array of size 2
> to store the track number string into. So, higher than 1 digits, plus
> the null character cause the buffer to overflow. I imagine other builds
> do not have problems because they are lucky with stack frame
> construction.
>
> Attached is a patch. This works when I rebuild the package from
> sources.
>
>
> ** Patch added: "fix_2_digit_track_numbers.diff"
> https://bugs.launchpad.net/ubuntu/+source/ripperx/+bug/514739/+attachment/1724546/+files/fix_2_digit_track_numbers.diff
>

I'll take a look at your patch - thanks for that - but find it kind of
difficult to believe that this is the cause for an issue has only
recently started happening. I've been using ripperX regularly for over
5 years on Debian with CDs that have more 9 tracks and never encountered
this problem (and I still don't have the problem locally). Your success
could be related to your rebuild - i.e. perhaps some other change in the
environment/dependencies.

Thanks,
Tony

[fractal13 (cgl-infowest)]

I rebuilt ripperX from the source packages and the error returned on my system. This would seem to be good evidence that the patch is useful.

I also have used ripperX for many years. That's why I was motivated to find a solution to the problem when I moved to lucid. However, just because the compiler has been kind to us all of these years, doesn't make under allocating space in an array OK. This is an error. The only question is if it is the only error.

Thanks for your work.

[fractal13 (cgl-infowest)]

Here's another buffer overflow bug fix. To see this problem, you have to encode many tracks without restarting ripperX. The path environment variable outgrows the buffer size in this function.

[Thomas Egerer (hakke-007)]

I've encountered the same problem on a non-AMD64 machine.
It's quite obvious why the application crashes, because glibc abort()s due to failed fortification checks. From my experience ripperX reproducibly crashes when finishing an mp3 with a track number > 9 (I ripped a number of CDs and ripperX gave me all tracks encoded and properly named, but without ID3-tag for all those >= 10]). While examining the problem I stumbled across this thread and -- even though fractal13's patch works -- I'd propose a different solution.
a) using dynamically allocated memory *is* overkill. And even if it has little to none performance impact in this case, if all bugs would be fixed like this, I'd still be sitting here watching my box boot instead of writing this.
b) according to red book standard (available for $300 or so :() the number of tracks on a compact disc is limited to 99 (ever wondered why your CD player has a two-digit display only?), so a static char buffer of size 3 is sufficient. If you use snprintf to limit the number of characters you're fine.
So please refrain from 'spoiling' my favourite mp3-encoder by using malloc to fix this bug.

[Tony Mancill (tmancill)]

On 11/25/2010 02:06 AM, Thomas Egerer wrote:
> I've encountered the same problem on a non-AMD64 machine.
> It's quite obvious why the application crashes, because glibc abort()s due to failed fortification checks. From my experience ripperX reproducibly crashes when finishing an mp3 with a track number > 9 (I ripped a number of CDs and ripperX gave me all tracks encoded and properly named, but without ID3-tag for all those >= 10]). While examining the problem I stumbled across this thread and -- even though fractal13's patch works -- I'd propose a different solution.
> a) using dynamically allocated memory *is* overkill. And even if it has little to none performance impact in this case, if all bugs would be fixed like this, I'd still be sitting here watching my box boot instead of writing this.
> b) according to red book standard (available for $300 or so :() the number of tracks on a compact disc is limited to 99 (ever wondered why your CD player has a two-digit display only?), so a static char buffer of size 3 is sufficient. If you use snprintf to limit the number of characters you're fine.
> So please refrain from 'spoiling' my favourite mp3-encoder by using malloc to fix this bug.
>
> ** Patch added: "revised patch for this bug not using malloc"
> https://bugs.launchpad.net/ubuntu/+source/ripperx/+bug/514739/+attachment/1744481/+files/fix_2_digit_track_numbers.diff
>

Hi Thomas,

Thank you for your post and the patch. Just to be clear, you are still
experiencing crashes with the latest upstream release of ripperX, 2.7.3?

Thank you,
tony

[Thomas Egerer (hakke-007)]

Hi Tony,

the upstream HEAD revision is not affected by this problem any more. Plus: I had to amend my patch, I overlooked the free statement to s_track_num.
To be honest, I didn't check the svn repo before posting here. The bug is still new while the fix is already applied to the upstream repo.

Thomas

[Tony Mancill (tmancill)]

On 11/27/2010 09:02 AM, Thomas Egerer wrote:
> Hi Tony,
>
> the upstream HEAD revision is not affected by this problem any more. Plus: I had to amend my patch, I overlooked the free statement to s_track_num.
> To be honest, I didn't check the svn repo before posting here. The bug is still new while the fix is already applied to the upstream repo.
>
> Thomas
>
> ** Patch added: "Amended version of the revised fix for this bug"
> https://bugs.launchpad.net/ubuntu/+source/ripperx/+bug/514739/+attachment/1747091/+files/fix_2_digit_track_numbers.diff
>

Hi Thomas,

It appears that the latest upstream release will be part of the next Ubuntu
release [1].

Hopefully that resolves the issue for good.

Cheers,
tony

[1] http://packages.ubuntu.com/natty/ripperx

[Marc Deslauriers (mdeslaur)]

Patches in comments 6 and 9 are in 2.7.3-1 in natty. We need to fix this in lucid and maverick.

[Marc Deslauriers (mdeslaur)]

SRU Request:

Impact: Application segfaults if encoding more than 9 songs because of buffer overflow.

Updates were uploaded containing the patches from this bug that are also now in natty. See attached debdiff for details.

[Martin Pitt (pitti)]

Accepted ripperx into lucid-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

[Martin Pitt (pitti)]

Accepted ripperx into maverick-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

[fractal13 (cgl-infowest)]

I've ripped several disks with the lucid-proposed package, and no problems occur with > 9 tracks.

However, it would be really nice if the package also contained the fix here: https://bugs.launchpad.net/ubuntu/+source/ripperx/+bug/671852.

Thanks to everyone for their work on this project.

[Martin Pitt (pitti)]

Thanks for testing! Still keeping verification-needed for the maverick update, can anyone please test that?

[Launchpad Janitor (janitor)]

This bug was fixed in the package ripperx - 2.7.2-3ubuntu0.10.04.1

---------------
ripperx (2.7.2-3ubuntu0.10.04.1) lucid-proposed; urgency=low

  * Fix buffer overflows (LP: #514739)
    - src/job_control.c: allow more than one digit track numbers.
    - src/ripper_encoder_manipulation.c: only add path if it isn't already
      present.
 -- Marc Deslauriers <email address hidden> Tue, 28 Dec 2010 13:30:33 -0500

[maf (maf-tkrat)]

I have tested the maverick update package and it does indeed fix the problem for me.

[Launchpad Janitor (janitor)]

This bug was fixed in the package ripperx - 2.7.2-3ubuntu0.10.10.1

---------------
ripperx (2.7.2-3ubuntu0.10.10.1) maverick-proposed; urgency=low

  * Fix buffer overflows (LP: #514739)
    - src/job_control.c: allow more than one digit track numbers.
    - src/ripper_encoder_manipulation.c: only add path if it isn't already
      present.
 -- Marc Deslauriers <email address hidden> Tue, 28 Dec 2010 13:30:33 -0500