FromVar hardening#4040
Conversation
rajatd
force-pushed
the
is_fromVar_master
branch
from
b57a336 to
7f3de4d
Compare
| AssertMsg(Is(aValue), "var must be an ArrayBuffer"); | ||
| AssertOrFailFastMsg(Is(aValue), "var must be an ArrayBuffer"); | ||
|
|
||
| return static_cast<ArrayBuffer *>(RecyclableObject::FromVar(aValue)); |
There was a problem hiding this comment.
This can be UnsafeFromVar, since there is already failfast right above
There was a problem hiding this comment.
Looks like a number of the FromVars have this. But seems like in some they just directly static_cast a var to the type. If that's possible, it seems preferable here
In reply to: 147226615 [](ancestors = 147226615)
There was a problem hiding this comment.
Makes sense. I'll try to make it uniform across FromVars, either with or without RecyclableObject::UnsafeFromVar
| ScriptContext * scriptContext = function->GetScriptContext(); | ||
|
|
||
| TypeId typeId = JavascriptOperators::GetTypeId(arrayArg); | ||
| if (!JavascriptNativeArray::Is(typeId) && !(TypedArrayBase::Is(typeId) && typeId != TypeIds_CharArray && typeId != TypeIds_BoolArray)) |
There was a problem hiding this comment.
is this change in semantics? or were the extra conditions just redundant?
There was a problem hiding this comment.
TypedArrayBase::Is does a range check for typeId and TypeIds_CharArray and TypeIds_BoolArray fall in that range. Presumably, they didn't fall in the range when this code was written.
There was a problem hiding this comment.
If TypedArrayBase::Is(typeId) returns true for TypeIds_CharArray, then this is a change in semantics: the logic before (pushing in the negation) was
!JavascriptNativeArray::Is(typeId) &&
(!TypedArrayBase::Is(typeId) || typeId == TypeIds_CharArray || typeId == TypeIds_BoolArray)which means that if it was a CharArray or BoolArray (and not a native array) the condition would succeed, while now it would fail.
There was a problem hiding this comment.
That is true; my bad. Will revert this. Thanks for catching this.
rajatd
force-pushed
the
is_fromVar_master
branch
from
1dc9e51 to
f8acf7f
Compare
Merge pull request #4040 from rajatd:is_fromVar_master This PR hardens the FromVar method on Chakra's runtime classes to fail fast if the var passed in is not of the expected type. Although, there are a lot of instances in the code where we call FromVar only after ensuring that the var has the expected type. Those cases will now have to do a redundant check in FromVar and that has perf impact. To counter such perf loss, I have introduced two new methods: 1. `UnsafeFromVar` - it is identical to FromVar that we have today (checks the type, but only in debug builds). Used in cases where the type check is done by a switch on typeId. 2. `JavascriptOperators::TryFromVar<T>` - Performs both the type check and pointer cast (as dictated by the template parameter) for the passed in var. Used in cases where T::Is() is immediately followed by a T::FromVar.
4 participants
This PR hardens the FromVar method on Chakra's runtime classes to fail fast if the var passed in is not of the expected type. Although, there are a lot of instances in the code where we call FromVar only after ensuring that the var has the expected type. Those cases will now have to do a redundant check in FromVar and that has perf impact. To counter such perf loss, I have introduced two new methods:
UnsafeFromVar- it is identical to FromVar that we have today (checks the type, but only in debug builds). Used in cases where the type check is done by a switch on typeId.JavascriptOperators::TryFromVar<T>- Performs both the type check and pointer cast (as dictated by the template parameter) for the passed in var. Used in cases where T::Is() is immediately followed by a T::FromVar.