54694 - segfault on control-VK_LEFT / alt-VK_LEFT

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[Axel Hecht]

If I type control left key, the browser segfaults.

It's in widget/src/gtk/nsGtkEventHandler.cpp:370
and aGEK is
(gdb) p *aGEK
$2 = {type = GDK_KEY_PRESS, window = 0x7cd368, send_event = 0 '\000', time =
3877775034, state = 4, keyval = 65361, length = 0, 
  string = 0x77dc28 ""}

It looks like the guint keyval is too large for the char macro isprint.

Tested on solaris, cvs from Sep 29

gtk 1.2.7, libgdk-1.2.so.0.5.2

Axel

Stack trace:

#0  0xfcf52df0 in nsConvertCharCodeToUnicode (aGEK=0x211698) at
/tmp/mozilla/widget/src/gtk/nsGtkEventHandler.cpp:370
#1  0xfcf53068 in InitKeyPressEvent (aGEK=0x211698, p=0x273348,
anEvent=@0xffbef1f8)
    at /tmp/mozilla/widget/src/gtk/nsGtkEventHandler.cpp:430
#2  0xfcf53c64 in handle_key_press_event (w=0x0, event=0x211698, p=0x7bfba0)
    at /tmp/mozilla/widget/src/gtk/nsGtkEventHandler.cpp:705
#3  0xfcf54690 in dispatch_superwin_event (event=0x211698, window=0x7bfba0)
    at /tmp/mozilla/widget/src/gtk/nsGtkEventHandler.cpp:990
#4  0xfcf54168 in handle_gdk_event (event=0x211698, data=0x0) at
/tmp/mozilla/widget/src/gtk/nsGtkEventHandler.cpp:843
#5  0xfd08d7a0 in gdk_event_dispatch (source_data=0x211698,
current_time=0xffbef4c0, user_data=0x0) at gdkevents.c:2129
#6  0xfccc7710 in g_main_dispatch (dispatch_time=0xffbef4c0) at gmain.c:656
#7  0xfccc8010 in g_main_iterate (block=1236, dispatch=1) at gmain.c:877
#8  0xfccc8220 in g_main_run (loop=0x21a998) at gmain.c:935
#9  0xfcdc71c0 in gtk_main () at gtkmain.c:476
#10 0xfcf4510c in nsAppShell::Run (this=0x7fa30) at
/tmp/mozilla/widget/src/gtk/nsAppShell.cpp:335
#11 0xfdb472a0 in nsAppShellService::Run (this=0xdbda8) at
/tmp/mozilla/xpfe/appshell/src/nsAppShellService.cpp:407
#12 0x235d4 in main1 (argc=1, argv=0xffbef95c, nativeApp=0x0) at
/tmp/mozilla/xpfe/bootstrap/nsAppRunner.cpp:1004
#13 0x23fd0 in main (argc=1, argv=0xffbef95c) at
/tmp/mozilla/xpfe/bootstrap/nsAppRunner.cpp:1185

Comment 1

[Axel Hecht]

heya, just looked into ctype.h, and it defines isprint several ways, but they
have one thing in common:

#define	isprint(c)	(__ctype_mask[c] & _ISPRINT)

They all [c], which might be a bad idea on large input values. Looks like we 
need some bounds check there. Or we're just testing the wrong thing

Axel

Comment 2

[Akkana Peck]

Sounds like we need a bounds check.  That's easy enough to do (though I don't
see this crash on Linux so someone on Solaris will have to verify the fix.

Anyone know whether this happens on the branch?  If so, this should get an rtm
keyword.

Comment 3

[Axel Hecht]

Verified on
ftp://ftp.mozilla.org/pub/mozilla/nightly/2000-09-28-10-MN6/mozilla-sparc-sun-solaris2.6.tar.gz
adding crash and rtm keywords.
It is obviously not only the VK_LEFT.

The difference to linux is, the linux macros have a (int) cast in the macro.
So this may be needed, thought the headers don't tell to what. We perhaps 
shouldn't rely on isprint to be correct for large values. Non char? non int?
I would vote for char. 

Axel

Comment 4

[edburns]

Adding Rich to CC.

Comment 5

[Akkana Peck]

Attachment: Patch: Add some bounds checking

Comment 6

[Axel Hecht]

Applying this patch keeps us from segfaulting, BUT:

history back and forward don't work. They used to work with ctrl VK_LEFT and
RIGHT some time ago, with this patch they just don't.
Reverting nsGtkEventHandler.cpp back to 1.149 makes META!!!! VK_LEFT and RIGHT
do the backward and forward. And they are not even calling the 
nsConvertCharCodeToUnicode function. 
crtl left and right do work in textfields, moving to the start and end of the
text.
I checked in nsXBLProtocolHandler.cpp, but the checks for meta don't get called.
IMHO. It looks like the modifiers are cached, I am clueless as where to look.
hyatt changed that file recently, so adding him for some info on where to
investigate on this.

Axel

Comment 7

[Akkana Peck]

That's bizarre.  Our event handler doesn't even recognize the meta key,
currently (there's a separate bug on that, with a patch from Sun, which may get
checked in soon).  Meta-left and right should not do anything (or at least,
shouldn't do anything different from left and right with no modifier pressed). I
have no idea why it would work before, or why this patch would have changed that.

Someone with a Sun is going to have to help with this, I'm afraid.  Making
changes on Linux and guessing their effect doesn't look like it's going to be
the right approach. :-)

Comment 8

[Axel Hecht]

got my nsGtkEventHandler back to tip+patch, and the textfield navigation is 
broken, too.
But hey, meta left opens the QA menu, meta right the search menu and meta down
the tasks menu. I am a bit dissapointed, meta up is silent.
Needless to mention, that those menus don't come up with revision 1.149.

Axel

Comment 9

[rubydoo123]

per comment from pdt team in another bug, solaris is a lower priority for rtm, 
this will have to wait

Comment 10

[Rich Burridge]

Changing the priority to P1. Fixing this kind of crash is important to Sun,
so applying the small bounds-checking fix in the attachment should be the 
first step in resolving this problem.

Comment 11

[Axel Hecht]

I tested the latest patch by jag on 53667, and it fixes the segfault.
As it completely removes the ctype.h, we may get on safer ground here.

Axel

Comment 12

[Peter "jag" Annema]

So actually, we were testing the right thing, but in the wrong way, something
not fixed by the patch attached here. I've got a more elegant patch attached to
bug 53667 which also solves some cases (bug 53667, bug 55792) of ctrl + key and
alt + key not working on Solaris / Linux (and any others using Gtk). This patch
has r=,mo=Pavlov and sr=blizzard.

Comment 13

[Peter "jag" Annema]

I just checked my patch in on the trunk.

Comment 14

[Peter "jag" Annema]

Taking this bug, I'm pretty sure my checkin fixed it, I'll track.

Note, shaver mentioned this crashes on linux too. From #mozilla:

<shaver> alt-left-arrow to go back in history crashed me
<jag> shaver: but I wonder if it's the key combo, or the history.go(-1) thing
<shaver> jag: using the back button works OK
<jag> shaver: does alt-left-arrow crash your repeatedly?
<shaver> jag: reproducably, yes

Marking platform Linux since that apparantly is of higher priority than Solaris.

PDT: please reconsider this for rtm++.

Comment 15

[Chris McAfee]

*** Bug 54884 has been marked as a duplicate of this bug. ***

Comment 16

[John Morrison]

The current situation is that this bug and bug 53667 are coupled. On the 
branch, ALT-VK_LEFT has no effect, so no crash on the branch, but broken 
functionality. 

On the trunk, where this is checked in, the keybinding works, and I cannot get 
a crash in 2000101708 linux 6.1, even when I just pounded on -> and <- through 
a stack of 10 pages as fast as I could press the keys, continuously for several 
minutes. (And they say you can't find good trained monkeys these days).

I think we should take this patch on the branch for rtm.

Comment 17

[Phil Peterson]

rtm++

Comment 18

[Phil Peterson]

Talked to jag in email, and it's apparent I don't understand which patch is to
fix which bug (between this and bug 53667). jrgm, are you arguing to take jag's
patch on the branch? Or akkana's? If the branch doesn't crash, can we live
without either?

Comment 19

[John Morrison]

Since I was testing the current linux trunk bits, I believe that I was testing
the change jag made on the trunk, and implicitly endorsing that one.

We can live without this binding, albeit an emptier existence. Seriously, I 
was just dealing with aaronl who was reporting a couple of (already reported)
bugs in keyboard accessibility. The general state in this area is already sad
enough that I would really hate to pass on what is by all appearances a solid
fix. [Can some real X-heads give these bindings good bash in a current trunk
build and look for any problems with it].

[I note that there is a new comment at the end of bug 53667 about IME issues,
which may be an issue, but I would hope that i18n can sign off on this change
(or not) in very short order].

Comment 20

[Peter "jag" Annema]

Attachment: [patch] crash fix

Comment 21

[Phil Peterson]

Looks like we need code reviewers on this before it can be rtm++

Comment 22

[Peter "jag" Annema]

The patch I attached is nearly the same as akkana's patch.

The difference is that akkana's patch also checks whether we have a keyval <
256, in which case it's "safe" to call isprint. This fixes the arrow keys case.

However, such a check may be considered too risky (apparantly depending on
undefined isprint behaviour is less risky, since at least then we think we know
how it might fail), so I split off the "prevent crashing" part of akkana's fix.

Comment 23

[Akkana Peck]

r=akkana for the crash fix.

We really should take this -- there's no way this could increase risk, and it
fixes a crash which people are going to hit.

Comment 24

[Mike Shaver (:shaver emeritus)]

Small fix, "provably" zero additional risk, my favourite kind: sr=shaver.

Comment 25

[Peter "jag" Annema]

rtm+ again for the second patch.

Comment 26

[Brendan Eich [:brendan]]

Do we need to make sure the character is in [0,127], per bug 56777?

/be

Comment 27

[Brendan Eich [:brendan]]

PDT, anyone else alarmed by my question: jag argues that we can avoid a crasher
with the last patch ("[patch] crash fix") and that my concern about <ctype.h>
indexing badly with signed chars above ASCII is not a crash-concern, so need not
be fixed for rtm.

/be

Comment 28

[selmer (gone)]

rtm++

Comment 29

[Peter "jag" Annema]

Checked in on branch, trunk has a different fix already, marking fixed.

Comment 30

[John Morrison]

verified fixed for 2000110109 linux MN6 branch build, marking vtrunk.
Anyone wish to check this for Solaris?

No crash, either as Ctrl or ALT plus VK_[LEFT|RIGHT], and these keybindings 
work correctly[**]

[**] modulo bug 58722, where installing Java breaks these bindings.

Comment 31

[Peter "jag" Annema]

Well, those keybindings actually working is pure luck :-) (We still do the wrong
thing and pass values >= 128 on to isprint).

Apparantly installing Java changes your luck ;-)

Comment 32

[Asa Dotzler [:asa]]

does this really need a vtrunk.  Jag, was this handled differently on teh trunk?
is there another bug?

Comment 33

[Peter "jag" Annema]

Trunk has a different fix (53667). Removing keyword.

Comment 34

[John Morrison]

... so then, verified. Ta.

Comment 35

[Keyser Sose]

*** Bug 59812 has been marked as a duplicate of this bug. ***