Closed Bug 54694 Opened 24 years ago Closed 24 years ago

segfault on control-VK_LEFT / alt-VK_LEFT

Categories

(Core :: XUL, defect, P1)

Product:
Core
Component:
XUL
Platform:
Sun
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Future

People

(Reporter: axel, Assigned: jag+mozbugs)

References

Details

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

Attachments

(2 files)

Patch: Add some bounds checking
508 bytes, patch
Details | Diff | Splinter Review
[patch] crash fix
561 bytes, patch
Details | Diff | Splinter Review 
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
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
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
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
Adding Rich to CC.
Priority: P3 → P2
Whiteboard: SIMPLE FIX IN HAND
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
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
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
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
Whiteboard: NEED SOLARIS HELP[rtm-] → NEED SOLARIS HELP[rtm-] suntrak-n6
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
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
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.
I just checked my patch in on the trunk.
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
*** Bug 54884 has been marked as a duplicate of this bug. ***
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.
rtm++
Whiteboard: [rtm+] Fix on trunk, reviewed, approved (in bug 53667), suntrak-n6 → [rtm++] Fix on trunk, reviewed, approved (in bug 53667), suntrak-n6
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
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].

    
    

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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
rtm++

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

    
    

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

    
    

    
  
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

    
    

    
  
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?

    
    

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

    
    

    
  
... so then, verified. Ta.
Status: RESOLVED → VERIFIED

    
    

    
  
*** 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: