Bug 181542 – Replace all use of ConstExprPoisoned with Poisoned.

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

[Mark Lam]

Benchmarks show that perf is neutral even when using Poisoned instead of ConstExprPoisoned.  Since Poisoned is better security properties than ConstExprPoisoned, let's try to convert over to using Poisoned, and obsolete ConstExprPoisoned.

[Radar WebKit Bug Importer]

<rdar://problem/36442138>

[Mark Lam]

Created attachment 331071 [details]
work in progress for EWS testing only.

Let's do a viability test run on the EWS to make sure that we can actually do this on older compilers (which the EWS uses).

[Mark Lam]

Created attachment 331076 [details]
work in progress for EWS testing only.

[Mark Lam]

Created attachment 331130 [details]
test for EWS.

[Mark Lam]

Created attachment 331168 [details]
test for EWS.

[Mark Lam]

Created attachment 331169 [details]
test for EWS.

[Mark Lam]

Created attachment 331171 [details]
test for EWS.

[Mark Lam]

Created attachment 331173 [details]
test for EWS.

[Mark Lam]

Created attachment 331176 [details]
test for EWS.

[Mark Lam]

Created attachment 331177 [details]
test for EWS.

[Mark Lam]

Created attachment 331258 [details]
proposed patch.

[JF Bastien]

Comment on attachment 331258 [details]
proposed patch.

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

Some suggestions, none major, r=me

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:486
> +    unpoison(_g_CodeBlockPoison, scratch, scratch2)

You should paranoid-zero-out scratch2 here. There's a bunch more below. Maybe file a follow-up to do so?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:758
> +        xorp scratch, field

Or maybe this should just zero-out the poison? If we ever re-use it we can do so in a different macro.

> Source/JavaScriptCore/runtime/JSCPoison.cpp:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

-2018

> Source/JavaScriptCore/runtime/JSCPoison.cpp:35
> +uintptr_t g_nativeCodePoison;

I don't understand these. They're not used with this lowercase spelling in the code you're changing? Are they just stale?

> Source/JavaScriptCore/runtime/JSCPoison.cpp:2
> + * Copyright (C) 2017-2018 Apple Inc. All rights reserved.

Actually I don't get it, you're adding this file above *and* modifying it here? I think this patch has some borky-bork going on!

> Source/JavaScriptCore/runtime/JSCPoison.cpp:34
> +#define INSTANTIATE_POISON(poisonID) \

C++ pedant: this is "define" (to complement header file "declare").

> Source/JavaScriptCore/runtime/JSCPoison.h:58
> +#define POISON(_poisonID_) g_##_poisonID_##Poison

I like the style, but I'd name the macro POISON_FOR so it reads better when used.

> Source/JavaScriptCore/runtime/WriteBarrier.h:198
> +    static auto poison() { return Traits::poison(); }

Wooh! That was easy :D

> Source/WTF/wtf/Poisoned.h:69
> +enum PrePoisonedTag { PrePoisoned };

I'm not sure "pre" is super obvious. How about "AlreadyPoisoned"? Or "DontPoison"? No strong feeling on this, feel free to ignore.

> Source/WTF/wtf/Poisoned.h:91
> +    Poisoned(Poisoned&& other)

Can this be =default?

> Source/WTF/wtf/Poisoned.h:132
> +    bool operator>=(const Poisoned& b) const { return m_poisonedBits >= b.m_poisonedBits; }

Do we use the relational operators? I'd only add them if we do, it seems weird to order poisoned values (as opposed to just comparing them) but sometimes you gotta map stuff I guess. Seems like if you have them you'd want hash too (but I'm not sure you want that either!).

> Source/WTF/wtf/Poisoned.h:209
> +    template<uintptr_t&, typename, typename> friend class Poisoned;

Not sure I get why this is friending itself.

> Source/WTF/wtf/Poisoned.h:220
>  template<uintptr_t& key, typename T>

Shouldn't this be called poison instead of key? Since we don't do keys anymore. Here and a bunch of other places.

> Tools/TestWebKitAPI/Tests/WTF/PoisonedRef.cpp:48
> +        // Make sure we get 2 different poison values.

?

> Tools/TestWebKitAPI/Tests/WTF/PoisonedRefPtr.cpp:50
> +        // Make sure we get 2 different poison values.

?

> Tools/TestWebKitAPI/Tests/WTF/PoisonedUniquePtr.cpp:42
> +        // Make sure we get 2 different poison values.

This one does what it says :)

[Mark Lam]

Comment on attachment 331258 [details]
proposed patch.

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

Thanks for the review.  My responses below ...

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:486
>> +    unpoison(_g_CodeBlockPoison, scratch, scratch2)
> 
> You should paranoid-zero-out scratch2 here. There's a bunch more below. Maybe file a follow-up to do so?

I'll think about this some more and make a change in a follow up patch if needed.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:758
>> +        xorp scratch, field
> 
> Or maybe this should just zero-out the poison? If we ever re-use it we can do so in a different macro.

Ditto.  I'll think about it some more and follow up later if needed.

>>> Source/JavaScriptCore/runtime/JSCPoison.cpp:2
>>> + * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
>> 
>> -2018
> 
> Actually I don't get it, you're adding this file above *and* modifying it here? I think this patch has some borky-bork going on!

I think you're not used to how our bugzilla present patches.  The first JSCPoison.cpp above is because JSCPoisonedPtr.cpp got renamed to JSCPoison.cpp.  This 2nd version of JSCPoison.cpp that you're commenting on is showing the code changes I made after renaming the file.  If you re-read the patch with that in mind, then everything will make sense.

>> Source/JavaScriptCore/runtime/JSCPoison.cpp:34
>> +#define INSTANTIATE_POISON(poisonID) \
> 
> C++ pedant: this is "define" (to complement header file "declare").

Sure I can change this to be DEFINE_POISON instead of INSTANTIATE_POISON.

>> Source/JavaScriptCore/runtime/JSCPoison.cpp:35
>> +    uintptr_t POISON(poisonID);
> 
> I don't understand these. They're not used with this lowercase spelling in the code you're changing? Are they just stale?

See my comment above about how (our) bugzilla shows patches.  To answer your question, this is where I define (instantiate storage for) all the poison values.  The use of the POISON() macro is so that I don't have to type out g_##poisoneID##Poison.  The advantage of doing this is that the encoding of the variable name is only done in one place i.e. the POISON() macro.

>> Source/JavaScriptCore/runtime/JSCPoison.h:58
>> +#define POISON(_poisonID_) g_##_poisonID_##Poison
> 
> I like the style, but I'd name the macro POISON_FOR so it reads better when used.

I did consider POISON_FOR(), but I settled on POISON() because:
1. it is clear enough by inference that the POISON(CodeBlock) is obviously the POISON for CodeBlock.
2. see all the places where I have to use the POISON() macro in this patch (with more to come all over the code base as we poison more things).  POISON is more concise and less typing.  That's why I prefer it.

Do you feel strongly about using POISON_FOR instead of POISON?  If not, I prefer to keep it as POISON.

>> Source/WTF/wtf/Poisoned.h:69
>> +enum PrePoisonedTag { PrePoisoned };
> 
> I'm not sure "pre" is super obvious. How about "AlreadyPoisoned"? Or "DontPoison"? No strong feeling on this, feel free to ignore.

Hmmm, I like AlreadyPoisoned.  I'll apply it.

>> Source/WTF/wtf/Poisoned.h:91
>> +    Poisoned(Poisoned&& other)
> 
> Can this be =default?

Yes, I think so.  I'll apply it.

>> Source/WTF/wtf/Poisoned.h:132
>> +    bool operator>=(const Poisoned& b) const { return m_poisonedBits >= b.m_poisonedBits; }
> 
> Do we use the relational operators? I'd only add them if we do, it seems weird to order poisoned values (as opposed to just comparing them) but sometimes you gotta map stuff I guess. Seems like if you have them you'd want hash too (but I'm not sure you want that either!).

Ah yes, you are right.  It is wrong to compare the poisoned values.  I should either =delete them or change them to use unpoisoned values.  Since these are trivial to implement and I'd rather have an as complete as possible smart pointer class, I'll opt for fixing the relational operators.

>> Source/WTF/wtf/Poisoned.h:209
>> +    template<uintptr_t&, typename, typename> friend class Poisoned;
> 
> Not sure I get why this is friending itself.

It is friending other specializations of itself so that it can unpoison their pointers for work.  Specifically, this was motivated by line 175 in swap().

>> Source/WTF/wtf/Poisoned.h:220
>>  template<uintptr_t& key, typename T>
> 
> Shouldn't this be called poison instead of key? Since we don't do keys anymore. Here and a bunch of other places.

Yeah, key is a holdover from when Poisoned used to be called ScrambledPtr.  I'll do a follow up refactoring patch to rename key to poison in all the relevant places.

>> Tools/TestWebKitAPI/Tests/WTF/PoisonedRef.cpp:48
>> +        // Make sure we get 2 different poison values.
> 
> ?

Copy-paste error.  Will remove.

>> Tools/TestWebKitAPI/Tests/WTF/PoisonedRefPtr.cpp:50
>> +        // Make sure we get 2 different poison values.
> 
> ?

Will remove.

[Mark Lam]

Created attachment 331281 [details]
patch for landing.

[JF Bastien]

Comment on attachment 331281 [details]
patch for landing.

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

All looks good. Question on relational operators can be addressed later.

> Source/WTF/wtf/Poisoned.h:133
> +    bool operator>=(T b) const { return unpoisoned() >= b; }

What I meant is that I'd drop these entirely, and only keep == and !=, unless we actually use them. If we keep them, it may actually be better to compare the poisoned values because that doesn't leak any info about the underlying data, whereas comparing the underlying data definitely leaks it (albeit that's a super unlikely source of info).

[Mark Lam]

Comment on attachment 331281 [details]
patch for landing.

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

>> Source/WTF/wtf/Poisoned.h:133
>> +    bool operator>=(T b) const { return unpoisoned() >= b; }
> 
> What I meant is that I'd drop these entirely, and only keep == and !=, unless we actually use them. If we keep them, it may actually be better to compare the poisoned values because that doesn't leak any info about the underlying data, whereas comparing the underlying data definitely leaks it (albeit that's a super unlikely source of info).

Thanks.  I understood you.  However, I'm choosing to support them instead.  My reason is that we intended Poisoned to be almost a drop in replacement for pointers.  So, as much as possible (with a minimum of effort),  I prefer to be able to use Poison in any way that we can use raw pointers.  Since these operators are trivial to implement, I went ahead and implemented them.

Also, today I realize that my previous implementation was not wrong; it was just incomplete.  It was also late last night when I was working on it, and I thought I had a bug where there was not.  I'll land the current patch and reinstall the original operators (for optimization reasons because similarly poisoned pointers can just compare the poisoned values) in a subsequent patch.

[WebKit Commit Bot]

Comment on attachment 331281 [details]
patch for landing.

Clearing flags on attachment: 331281

Committed r226940: <https://trac.webkit.org/changeset/226940>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.