Bug 202085 – Introducing Integrity audit functions.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

These will be used later to help debug memory corruption bugs.

[Mark Lam]

Created attachment 379340 [details]
proposed patch.

Let's get some EWS testing first.

[Mark Lam]

Comment on attachment 379340 [details]
proposed patch.

Ready for a review.

[Keith Miller]

Comment on attachment 379340 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:29
> +           Random audits are triggered by the m_triggerBits bits in VM::m_integrityRandom.
> +           m_triggerBits is a 64-bit bitfield.

Can you specify where auditing happens. It looks like it happens during GC on whatever thread is assisting with GC at the time.

> Source/JavaScriptCore/tools/Integrity.cpp:64
> +    return vm.random().getUint32() >= threshold;

Shouldn't this be: vm.random().getUint32() <= threshold?

> Source/JavaScriptCore/tools/Integrity.h:64
> +    static constexpr int numberOfTriggerBits = (sizeof(m_triggerBits) * 8) - 1;

Can we make this: (sizeof(m_triggerBits) * CHAR_BITS) - 1?

> Source/JavaScriptCore/tools/VMInspectorInlines.h:39
> +#define __CONDITION(x) (x), #x
> +#define __VERIFY(action, verifier, cond, ...) do { \

Nit: Why are these prefixed with __? That's not usually how we format our macros.

> Source/JavaScriptCore/tools/VMInspectorInlines.h:74
> +        if (cellType == BigIntType) {
> +            auto* bigInt = jsCast<JSBigInt*>(cell);
> +            cellSize = JSBigInt::allocationSize(bigInt->length());
> +        } else if (cellType == DirectArgumentsType) {
> +            auto* args = jsCast<DirectArguments*>(cell);
> +            cellSize = DirectArguments::allocationSize(args->m_minCapacity);
> +        } else if (cellType == FinalObjectType)
> +            cellSize = JSFinalObject::allocationSize(structure->inlineCapacity());
> +        else if (cellType == LexicalEnvironmentType) {
> +            auto* env = jsCast<JSLexicalEnvironment*>(cell);
> +            cellSize = JSLexicalEnvironment::allocationSize(env->symbolTable());
> +        } else if (cellType == ModuleEnvironmentType) {
> +            auto* env = jsCast<JSModuleEnvironment*>(cell);
> +            cellSize = JSModuleEnvironment::allocationSize(env->symbolTable());
> +        } else if (cellType == ModuleNamespaceObjectType) {
> +            auto* obj = jsCast<JSModuleNamespaceObject*>(cell);
> +            cellSize = JSModuleNamespaceObject::allocationSize(obj->m_names.capacity());
> +        }

why not make this a switch? Seems like you also want to have a RELEASE_ASSERT_NOT_REACHED() in the default case?

[Mark Lam]

Comment on attachment 379340 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:29
>> +           m_triggerBits is a 64-bit bitfield.
> 
> Can you specify where auditing happens. It looks like it happens during GC on whatever thread is assisting with GC at the time.

I can add a general comment about where to deploy this but that's not what this patch is about.  In this patch, I'm introducing the auditing tool.  I added 1 single use of it in SlotVisitor as an example.  See https://bugs.webkit.org/show_bug.cgi?id=202060 for other (though not complete) places I would deploy this.  That patch was getting humungous.  So, I decided to post this part for review separately so that it can get better attention in a review.

>> Source/JavaScriptCore/tools/Integrity.cpp:64
>> +    return vm.random().getUint32() >= threshold;
> 
> Shouldn't this be: vm.random().getUint32() <= threshold?

You are correct.  Will fix.

>> Source/JavaScriptCore/tools/Integrity.h:64
>> +    static constexpr int numberOfTriggerBits = (sizeof(m_triggerBits) * 8) - 1;
> 
> Can we make this: (sizeof(m_triggerBits) * CHAR_BITS) - 1?

Will fix.

>> Source/JavaScriptCore/tools/VMInspectorInlines.h:39
>> +#define __VERIFY(action, verifier, cond, ...) do { \
> 
> Nit: Why are these prefixed with __? That's not usually how we format our macros.

I wanted short names but don't want it to collide with what other headers may use.  Also, these are used locally only unlike other macros that we use across many files.  I'll rename these AUDIT_CONDITION and AUDIT_VERIFY.

>> Source/JavaScriptCore/tools/VMInspectorInlines.h:74
>> +        }
> 
> why not make this a switch? Seems like you also want to have a RELEASE_ASSERT_NOT_REACHED() in the default case?

Sure, why not.  This started out being a small list, but it grew and grew and grew.  I'll switch to the switch.

[Mark Lam]

Created attachment 379348 [details]
proposed patch.

[Saam Barati]

Everything is red

[Mark Lam]

Created attachment 379383 [details]
proposed patch.

[Saam Barati]

Comment on attachment 379383 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:9
> +        be used wherever we want to audit a cell to ensure it is not corrupted.  However,

"ensure" => "probabilistically ensure"?

I think the most interesting part of this patch is how you plan to audit a cell to see if it's corrupted, but you don't discuss that at all in this changelog

> Source/JavaScriptCore/ChangeLog:12
> +        example use in SlotVisitor.  We'll follow up later with more patches to deploy
> +        this tool throughout the VM.

How are you planning on deploying this? Is there somewhere you document your plans?

> Source/JavaScriptCore/ChangeLog:17
> +               Minimal - do a minimal quick audit (minimize perf impact).

why have this?

> Source/JavaScriptCore/ChangeLog:83
> +           by re-seeding it with a cryptographically random number each GC.

why? If anything, it seems way more useful to be able to seed your RNG with a constant. (For example, in our fuzzers, we want fixed random numbers to be able to reproduce crashes more easily.)

> Source/JavaScriptCore/heap/SlotVisitor.cpp:225
>          validateCell(jsCell);
> +        Integrity::auditCell(vm(), jsCell);

why both auditCell and validateCell?

> Source/JavaScriptCore/runtime/ClassInfo.h:225
> +    unsigned cppSize;

nit: not a fan of this name. These all sound better to me:
classSize
staticClassSize

> Source/JavaScriptCore/runtime/OptionsList.h:347
> +    v(Double, randomIntegrityAuditRate, 0.05, Normal, "Probability of random integrity audits (0.0 - 1.0)") \

"(0.0 - 1.0)" => "[0.0 - 1.0]"

> Source/JavaScriptCore/tools/Integrity.cpp:41
> +    reloadAndCheckShouldAuditSlow(vm); // This will initialize the audit trigger bits.

nit: no need for this comment

> Source/JavaScriptCore/tools/Integrity.cpp:54
> +    // Reload the trigger bits.
> +    m_triggerBits = 1ull << 63; // Set the terminator bit (i.e. "need reload") in the high bit.

these comments aren't helpful

> Source/JavaScriptCore/tools/Integrity.cpp:58
> +        bool trigger = (vm.random().getUint32() <= threshold);

style nit: not need for parens

> Source/JavaScriptCore/tools/Integrity.h:48
> +static constexpr AuditLevel DefaultAuditLevel = AuditLevel::None;
> +#else
> +static constexpr AuditLevel DefaultAuditLevel = AuditLevel::Random;

nit: I think the "D" in "DefaultAuditLevel" should be lower case, right?

> Source/JavaScriptCore/tools/Integrity.h:86
> +    if (auditLevel == AuditLevel::None)
> +        return;
> +
> +    if (auditLevel == AuditLevel::Minimal)
> +        auditCellMinimally(vm, cell);
> +    else if (auditLevel == AuditLevel::Full)
> +        auditCellFully(vm, cell);
> +    else if (auditLevel == AuditLevel::Random)
> +        auditCellRandomly(vm, cell);

nit: why not switch statement here so compiler will warn you when you don't implement a new enum type in future?

> Source/JavaScriptCore/tools/VMInspector.h:89
> +    static bool verifyCellSize(VM&, JSCell*, Optional<size_t> allocatorCellSize = { });

why optional? You always pass in non null

[Mark Lam]

Comment on attachment 379383 [details]
proposed patch.

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

z

>> Source/JavaScriptCore/ChangeLog:9
>> +        be used wherever we want to audit a cell to ensure it is not corrupted.  However,
> 
> "ensure" => "probabilistically ensure"?
> 
> I think the most interesting part of this patch is how you plan to audit a cell to see if it's corrupted, but you don't discuss that at all in this changelog

I've made the text change.

I'll add something below to describe what I do in the full audit.

>> Source/JavaScriptCore/ChangeLog:12
>> +        this tool throughout the VM.
> 
> How are you planning on deploying this? Is there somewhere you document your plans?

https://bugs.webkit.org/show_bug.cgi?id=202060 is a first attempt at this, which is how I realized that the patch was going to be too large.  So, I broke out the auditing mechanism out into this patch.

>> Source/JavaScriptCore/ChangeLog:17
>> +               Minimal - do a minimal quick audit (minimize perf impact).
> 
> why have this?

I plan to experiment with possibly turning this on always for release builds later.  I included the option here so that it's easier to experiment with.  If this turns out to be non-viable, I can remove it later.  If you strongly prefer that I leave this part out until we have a definitive use, I can remove it.

>> Source/JavaScriptCore/ChangeLog:83
>> +           by re-seeding it with a cryptographically random number each GC.
> 
> why? If anything, it seems way more useful to be able to seed your RNG with a constant. (For example, in our fuzzers, we want fixed random numbers to be able to reproduce crashes more easily.)

Because in the real world, when not fuzzing, we actually want random things to be really random.  The only reason for using WeakRandom is for speed.  Doing this adds more random-ness.  When we add a test mode to make random numbers reproducible later, we can add code to disable this.

>> Source/JavaScriptCore/heap/SlotVisitor.cpp:225
>> +        Integrity::auditCell(vm(), jsCell);
> 
> why both auditCell and validateCell?

validateCell() is pre-existing code that always run even in release mode.  I can certainly consolidate it.  Can we leave that for a follow up exercise?

>> Source/JavaScriptCore/runtime/ClassInfo.h:225
>> +    unsigned cppSize;
> 
> nit: not a fan of this name. These all sound better to me:
> classSize
> staticClassSize

Sure, I'll replace with staticClassSize.

>> Source/JavaScriptCore/runtime/OptionsList.h:347
>> +    v(Double, randomIntegrityAuditRate, 0.05, Normal, "Probability of random integrity audits (0.0 - 1.0)") \
> 
> "(0.0 - 1.0)" => "[0.0 - 1.0]"

Thanks.  Fixed.

>> Source/JavaScriptCore/tools/Integrity.cpp:41
>> +    reloadAndCheckShouldAuditSlow(vm); // This will initialize the audit trigger bits.
> 
> nit: no need for this comment

Removed.

>> Source/JavaScriptCore/tools/Integrity.cpp:54
>> +    m_triggerBits = 1ull << 63; // Set the terminator bit (i.e. "need reload") in the high bit.
> 
> these comments aren't helpful

Removed.

>> Source/JavaScriptCore/tools/Integrity.cpp:58
>> +        bool trigger = (vm.random().getUint32() <= threshold);
> 
> style nit: not need for parens

Removed.

>> Source/JavaScriptCore/tools/Integrity.h:48
>> +static constexpr AuditLevel DefaultAuditLevel = AuditLevel::Random;
> 
> nit: I think the "D" in "DefaultAuditLevel" should be lower case, right?

I always feel conflicted about this.  Constants (e.g in enums) should be upper case.  Variables should be lower case.  Here we have a variable that is not really a variable, and a constant that is not an enum.  The initialization here looks wrong, but its use below ...

    template<AuditLevel auditLevel = DefaultAuditLevel>

... looks correct.  In contrast, ...

    template<AuditLevel auditLevel = defaultAuditLevel>

... looks wrong.  How strongly do you feel about this?  I don't think there's a official style guide rule about this yet.

>> Source/JavaScriptCore/tools/Integrity.h:86
>> +        auditCellRandomly(vm, cell);
> 
> nit: why not switch statement here so compiler will warn you when you don't implement a new enum type in future?

Sure.  Fixed.

>> Source/JavaScriptCore/tools/VMInspector.h:89
>> +    static bool verifyCellSize(VM&, JSCell*, Optional<size_t> allocatorCellSize = { });
> 
> why optional? You always pass in non null

I started out with this as a size_t optionalAllocatorCellSize = 0, but thought that using Optional documents the code better.  This code isn't exactly in the fast path, plus it's a template.  I figure the compiler will optimize it all away.

[Mark Lam]

Created attachment 379412 [details]
proposed patch.

[Mark Lam]

(In reply to Mark Lam from comment #9)
> >> Source/JavaScriptCore/ChangeLog:83
> >> +           by re-seeding it with a cryptographically random number each GC.
> > 
> > why? If anything, it seems way more useful to be able to seed your RNG with a constant. (For example, in our fuzzers, we want fixed random numbers to be able to reproduce crashes more easily.)
> 
> Because in the real world, when not fuzzing, we actually want random things
> to be really random.  The only reason for using WeakRandom is for speed. 
> Doing this adds more random-ness.  When we add a test mode to make random
> numbers reproducible later, we can add code to disable this.

Saam clarified that I need to have some affordance here to turn off randomness for fuzzing.  Will fix.

> >> Source/JavaScriptCore/tools/VMInspector.h:89
> >> +    static bool verifyCellSize(VM&, JSCell*, Optional<size_t> allocatorCellSize = { });
> > 
> > why optional? You always pass in non null
> 
> I started out with this as a size_t optionalAllocatorCellSize = 0, but
> thought that using Optional documents the code better.  This code isn't
> exactly in the fast path, plus it's a template.  I figure the compiler will
> optimize it all away.

Saam convinced me that I don't need this optional.  It's removed.

I'll also fix Random::shouldAudit() to be safe to call concurrently because SlotVisitor::appendJSCellOrAuxiliary() can be called from more than 1 thread at the same time.

[Mark Lam]

Created attachment 379429 [details]
proposed patch.

[Saam Barati]

Comment on attachment 379383 [details]
proposed patch.

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

>>> Source/JavaScriptCore/tools/Integrity.h:48
>>> +static constexpr AuditLevel DefaultAuditLevel = AuditLevel::Random;
>> 
>> nit: I think the "D" in "DefaultAuditLevel" should be lower case, right?
> 
> I always feel conflicted about this.  Constants (e.g in enums) should be upper case.  Variables should be lower case.  Here we have a variable that is not really a variable, and a constant that is not an enum.  The initialization here looks wrong, but its use below ...
> 
>     template<AuditLevel auditLevel = DefaultAuditLevel>
> 
> ... looks correct.  In contrast, ...
> 
>     template<AuditLevel auditLevel = defaultAuditLevel>
> 
> ... looks wrong.  How strongly do you feel about this?  I don't think there's a official style guide rule about this yet.

It doesn't bother me much either way, I just thought our style guide said it should be lower case. If there is no style rule, I don't have a personal preference

[Saam Barati]

Comment on attachment 379429 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/heap/Heap.cpp:1273
> +        vm().random().setSeed(cryptographicallyRandomNumber()); // Piggy back on the GC to randomize a bit.

nit: not a fan of comments that just says what the code is doing

> Source/JavaScriptCore/tools/VMInspectorInlines.h:39
> +#undef AUDIT_CONDITION
> +#undef AUDIT_VERIFY

why? Seems slightly gross, and shouldn't be needed

> Source/JavaScriptCore/tools/VMInspectorInlines.h:56
> +    if (isDynamicallySizedType(cellType)) {

nit: seems like this should be a helper that exists outside of VMInspector, something like:

size_t cellSize(JSCell*);

It can then branch on isDynamicallySizedType or look in classinfo.

> Source/JavaScriptCore/tools/VMInspectorInlines.h:90
> +        AUDIT_VERIFY(action, verifier, cellSize <= allocatorCellSize, cell, cellType, cellSize, allocatorCellSize);

might be nice to also assert the dynamically sized cell size is >= class info's cell size

[Mark Lam]

Comment on attachment 379429 [details]
proposed patch.

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

>> Source/JavaScriptCore/heap/Heap.cpp:1273
>> +        vm().random().setSeed(cryptographicallyRandomNumber()); // Piggy back on the GC to randomize a bit.
> 
> nit: not a fan of comments that just says what the code is doing

Will remove.

>> Source/JavaScriptCore/tools/VMInspectorInlines.h:39
>> +#undef AUDIT_VERIFY
> 
> why? Seems slightly gross, and shouldn't be needed

Will remove.

>> Source/JavaScriptCore/tools/VMInspectorInlines.h:56
>> +    if (isDynamicallySizedType(cellType)) {
> 
> nit: seems like this should be a helper that exists outside of VMInspector, something like:
> 
> size_t cellSize(JSCell*);
> 
> It can then branch on isDynamicallySizedType or look in classinfo.

Sounds good.  I'll refactor this in a follow up patch.

>> Source/JavaScriptCore/tools/VMInspectorInlines.h:90
>> +        AUDIT_VERIFY(action, verifier, cellSize <= allocatorCellSize, cell, cellType, cellSize, allocatorCellSize);
> 
> might be nice to also assert the dynamically sized cell size is >= class info's cell size

Will add.

[Mark Lam]

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

[Radar WebKit Bug Importer]

<rdar://problem/55651198>