segfault on control-VK_LEFT / alt-VK_LEFT

VERIFIED FIXED in Future

Status

()

P1
normal
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: axel, Assigned: jag+mozbugs)

Tracking

({crash})

Trunk
Future
Sun
Linux
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++] reviewed, approved, suntrak-n6)

Attachments

(2 attachments)

(Reporter)

Description

19 years ago
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
(Reporter)

Comment 1

19 years ago
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

19 years ago
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.
Status: NEW → ASSIGNED
(Reporter)

Comment 3

19 years ago
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
Keywords: crash, rtm

Comment 4

19 years ago
Adding Rich to CC.

Updated

19 years ago
Priority: P3 → P2
Whiteboard: SIMPLE FIX IN HAND

Comment 5

19 years ago
Created attachment 15854 [details] [diff] [review]
Patch: Add some bounds checking
(Reporter)

Comment 6

19 years ago
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

19 years ago
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. :-)
Whiteboard: SIMPLE FIX IN HAND → NEED SOLARIS HELP
(Reporter)

Comment 8

19 years ago
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

18 years ago
per comment from pdt team in another bug, solaris is a lower priority for rtm, 
this will have to wait
Whiteboard: NEED SOLARIS HELP → NEED SOLARIS HELP[rtm-]
Target Milestone: --- → Future

Updated

18 years ago
Whiteboard: NEED SOLARIS HELP[rtm-] → NEED SOLARIS HELP[rtm-] suntrak-n6

Comment 10

18 years ago
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.
Priority: P2 → P1
(Reporter)

Comment 11

18 years ago
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
(Assignee)

Comment 12

18 years ago
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.
(Assignee)

Comment 13

18 years ago
I just checked my patch in on the trunk.
(Assignee)

Comment 14

18 years ago
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++.
Assignee: akkana → disttsc
Status: ASSIGNED → NEW
OS: Solaris → Linux
Summary: segfault on control-VK_LEFT → segfault on control-VK_LEFT / alt-VK_LEFT
Whiteboard: NEED SOLARIS HELP[rtm-] suntrak-n6 → [rtm+] Fix on trunk, reviewed, approved (in bug 53667), suntrak-n6

Comment 15

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

Comment 16

18 years ago
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

18 years ago
rtm++
Whiteboard: [rtm+] Fix on trunk, reviewed, approved (in bug 53667), suntrak-n6 → [rtm++] Fix on trunk, reviewed, approved (in bug 53667), suntrak-n6

Comment 18

18 years ago
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?
Whiteboard: [rtm++] Fix on trunk, reviewed, approved (in bug 53667), suntrak-n6 → [rtm need info] Fix on trunk, reviewed, approved (in bug 53667), suntrak-n6

Comment 19

18 years ago
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].
(Assignee)

Comment 20

18 years ago
Created attachment 17671 [details] [diff] [review]
[patch] crash fix

Comment 21

18 years ago
Looks like we need code reviewers on this before it can be rtm++
(Assignee)

Comment 22

18 years ago
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

18 years ago
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.
Small fix, "provably" zero additional risk, my favourite kind: sr=shaver.
(Assignee)

Comment 25

18 years ago
rtm+ again for the second patch.
Whiteboard: [rtm need info] Fix on trunk, reviewed, approved (in bug 53667), suntrak-n6 → [rtm+] reviewed, approved, suntrak-n6
Do we need to make sure the character is in [0,127], per bug 56777?

/be
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

18 years ago
rtm++
Whiteboard: [rtm+] reviewed, approved, suntrak-n6 → [rtm++] reviewed, approved, suntrak-n6
(Assignee)

Comment 29

18 years ago
Checked in on branch, trunk has a different fix already, marking fixed.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 30

18 years ago
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.
Keywords: vtrunk
(Assignee)

Comment 31

18 years ago
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 ;-)
does this really need a vtrunk.  Jag, was this handled differently on teh trunk?
is there another bug?
(Assignee)

Comment 33

18 years ago
Trunk has a different fix (53667). Removing keyword.
Keywords: vtrunk

Comment 34

18 years ago
... so then, verified. Ta.
Status: RESOLVED → VERIFIED

Comment 35

18 years ago
*** Bug 59812 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.