Bug 240264 – Add optional Integrity checks at JSC API boundaries.

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

[Mark Lam]

This will help us detect if bad values are passed across the API boundary.

[Mark Lam]

Created attachment 459095 [details]
proposed patch with ENABLE(EXTRA_INTEGRITY_CHECKS) on.

[Mark Lam]

Comment on attachment 459095 [details]
proposed patch with ENABLE(EXTRA_INTEGRITY_CHECKS) on.

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

> Source/JavaScriptCore/tools/Integrity.h:36
> +#define ENABLE_EXTRA_INTEGRITY_CHECKS 1

This is deliberately left enabled so that we can test this on the EWS.  Will disable before landing.

[Mark Lam]

Created attachment 459096 [details]
proposed patch w/ ENABLE(EXTRA_INTEGRITY_CHECKS) on.

[Mark Lam]

Created attachment 459097 [details]
proposed patch.

[Mark Lam]

Created attachment 459098 [details]
proposed patch w/ extra audits enabled for EWS testing only.

[Mark Lam]

Created attachment 459123 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 459123 [details]
proposed patch.

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:22
> +        4. Moved isSanePointer() to Integrity.h so that it can be used in more places.

Should we put it under `Integrity` namespace, what do you think of?

> Source/JavaScriptCore/ChangeLog:48
> +        11. Also strengten Integrity::auditStructureID() so that it will check if a
> +            Structure's memory has been released.

Can we split these lists into two sets of lists? They are based on,

1. features enabled only when ENABLE(EXTRA_INTEGRITY_CHECKS) is true
2. others

e.g.

The features enabled with ENABLE(EXTRA_INTEGRITY_CHECKS) are,

1. ...
2. ...
3. ..

> Source/JavaScriptCore/runtime/VM.cpp:410
> +    WTF::compilerFence();

Let's use `storeStoreFence()` to ensure that all the above stores are effective when we set m_isInService.

> Source/JavaScriptCore/runtime/VM.cpp:435
> +    WTF::compilerFence();

Use storeStoreFence to ensure that when m_isInService is true, we are not starting the following destruction operations.

> Source/JavaScriptCore/tools/Integrity.h:41
> +#if USE(JSVALUE32)
> +#define ENABLE_EXTRA_INTEGRITY_CHECKS 0 // Not supported.
> +#else
> +// Force ENABLE_EXTRA_INTEGRITY_CHECKS to 1 for your local build if you want
> +// more prolific audits to be enabled.
> +#define ENABLE_EXTRA_INTEGRITY_CHECKS 0
> +#endif

What do you think about enabling it for debug builds? Is it too costly?

[Mark Lam]

Comment on attachment 459123 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/ChangeLog:22
>> +        4. Moved isSanePointer() to Integrity.h so that it can be used in more places.
> 
> Should we put it under `Integrity` namespace, what do you think of?

OK, I moved it into the Integrity namespace.

>> Source/JavaScriptCore/ChangeLog:48
>> +            Structure's memory has been released.
> 
> Can we split these lists into two sets of lists? They are based on,
> 
> 1. features enabled only when ENABLE(EXTRA_INTEGRITY_CHECKS) is true
> 2. others
> 
> e.g.
> 
> The features enabled with ENABLE(EXTRA_INTEGRITY_CHECKS) are,
> 
> 1. ...
> 2. ...
> 3. ..

Most of this patch is adding support code that is always built in.  The only code dependent on ENABLE(EXTRA_INTEGRITY_CHECKS) are the audit calls added at the API boundary.  Those are no-ops when ENABLE(EXTRA_INTEGRITY_CHECKS) is disabled.

The other change is that I added back a check to load from Structure memory (to test readability) in auditStructureID.  This used to be enabled on Release builds as well, but was removed when the new StructureID management system was installed.  My patch adds back this check but only for Debug builds and also for when ENABLE(EXTRA_INTEGRITY_CHECKS) is enabled.

I've updated the ChangeLog to try to make this clearer.

>> Source/JavaScriptCore/runtime/VM.cpp:410
>> +    WTF::compilerFence();
> 
> Let's use `storeStoreFence()` to ensure that all the above stores are effective when we set m_isInService.

Fixed.

>> Source/JavaScriptCore/runtime/VM.cpp:435
>> +    WTF::compilerFence();
> 
> Use storeStoreFence to ensure that when m_isInService is true, we are not starting the following destruction operations.

Fixed.

>> Source/JavaScriptCore/tools/Integrity.h:41
>> +#endif
> 
> What do you think about enabling it for debug builds? Is it too costly?

I'll land this patch with this disabled, and investigate enabling this always on Debug builds in a later patch.

[Mark Lam]

Created attachment 459134 [details]
patch for landing.

[Mark Lam]

Landed in r294017: <http://trac.webkit.org/r294017>.

[Radar WebKit Bug Importer]

<rdar://problem/93056353>