Bug 210314 – Fix bad tests in testmasm's testCagePreservesPACFailureBit().

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Some of these tests will crash when validated untagging is enabled.

[Radar WebKit Bug Importer]

<rdar://problem/61556785>

[Mark Lam]

Created attachment 396032 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 396032 [details]
proposed patch.

r=me

[Mark Lam]

Thanks for the review.  Landed in r259848: <http://trac.webkit.org/r259848>.

[Keith Miller]

Comment on attachment 396032 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review

> Source/JavaScriptCore/assembler/testmasm.cpp:2136
> +        CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), ptr);

This isn't quite a strong as the old test. It doesn't guarantee that the cage gave you a pointer that will crash when dereferenced. However, I think that can be tested by also asserting, 

CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), removeArrayPtrTag(invoke<void*>(cage, taggedNotCagedPtr, 1))));

[Mark Lam]

Landed the additional test in r259890: <http://trac.webkit.org/r259890>.

[Saam Barati]

Comment on attachment 396032 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        Some of these tests will crash when validated untagging is enabled.

why?

[Saam Barati]

Comment on attachment 396032 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review

>> Source/JavaScriptCore/ChangeLog:9
>> +        Some of these tests will crash when validated untagging is enabled.
> 
> why?

(You should say that here, not just what, but why)

[Mark Lam]

(In reply to Saam Barati from comment #8)
> Comment on attachment 396032 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396032&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:9
> >> +        Some of these tests will crash when validated untagging is enabled.
> > 
> > why?
> 
> (You should say that here, not just what, but why)

It's in the title: "when validated untagging is enabled".  When we start validating the untagged pointer (as in crash if validation fails), we will crash if we are untagging badly tagged pointers on purpose in the test.

[Mark Lam]

(In reply to Mark Lam from comment #9)
> (In reply to Saam Barati from comment #8)
> > Comment on attachment 396032 [details]
> > proposed patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=396032&action=review
> > 
> > >> Source/JavaScriptCore/ChangeLog:9
> > >> +        Some of these tests will crash when validated untagging is enabled.
> > > 
> > > why?
> > 
> > (You should say that here, not just what, but why)
> 
> It's in the title: "when validated untagging is enabled".  When we start
> validating the untagged pointer (as in crash if validation fails), we will
> crash if we are untagging badly tagged pointers on purpose in the test.

Sorry, not title.  It's in the bug and ChangeLog description.

[Saam Barati]

Comment on attachment 396032 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review

>> Source/JavaScriptCore/assembler/testmasm.cpp:2136
>> +        CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), ptr);
> 
> This isn't quite a strong as the old test. It doesn't guarantee that the cage gave you a pointer that will crash when dereferenced. However, I think that can be tested by also asserting, 
> 
> CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), removeArrayPtrTag(invoke<void*>(cage, taggedNotCagedPtr, 1))));

can't we just assert using some functions that we're looking at unmapped memory?

[Keith Miller]

Comment on attachment 396032 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review

>>> Source/JavaScriptCore/assembler/testmasm.cpp:2136
>>> +        CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), ptr);
>> 
>> This isn't quite a strong as the old test. It doesn't guarantee that the cage gave you a pointer that will crash when dereferenced. However, I think that can be tested by also asserting, 
>> 
>> CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), removeArrayPtrTag(invoke<void*>(cage, taggedNotCagedPtr, 1))));
> 
> can't we just assert using some functions that we're looking at unmapped memory?

The point is to ensure you have PAC bits.