mpg123 / Bugs / #54 mpg123 says bufferoverflow if full path is

Thomas Orgis - 2007-12-28

Logged In: YES
user_id=470743
Originator: NO

Hm, cannot confirm that with current version, a command line like that works for me:

mpg123 -o alsa -C '/data/thorma/var/music/sampler/1991_techno_attack_original_german_techno-house/06-ministers_of_sex_@_secrets_of_sexual_attraction_(nympho_mix).mp3'

The mp3 file path is well above 49 chars. That's on a Source Mage install with glibc 2.4, alsa output would be defalult anyway.
But, reading your backtrace

/lib/libc.so.6(__chk_fail+0x41)[0xb7ddf3b1]
/lib/libc.so.6[0xb7ddfaf4]
mpg123[0x80572c4]
mpg123(open_module+0x11)[0x8057351]

it looks like a crash in the module loader (which is a bit strange if triggered by mp3 file path).
Can you compile mpg123 with the -g compiler switch and try the same again?
The backtrace (mpg123 ran from the source directory) should look more helpful then.
Also, does the crash happen when compiling without dynamic modules (./configure --disable-modules)?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

swi - 2007-12-28

Logged In: YES
user_id=1635116
Originator: NO

I confirm that bug.

morsov@alexey Music/Pink Floyd/Pink Floyd - 1975 - Wish You Were Here $ mpg123 -C Pink\ Floyd\ -\ 04\ -\ Wish\ You\ Were\ Here.mp3
[getlopt.c:115] debug: getsingleopt: -C
[getlopt.c:71] debug: int at 0x61cca4
[getlopt.c:77] debug: casting assignment done
[getlopt.c:115] debug: getsingleopt: Pink Floyd - 04 - Wish You Were Here.mp3
[module.c:159] debug: pwd: increased buffer to 100
*** buffer overflow detected ***: mpg123 terminated
======= Backtrace: =========
/lib64/libc.so.6(__chk_fail+0x2f)[0x2b66326554ff]
/lib64/libc.so.6[0x2b6632655b27]
mpg123[0x413f40]
mpg123(open_module+0x2f)[0x41400f]
mpg123(open_output_module+0x151)[0x405c61]
mpg123(init_output+0x4c)[0x405dfc]
mpg123(main+0x406)[0x40e636]
/lib64/libc.so.6(__libc_start_main+0xf4)[0x2b66325a6c14]
mpg123[0x404c99]
======= Memory map: ========

Sure, if i pass fullpath to filename it'll play. So crash appear when get current dir fullpath name. mpg123 expand buffer from initial 50 (PATH_STEP) to 100, but not successfull.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

swi - 2007-12-28

Logged In: YES
user_id=1635116
Originator: NO

I think this patch solve this problem. At least it's work for me.

================ patch begin ===============
diff --git a/mpg123/src/module.c b/mpg123/src/module.c
index 25f91e4..3efc7e7 100644
--- a/mpg123/src/module.c
+++ b/mpg123/src/module.c
@@ -9,6 +9,7 @@
#include <dirent.h>
#include <sys/stat.h>
#include <unistd.h>
+#include <limits.h>
#include <errno.h>
#include <ctype.h>
#include <ltdl.h>
@@ -146,19 +147,58 @@ void close_module( mpg123_module_t* module )

}

-#define PATH_STEP 50
+#ifdef PATH_MAX
+static int pathmax = PATH_MAX;
+#else
+static int pathmax = 0;
+#endif
+
+static long posix_version = 0;
+
+#define PATH_MAX_GUESS 1024
+//#define PATH_STEP 50
static char *get_the_cwd()
{
- size_t bs = PATH_STEP;
- char *buf = malloc(bs);
- while((buf != NULL) && getcwd(buf, bs) == NULL)
+ char *buf;
+ int size;
+
+ if (posix_version == 0)
+ posix_version = sysconf(_SC_VERSION);
+
+ if (pathmax == 0)
+ { /* first call of the function */
+ errno = 0;
+ if ((pathmax = pathconf("/", _PC_PATH_MAX)) < 0 )
+ {
+ if (errno == 0)
+ pathmax = PATH_MAX_GUESS;
+ else
+ {
+ printf("Error call sysconf with _PC_PATH_MAX\n");
+ exit(133);
+ }
+ }
+ else
+ {
+ pathmax++;
+ }
+ }
+
+ size = pathmax;
+
+ if ((buf = malloc(size)) == NULL)
{
- char *buf2;
- buf2 = realloc(buf, bs+=PATH_STEP);
- if(buf2 == NULL){ free(buf); buf = NULL; }
- else debug1("pwd: increased buffer to %lu", (unsigned long)bs);
+ printf("Error call malloc\n");
+ exit(133);
}
- return buf;
+
+ if (getcwd(buf,size) == NULL)
+ {
+ printf("Error getcwd\n");
+ exit(133);
+ }
+
+ return(buf);
}

void list_modules()
================ patch end ===============

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2007-12-28

Logged In: YES
user_id=470743
Originator: NO

Aha, that makes more sense: We are in a directory with >=50 chars path length and call mpg123 with output modules.
Konqueror gives full path to mpg123, while the run from command line was from within the file's directory with long path.

Long story short: Samuraiwithin, I can fix it with a much smaller patch -- that could mean that I'm smart but I'm the one who failed to add the crucial line in the first place...

The idea of the function is right as such:

#define PATH_STEP 50
static char *get_the_cwd()
{
size_t bs = PATH_STEP;
char *buf = malloc(bs);
while((buf != NULL) && getcwd(buf, bs) == NULL)
{
char *buf2;
buf2 = realloc(buf, bs+=PATH_STEP);
if(buf2 == NULL){ free(buf); buf = NULL; }
else debug1("pwd: increased buffer to %lu", (unsigned long)bs);
}
return buf;
}

But it fails to actually use the reallocated buffer!
Please test the following patch, which shall very quickly result into release 1.0.1:

Index: src/module.c

--- src/module.c (Revision 1312)
+++ src/module.c (Arbeitskopie)
@@ -157,6 +157,8 @@
buf2 = realloc(buf, bs+=PATH_STEP);
if(buf2 == NULL){ free(buf); buf = NULL; }
else debug1("pwd: increased buffer to %lu", (unsigned long)bs);
+
+ buf = buf2;
}
return buf;
}

My glibc is not complaining about the overflow and it silently works for me (perhaps not for 2000 char paths)... so I cannot definetely confirm the fix but it looks rather obvious.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2007-12-28

milestone: 614537 --> 1.0.0

priority: 5 --> 9
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

swi - 2007-12-28

Logged In: YES
user_id=1635116
Originator: NO

Thanks for quick response.
Well, my patch in fact riped from "Advanced Programming in UNIX environment: Second Edition" book :) It's use standard PATH_MAX, that is defined on allmost systems.

Rebuild package mpg123 with your patch successfull solve this problem too :)

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2007-12-28

Logged In: YES
user_id=470743
Originator: NO

Yeah, your code is POSIX.1-2001, which is a portability standard.
But we are a bit notorious with mpg123 to keep it working on pre-POSIX systems, even.
Call it a spleen;-) Well, or think about MinG"32 platform, which
Though I don't suppose the really old systems to use the new module loading stuff (we still have a plain Makefile in place for systems where the autoconf stuff doesn't work...), relying on just malloc, realloc and getcwd is even more portable.

Anyhow, going to release the bugfix version 1.0.1 quickly to have that nasty bug squashed.
Thanks for the quick dialog!

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

imre_herceg - 2007-12-28

Logged In: YES
user_id=1968536
Originator: YES

I also added the line buf=buf2; to the file module.c and compiled the program, now it works allright.
Thanks

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2007-12-28

status: open --> closed-fixed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2007-12-28

Logged In: YES
user_id=470743
Originator: NO

Thanks to both of you for the work on this one, closing with new release.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

mpg123 says bufferoverflow if full path is > 49 characters

Release Series

Searches

Help

#54 mpg123 says bufferoverflow if full path is > 49 characters

Discussion

Index: src/module.c