944037 - GDB magic for Asm.js SIGSEGV should probably not call malloc from signal handlers

Status: REOPENED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

As mentioned in bug 906860 comment 13, it might not be great to call malloc from a signal handler. Today I ran into a case where malloc itself segfaulted, so js-gdb.gdb's magic hangs GDB while malloc tries to acquire the lock it's already holding.

Also, the posthook-run hook seems weird; I don't think it's a good idea to try to free in the new process the $sigaction buffer we allocated in the previous process. (Patch was r=me; in situations like this I think Brendan makes a reference to some hapless bottle inspector in the Simpsons who marks as "PASS" a bottle that has a dead mouse in it.)

Comment 1

[Jim Blandy :jimb]

Attachment: Don't allocate AsmJS SIGSEGV catcher's sigaction buffer at signal delivery time.

Here's an alternative I'm playing with.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8339470 [details] [diff] [review]
Don't allocate AsmJS SIGSEGV catcher's sigaction buffer at signal delivery time.

Review of attachment 8339470 [details] [diff] [review]:
-----------------------------------------------------------------

That is gross and awful and better than the lazy malloc. And I didn't know 'commands' would default to the last breakpoint set! That enables some nasty hacks that I attempted in the past and failed.

It ought to use the full name 'commands' in both places, though.

I'll note that while gdb knows about struct sigaction for the js shell binary, it does not for arbitrary binaries (eg gdb /bin/ls). I don't know where the definition comes from, nor how to ask gdb. It seems like both the browser and JS shell binaries should be fine with it. I think you might want to qualify it as struct ::sigaction, just in case. (In the signal handler section, anyway, but probably in the main() breakpoint commands for consistency.)

Comment 3

[Luke Wagner [:luke]]

Note: with the JS_DISABLE_SLOW_SCRIPT_SIGNALS environment variable, the gdb hook is less necessary than before; it might be ok just to remove it.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8339470 [details] [diff] [review]
Don't allocate AsmJS SIGSEGV catcher's sigaction buffer at signal delivery time.

Review of attachment 8339470 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry that you hit it. Actually, efaust and nbp had thought that when it would happen, it would create a deadlock and / or segfault gdb (I remember efaust saying "I wouldn't do that, even to my worst enemy"). We all reacted like "it'll be fine until somebody will experience the issue, then they'll find a better way". Anyways, malloc'ing the struct after main means that every time we re-run the program in gdb, we leak the size of the struct, right? But nowadays, all computers have way too more memory, and if you still don't have enough RAM, you can download some on the Internet [0], so that seems fine. At least, better than the malloc deadlock thing.

[0] http://www.downloadmoreram.com/download.html

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8339470 [details] [diff] [review]
Don't allocate AsmJS SIGSEGV catcher's sigaction buffer at signal delivery time.

Review of attachment 8339470 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/js-gdb.gdb
@@ +1,5 @@
> +break main
> +command
> +    set $sigaction = malloc(sizeof(sigaction))
> +    echo Allocated IonMonkey sigaction buffer for GDB; continuing.\n
> +    continue

What happens if a developer set a breakpoint on main?  Shouldn't we take another symbol such as _start ?
Is there a way to hide this breakpoint, in a similar way as we can do in python, because this breakpoint will appear in the list of breakpoints when we run "info breakpoints", and might be disabled / deleted.

Comment 6

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.