Glyph detection does not work anymore

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Install 1.1.1
2. Send glyph detection sequence

What is the expected output? What do you see instead?
No information

Please use labels and text to provide additional information.

Original issue reported on code.google.com by towom...@googlemail.com on 18 Jun 2012 at 10:31

[GoogleCodeExporter]

Works fine for me (pressing enter twice after typing the command):

  $ echo -ne '\e]7771;?;1000\a'; cat -v

  ^[]7771;!;1000^[\

Please check again.

Original comment by andy.koppe on 18 Jun 2012 at 7:52

• Changed state: Undecided

[GoogleCodeExporter]

Cannot reproduce failure of the explicit command line test (although I think I 
saw it initially).
However, with mined, the glyph request doesn't get a response (even after 
significantly increasing the timeout).
After some more investigation and comparison of versions, I found it does not 
happen if mintty (including version 1.1.1) is compiled with gcc-3 but only if 
compiled with gcc-4.
Weird. Puzzled and further analysing... Maybe a timing issue that shows up only 
in some raw terminal mode?

Original comment by towom...@googlemail.com on 21 Jun 2012 at 1:12

[GoogleCodeExporter]

Hint for quick testing: mined uses ASCII menu borders if the glyph check fails 
and graphic menu borders if it succeeds (and the font contains border graphics).

Original comment by towom...@googlemail.com on 21 Jun 2012 at 1:15

[GoogleCodeExporter]

Analysed the issue:
In termout.c: do_cmd,
the size of the wcs array to handle glyphs is only 4 elements!
So as I'm requesting 45 glyphs, it obviously flows overs.
gcc-3 put the wcs variable above the string pointer in memory:
s @22A5AC wcs @22A5B0
so it did not matter;
gcc-4 has apparently reversed the order of variables in the stack frame:
s @22A5F8 wcs @22A5B4
since now wcs is below s it will overflow into it and destroy the request string
which then aborts the whole response at the ';' check.

Original comment by towom...@googlemail.com on 22 Jun 2012 at 1:00

[GoogleCodeExporter]

PS: command line check will also reveal the error, if only the string is long 
enough...

Original comment by towom...@googlemail.com on 22 Jun 2012 at 1:06

[GoogleCodeExporter]

Patch below (instead of counting requested glyphs precisely, a heuristic value 
might be term.cmd_len / 4).

--- termout.c   2012-05-10 06:57:55.000000000 +0200
+++ termout.c.patch     2012-06-22 15:27:16.545492100 +0200
@@ -952,7 +952,13 @@ do_cmd(void)
     when 7771: {  // Enquire about font support for a list of characters
       if (*s++ != '?')
         return;
-      wchar wcs[sizeof(term.cmd_len)];
+      char * spoi = s;
+      int semi = 0;
+      while (* spoi) {
+        if (* spoi ++ == ';')
+          semi ++;
+      }
+      wchar * wcs = malloc (semi * sizeof (wchar));
       uint n = 0;
       while (*s) {
         if (*s++ != ';')
@@ -967,6 +973,7 @@ do_cmd(void)
           s += sprintf(s, "%u", wcs[i]);
       }
       *s = 0;
+      free (wcs);
       child_printf("\e]7771;!%s\e\\", term.cmd_buf);
     }
   }

Original comment by towom...@googlemail.com on 22 Jun 2012 at 1:30

[GoogleCodeExporter]

Oh dear. Big thanks for analysing this.

Original comment by andy.koppe on 22 Jun 2012 at 3:48

• Changed state: Accepted
• Added labels: Difficulty-Easy, Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Fixed in r1284 on 1.1 branch, by simply dropping the sizeof operator, which 
obviously shouldn't have been there in the first place. Every argument 
character could be a semicolon, in which case the buffer is filled up with 
zeroes for each of them.

Original comment by andy.koppe on 22 Jul 2012 at 12:21

• Changed state: Fixed
• Added labels: Milestone-1.1.2

[GoogleCodeExporter]

Interesting - I wasn't aware that gcc would allow dynamic array sizes...

Original comment by towom...@googlemail.com on 6 Aug 2012 at 1:06

[GoogleCodeExporter]

Variable length arrays are part of the C99 standard. Very handy, if used 
correctly ...

Original comment by andy.koppe on 13 Sep 2012 at 7:38

• Changed title: Glyph detection does not work anymore
• Changed state: Verified