cmus crashes on selecting track with umlaut in title

[e***@arcor.de]

Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:31
31 pcmpeqb (%rdi), %xmm2
(gdb) bt
#0 strlen () at ../sysdeps/x86_64/strlen.S:31
#1 0x00007f95956c0e3f in tparm () from /lib/libncursesw.so.5
#2 0x0000000000423a17 in set_title (title=0x6411a0 "Die Ärzte - Geräusch (disc 1: Schwarzes Geräusch) - Schneller leben (2003)") at ui_curses.c:1146
#3 0x00000000004257d8 in do_update_titleline () at ui_curses.c:1209
#4 0x00000000004278c5 in update (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1762
#5 main_loop (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1855
#6 main (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:2242

glibc version: 2.11.1
ncurses version: 5.7


Und was machen Sie heute abend? Alles Events Ihrer Gegend auf einen Blick im Arcor.de-Veranstaltungskalender: http://www.arcor.de/rd/footer.events

[Xavier Chantry]

Post by e***@arcor.de
Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:31
31              pcmpeqb (%rdi), %xmm2
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:31
#1  0x00007f95956c0e3f in tparm () from /lib/libncursesw.so.5
#2  0x0000000000423a17 in set_title (title=0x6411a0 "Die Ärzte - Geräusch (disc 1: Schwarzes Geräusch) - Schneller leben (2003)") at ui_curses.c:1146
#3  0x00000000004257d8 in do_update_titleline () at ui_curses.c:1209
#4  0x00000000004278c5 in update (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1762
#5  main_loop (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1855
#6  main (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:2242
glibc version: 2.11.1
ncurses version: 5.7

Ah yeah, that's the error I have been fighting all day.

That segfault is not the only problem I have. I also get "text
corruption". Instead of setting window title, it just displays some
random strings inside cmus window.

IMO there is some bug in ui_curses.c that causes static const char
*t_ts to get corrupted, because that string does not contain what it
should.
It's first correctly set in init_curses with t_ts = "\033]0;";
But that value is always overridden afterwards, I am still struggling
to figure out why.

Note that the problem goes away with gcc -O0.
But with clang, it never goes away.

Another workaround is just to disable set_term_title (:set set_term_title=0)

[Xavier Chantry]

On Sun, May 23, 2010 at 3:24 PM, Xavier Chantry

Post by Xavier Chantry

Post by e***@arcor.de
Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:31
31              pcmpeqb (%rdi), %xmm2
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:31
#1  0x00007f95956c0e3f in tparm () from /lib/libncursesw.so.5
#2  0x0000000000423a17 in set_title (title=0x6411a0 "Die Ärzte - Geräusch (disc 1: Schwarzes Geräusch) - Schneller leben (2003)") at ui_curses.c:1146
#3  0x00000000004257d8 in do_update_titleline () at ui_curses.c:1209
#4  0x00000000004278c5 in update (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1762
#5  main_loop (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1855
#6  main (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:2242
glibc version: 2.11.1
ncurses version: 5.7

Ah yeah, that's the error I have been fighting all day.
That segfault is not the only problem I have. I also get "text
corruption". Instead of setting window title, it just displays some
random strings inside cmus window.
IMO there is some bug in ui_curses.c that causes static const char
*t_ts to get corrupted, because that string does not contain what it
should.
It's first correctly set in init_curses with t_ts = "\033]0;";
But that value is always overridden afterwards, I am still struggling
to figure out why.
Note that the problem goes away with gcc -O0.
But with clang, it never goes away.
Another workaround is just to disable set_term_title (:set set_term_title=0)

Ok I think there is an overflow in print_buffer, maybe just off-by-one
error, as this fixes the problem :
-static char print_buffer[512];
+static char print_buffer[511];

And this would match how ts value was modified, just the last byte was set to 0.

I need some more time reading ui_curses.c to understand fully how that
buffer is manipulated :)

[Gregory Petrosyan]

On Sun, May 23, 2010 at 5:24 PM, Xavier Chantry

Post by Xavier Chantry

Post by e***@arcor.de
Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:31
31 pcmpeqb (%rdi), %xmm2
(gdb) bt
#0 strlen () at ../sysdeps/x86_64/strlen.S:31
#1 0x00007f95956c0e3f in tparm () from /lib/libncursesw.so.5
#2 0x0000000000423a17 in set_title (title=0x6411a0 "Die Ärzte - Geräusch (disc 1: Schwarzes Geräusch) - Schneller leben (2003)") at ui_curses.c:1146
#3 0x00000000004257d8 in do_update_titleline () at ui_curses.c:1209
#4 0x00000000004278c5 in update (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1762
#5 main_loop (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1855
#6 main (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:2242
glibc version: 2.11.1
ncurses version: 5.7

Ah yeah, that's the error I have been fighting all day.
That segfault is not the only problem I have. I also get "text
corruption". Instead of setting window title, it just displays some
random strings inside cmus window.
IMO there is some bug in ui_curses.c that causes static const char
*t_ts to get corrupted, because that string does not contain what it
should.
It's first correctly set in init_curses with t_ts = "\033]0;";
But that value is always overridden afterwards, I am still struggling
to figure out why.
Note that the problem goes away with gcc -O0.
But with clang, it never goes away.
Another workaround is just to disable set_term_title (:set set_term_title=0)

*Love* this kind of bugs!

Unfortunately, can't reproduce it on up-to-date Ubuntu 10.04 (GCC
4.4.3-4ubuntu5).

Because several people are reporting it, my guess is that something in
a build environment became broken due to recent update, or, much more
likely, recent build environment update reveals the bug that was
present in cmus for a long time.

[reading ui_curses.c]

Gregory

[Xavier Chantry]

On Sun, May 23, 2010 at 3:53 PM, Gregory Petrosyan

Post by Gregory Petrosyan
On Sun, May 23, 2010 at 5:24 PM, Xavier Chantry

Post by Xavier Chantry

Post by e***@arcor.de
Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:31
31              pcmpeqb (%rdi), %xmm2
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:31
#1  0x00007f95956c0e3f in tparm () from /lib/libncursesw.so.5
#2  0x0000000000423a17 in set_title (title=0x6411a0 "Die Ärzte - Geräusch (disc 1: Schwarzes Geräusch) - Schneller leben (2003)") at ui_curses.c:1146
#3  0x00000000004257d8 in do_update_titleline () at ui_curses.c:1209
#4  0x00000000004278c5 in update (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1762
#5  main_loop (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1855
#6  main (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:2242
glibc version: 2.11.1
ncurses version: 5.7

Ah yeah, that's the error I have been fighting all day.
That segfault is not the only problem I have. I also get "text
corruption". Instead of setting window title, it just displays some
random strings inside cmus window.
IMO there is some bug in ui_curses.c that causes static const char
*t_ts to get corrupted, because that string does not contain what it
should.
It's first correctly set in init_curses with t_ts = "\033]0;";
But that value is always overridden afterwards, I am still struggling
to figure out why.
Note that the problem goes away with gcc -O0.
But with clang, it never goes away.
Another workaround is just to disable set_term_title (:set set_term_title=0)

*Love* this kind of bugs!
Unfortunately, can't reproduce it on up-to-date Ubuntu 10.04 (GCC
4.4.3-4ubuntu5).
Because several people are reporting it, my guess is that something in
a build environment became broken due to recent update, or, much more
likely, recent build environment update reveals the bug that was
present in cmus for a long time.
[reading ui_curses.c]

I also thought it was a recent build environment update revealing an old bug.

Another clue to save your time, better read format_print.c rather than
ui_curses.c since that fix it too :
@@ -1209,7 +1210,7 @@ static void do_update_titleline(void)
format_print(print_buffer, sizeof(print_buffer) - 1,
window_title_alt_format, track_fopts);
} else {
- format_print(print_buffer, sizeof(print_buffer) - 1,
+ format_print(print_buffer, sizeof(print_buffer) - 2,
window_title_format, track_fopts);
}

Sidenote : this seems to be unused :
#define print_buffer_size (sizeof(print_buffer) - 1)

[Xavier Chantry]

On Sun, May 23, 2010 at 3:57 PM, Xavier Chantry

Post by Xavier Chantry
On Sun, May 23, 2010 at 3:53 PM, Gregory Petrosyan

Post by Gregory Petrosyan
On Sun, May 23, 2010 at 5:24 PM, Xavier Chantry

Post by Xavier Chantry

Post by e***@arcor.de
Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:31
31              pcmpeqb (%rdi), %xmm2
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:31
#1  0x00007f95956c0e3f in tparm () from /lib/libncursesw.so.5
#2  0x0000000000423a17 in set_title (title=0x6411a0 "Die Ärzte - Geräusch (disc 1: Schwarzes Geräusch) - Schneller leben (2003)") at ui_curses.c:1146
#3  0x00000000004257d8 in do_update_titleline () at ui_curses.c:1209
#4  0x00000000004278c5 in update (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1762
#5  main_loop (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:1855
#6  main (argc=<value optimized out>, argv=0x7fff8174ce60) at ui_curses.c:2242
glibc version: 2.11.1
ncurses version: 5.7

Ah yeah, that's the error I have been fighting all day.
That segfault is not the only problem I have. I also get "text
corruption". Instead of setting window title, it just displays some
random strings inside cmus window.
IMO there is some bug in ui_curses.c that causes static const char
*t_ts to get corrupted, because that string does not contain what it
should.
It's first correctly set in init_curses with t_ts = "\033]0;";
But that value is always overridden afterwards, I am still struggling
to figure out why.
Note that the problem goes away with gcc -O0.
But with clang, it never goes away.
Another workaround is just to disable set_term_title (:set set_term_title=0)

*Love* this kind of bugs!
Unfortunately, can't reproduce it on up-to-date Ubuntu 10.04 (GCC
4.4.3-4ubuntu5).
Because several people are reporting it, my guess is that something in
a build environment became broken due to recent update, or, much more
likely, recent build environment update reveals the bug that was
present in cmus for a long time.
[reading ui_curses.c]

I also thought it was a recent build environment update revealing an old bug.
Another clue to save your time, better read format_print.c rather than
@@ -1209,7 +1210,7 @@ static void do_update_titleline(void)
                       format_print(print_buffer, sizeof(print_buffer) - 1,
                                       window_title_alt_format, track_fopts);
               } else {
-                       format_print(print_buffer, sizeof(print_buffer) - 1,
+                       format_print(print_buffer, sizeof(print_buffer) - 2,
                                       window_title_format, track_fopts);
               }
#define print_buffer_size (sizeof(print_buffer) - 1)

Well as long as we are careful to give length - 1 to format_print, we
should be fine.
The problem only happens with special chars because the code fails to
detect correct width of these char for some reasons.
u_char_width does not seem to work with the titles I have at least, it
always return 1 for every char including wide one.

So at the end of this loop :
while (l_str[pos]) {
str[pos] = l_str[pos];
pos++;
}

With ascii string, we have pos = llen. But with a string with one wide
char, we have pos = llen + 1
So that explains my 'off by one' error.

Maybe uchar.c only supports utf8 and not iso, or vice versa ?
Actually I think most of my id3 tags are ISO something, while uchar.c
mentions utf8/unicode.

[Gregory Petrosyan]

Post by Xavier Chantry
Well as long as we are careful to give length - 1 to format_print, we
should be fine.

But it would be an ugly kludge, you know :-)

Post by Xavier Chantry
The problem only happens with special chars because the code fails to
detect correct width of these char for some reasons.
u_char_width does not seem to work with the titles I have at least, it
always return 1 for every char including wide one.

If I understand the code correctly, u_char_width() returns the *width* of a
character, measured in abstract cells, not its size in UTF-8 representation.

Also, 'str_width' parameter of format_print is not the size of receiving
buffer, it is the desired *width* of resulting string (format_print() needs to
know it to correctly right-align text).

Post by Xavier Chantry
while (l_str[pos]) {
str[pos] = l_str[pos];
pos++;
}
With ascii string, we have pos = llen. But with a string with one wide
char, we have pos = llen + 1

And that's fine! llen is the *width* of string, not its size.

Post by Xavier Chantry
So that explains my 'off by one' error.

I think the error is a bit different.

One character can take up to 4 bytes in UTF-8, so if we have a buffer of 512
bytes, the maximum width we can specify is 512/4 - 1 = 127 cells.

Does this:


diff --git a/ui_curses.c b/ui_curses.c
index 0a390a0..8632734 100644
--- a/ui_curses.c
+++ b/ui_curses.c
@@ -104,7 +104,7 @@ static char print_buffer[512];
/* destination buffer for utf8_encode and utf8_decode */
static char conv_buffer[512];

-#define print_buffer_size (sizeof(print_buffer) - 1)
+#define print_buffer_max_width (sizeof(print_buffer) / 4 - 1)
static int using_utf8;

static const char *t_ts;
@@ -1181,10 +1181,10 @@ static void do_update_titleline(void)

/* set window title */
if (use_alt_format) {
- format_print(print_buffer, sizeof(print_buffer) - 1,
+ format_print(print_buffer, print_buffer_max_width,
window_title_alt_format, track_fopts);
} else {
- format_print(print_buffer, sizeof(print_buffer) - 1,
+ format_print(print_buffer, print_buffer_max_width,
window_title_format, track_fopts);
}


looks sane and fixes the issue?

Gregory

[Xavier Chantry]

On Sun, May 23, 2010 at 5:47 PM, Gregory Petrosyan

Post by Gregory Petrosyan

Post by Xavier Chantry
Well as long as we are careful to give length - 1 to format_print, we
should be fine.

But it would be an ugly kludge, you know :-)

Post by Xavier Chantry
The problem only happens with special chars because the code fails to
detect correct width of these char for some reasons.
u_char_width does not seem to work with the titles I have at least, it
always return 1 for every char including wide one.

If I understand the code correctly, u_char_width() returns the *width* of a
character, measured in abstract cells, not its size in UTF-8 representation.
Also, 'str_width' parameter of format_print is not the size of receiving
buffer, it is the desired *width* of resulting string (format_print() needs to
know it to correctly right-align text).

Post by Xavier Chantry
                while (l_str[pos]) {
                        str[pos] = l_str[pos];
                        pos++;
                }
With ascii string, we have pos = llen. But with a string with one wide
char, we have pos = llen + 1

And that's fine! llen is the *width* of string, not its size.

Post by Xavier Chantry
So that explains my 'off by one' error.

I think the error is a bit different.
One character can take up to 4 bytes in UTF-8, so if we have a buffer of 512
bytes, the maximum width we can specify is 512/4 - 1 = 127 cells.
diff --git a/ui_curses.c b/ui_curses.c
index 0a390a0..8632734 100644
--- a/ui_curses.c
+++ b/ui_curses.c
@@ -104,7 +104,7 @@ static char print_buffer[512];
 /* destination buffer for utf8_encode and utf8_decode */
 static char conv_buffer[512];
-#define print_buffer_size (sizeof(print_buffer) - 1)
+#define print_buffer_max_width (sizeof(print_buffer) / 4 - 1)
 static int using_utf8;
 static const char *t_ts;
@@ -1181,10 +1181,10 @@ static void do_update_titleline(void)
       /* set window title */
       if (use_alt_format) {
-                       format_print(print_buffer, sizeof(print_buffer) - 1,
+                       format_print(print_buffer, print_buffer_max_width,
                   window_title_alt_format, track_fopts);
       } else {
-                       format_print(print_buffer, sizeof(print_buffer) - 1,
+                       format_print(print_buffer,  print_buffer_max_width,
                   window_title_format, track_fopts);
       }
looks sane and fixes the issue?

Ah right, I am stupid, I am always reasoning in term of size instead of width.
Maybe renaming llen/rlen to lwidth/rwidth would make it clearer too.
So yes the function returning the width did work perfectly :)

The patch looks sane and fixes the issue.
However, I am still wondering if this is the safest function interface
possible. Since it's the caller that has to manually check that the
requested width is <= buffer length / 4. Is there any guarantee of
that for all the format_print calls in ui_curses.c ?
Though I don't see a good solution either, besides adding both buffer
length and requested width as argument to format_print and checking
there that width <= length / 4.

I am probably bothering too much :) ACK on your patch.

[Gregory Petrosyan]

On Sun, May 23, 2010 at 8:26 PM, Xavier Chantry

Post by Xavier Chantry
On Sun, May 23, 2010 at 5:47 PM, Gregory Petrosyan

Post by Gregory Petrosyan

Post by Xavier Chantry
Well as long as we are careful to give length - 1 to format_print, we
should be fine.

But it would be an ugly kludge, you know :-)

Post by Xavier Chantry
The problem only happens with special chars because the code fails to
detect correct width of these char for some reasons.
u_char_width does not seem to work with the titles I have at least, it
always return 1 for every char including wide one.

If I understand the code correctly, u_char_width() returns the *width* of a
character, measured in abstract cells, not its size in UTF-8 representation.
Also, 'str_width' parameter of format_print is not the size of receiving
buffer, it is the desired *width* of resulting string (format_print() needs to
know it to correctly right-align text).

Post by Xavier Chantry
while (l_str[pos]) {
str[pos] = l_str[pos];
pos++;
}
With ascii string, we have pos = llen. But with a string with one wide
char, we have pos = llen + 1

And that's fine! llen is the *width* of string, not its size.

Post by Xavier Chantry
So that explains my 'off by one' error.

I think the error is a bit different.
One character can take up to 4 bytes in UTF-8, so if we have a buffer of 512
bytes, the maximum width we can specify is 512/4 - 1 = 127 cells.
diff --git a/ui_curses.c b/ui_curses.c
index 0a390a0..8632734 100644
--- a/ui_curses.c
+++ b/ui_curses.c
@@ -104,7 +104,7 @@ static char print_buffer[512];
/* destination buffer for utf8_encode and utf8_decode */
static char conv_buffer[512];
-#define print_buffer_size (sizeof(print_buffer) - 1)
+#define print_buffer_max_width (sizeof(print_buffer) / 4 - 1)
static int using_utf8;
static const char *t_ts;
@@ -1181,10 +1181,10 @@ static void do_update_titleline(void)
/* set window title */
if (use_alt_format) {
- format_print(print_buffer, sizeof(print_buffer) - 1,
+ format_print(print_buffer, print_buffer_max_width,
window_title_alt_format, track_fopts);
} else {
- format_print(print_buffer, sizeof(print_buffer) - 1,
+ format_print(print_buffer, print_buffer_max_width,
window_title_format, track_fopts);
}
looks sane and fixes the issue?

Ah right, I am stupid, I am always reasoning in term of size instead of width.
Maybe renaming llen/rlen to lwidth/rwidth would make it clearer too.
So yes the function returning the width did work perfectly :)
The patch looks sane and fixes the issue.
However, I am still wondering if this is the safest function interface
possible.

Definitely not. Personally, I'm just too lazy to bother with that right now.

Post by Xavier Chantry
Since it's the caller that has to manually check that the
requested width is <= buffer length / 4. Is there any guarantee of
that for all the format_print calls in ui_curses.c ?

Well, they look more or less safe... (read: not immediately exploding :-)

Post by Xavier Chantry
Though I don't see a good solution either, besides adding both buffer
length and requested width as argument to format_print and checking
there that width <= length / 4.
I am probably bothering too much :) ACK on your patch.

OK, thanks, will push it to -maint shortly.

Gregory