563610 - TestPoisonArea freezes on executing poison area on some arm linux

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Check that madvise's errno is not ENOMEM in ProbeRegion

On ARM linux, when TestPoisonArea calls the poison area as a function, what happens is that the program is stuck with its instruction pointer on the poison area, but no segmentation fault occurs, and worse, no instruction is executed at all, making TestPoisonArea freeze.

This happens because the preferred addr is outside the boundaries allowed for userspace programs on that platform, apparently.

I think that instead of assuming any madvise error would make the preferred addr good to be used, ENOMEM should be special cased. That would make TestPoisonArea stop freezing, but use the "consolation prize".

A step further would probably be to define another preferred address in case the first one fails the ProbeRegion test, if possible, but that could be another bug.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 443312 [details] [diff] [review]
Check that madvise's errno is not ENOMEM in ProbeRegion

My understanding of this test is that the point of it is to test the actual code we're using in nsPresArena.cpp, some of which had to be copied into the test.  Testing different code seems substantially less useful.  (If Zack disagrees, though, I might reconsider.)

Also, local style is probably to brace single-statement if bodies.

Comment 2

[Mike Hommey [:glandium]]

(In reply to comment #1)
> (From update of attachment 443312 [details] [diff] [review])
> My understanding of this test is that the point of it is to test the actual
> code we're using in nsPresArena.cpp, some of which had to be copied into the
> test.  Testing different code seems substantially less useful.  (If Zack
> disagrees, though, I might reconsider.)

I figured that afterwards, and forgot to remove the review? flag for you. I'm still interested in Zack's opinion, though.

> Also, local style is probably to brace single-statement if bodies.

Would that be for both levels ?

Comment 3

[Zack Weinberg (:zwol)]

David is correct.  (The code should really be unified, but I never seem to get around to it.)  If the test hangs on jumping into the poison area, the browser is also going to hang on jumping into the poison area, which is not the UX we want for a crash.

Also, special-casing ENOMEM seems like a really poor proxy for the thing we're actually trying to test, and on most hardware an address outside the user-usable range *does* cause a predictable segfault and is actually preferred to a "consolation prize" address that could conceivably become accessible memory due to some other bug.  I'm inclined to call this a bug in the kernel, honestly.

Comment 4

[Mike Hommey [:glandium]]

(In reply to comment #3)
> I'm inclined to call this a bug in the kernel, honestly.

Bug in the kernel or not, there needs to be a solution, because freezing is definitely not something that would be nice for fennec, since, as you said, if that hangs in the test, it will hang in the browser. Would you have a better idea than special-casing ENOMEM ?

Comment 5

[Zack Weinberg (:zwol)]

The use of madvise there is a dirty hack because I couldn't find anything better.  As such, I have no better ideas :-/  But if you can find a more reliable way to identify an address that will predictably trigger a fatal signal if read, written, or jumped to, without actually doing the "attempt it and catch signals" thing that TestPoisonArea does (we'd rather not do that in the browser proper), we're all ears.

Comment 6

[Zack Weinberg (:zwol)]

I should also mention that we *believe* from code inspection that the browser will never directly branch to the poison area, so the execute test is less important than the other two.  But this is defensive code against exploits, so I'd rather not be relying on that belief if possible.

Comment 7

[Mike Hommey [:glandium]]

(In reply to comment #5)
> The use of madvise there is a dirty hack because I couldn't find anything
> better.  As such, I have no better ideas :-/  But if you can find a more
> reliable way to identify an address that will predictably trigger a fatal
> signal if read, written, or jumped to, without actually doing the "attempt it
> and catch signals" thing that TestPoisonArea does (we'd rather not do that in
> the browser proper), we're all ears.

On Linux, the only way I know is to parse /proc/self/maps or /proc/self/smaps. I don't think there is an API for that or something similar, though I would like to be proven wrong.

Comment 8

[Zack Weinberg (:zwol)]

Those files tell us about areas that *are currently* accessible in some way, but do not (appear to) indicate areas that *will never be* accessible, which is what we are looking for here.

Comment 9

[Mike Hommey [:glandium]]

It looks like it doesn't freeze everywhere. I have access to another machine where that doesn't. I'll try to narrow it down.

Comment 10

[Mike Hommey [:glandium]]

It looks like it was a kernel issue.