valueDiff for arrays?

[Mark Waddingham]

On 2018-08-04 19:19, J. Landman Gay via use-livecode wrote:
> This caught my attention. The increased security is great, and I've
> heard it said before, but would love to hear the reasons that make it
> more secure than lower level code. That way I'd have a knowledgeable
> reason to be smug.

It is impossible to write LCS code which *just* manipulates values which 
will crash/vulnerability because of the code you have written in LCS 
*unless* there is actually a crash/vulnerability bug in the engine in 
one of the core operations which you are using.

If you translate an operation to C++, though - it is far far easier to 
introduce crashes and thus vulnerabilities - this is because C++ does 
not insulate you from the fundamentals of memory management, pointers or 
all that stuff which comes with writing in a language which is (in 
reality) only a short step away from machine code.

Admittedly, if you are using C++ as modern C++ then it is also quite 
hard to introduce such things (assuming you have built the abstractions 
using templates and C++'s typing system to do so correctly and use them 
uniformly and correctly) - but you still *can* write code that does as 
you can always by-pass C++'s typing and such - also the engine is not 
modern C++ - it is C which was transformed in C++ and due to various 
requirements on usage (at the lower-level) of the engine - many APIs 
used internally in the engine (e.g. libfoundation) are actually C APIs.

For example. Here is the core engine code for array intersection (which 
actually covers intersection, recursive intersection and difference):

static void MCArraysDoIntersect(MCExecContext& ctxt, MCValueRef p_dst, 
MCValueRef p_src, MCArrayDoIntersectOp p_op, MCValueRef& r_result)
{
     if (!MCValueIsArray(p_dst))
     {
         r_result = MCValueRetain(p_dst);
         return;

     }

     if (!MCValueIsArray(p_src))
     {
         if (p_op != kMCArrayDoIntersectOpDifference)
         {
             r_result = MCValueRetain(kMCEmptyString);
         }
         else
         {
             r_result = MCValueRetain(p_dst);
         }
         return;
     }

     MCArrayRef t_dst_array;
     t_dst_array = (MCArrayRef)p_dst;

     MCArrayRef t_src_array;
     t_src_array = (MCArrayRef)p_src;

     MCAutoArrayRef t_result;
     if (!MCArrayMutableCopy(t_dst_array, &t_result))
         return;

     MCNameRef t_key;
     MCValueRef t_src_value;
     MCValueRef t_dst_value;
     uintptr_t t_iterator;
     t_iterator = 0;

     while(MCArrayIterate(t_dst_array, t_iterator, t_key, t_dst_value))
     {
         bool t_key_exists;
         t_key_exists = MCArrayFetchValue(t_src_array, ctxt . 
GetCaseSensitive(), t_key, t_src_value);

         if (t_key_exists == (p_op == kMCArrayDoIntersectOpDifference))
         {
             if (!MCArrayRemoveValue(*t_result, ctxt . 
GetCaseSensitive(), t_key))
             {
                 ctxt . Throw();
                 return;
             }
         }
         else if (p_op == kMCArrayDoIntersectOpIntersectRecursively)
         {
             MCAutoValueRef t_recursive_result;
             MCArraysDoIntersect(ctxt, t_dst_value, t_src_value, p_op, 
&t_recursive_result);

             if (ctxt . HasError())
                 return;

             if (!MCArrayStoreValue(*t_result, ctxt . GetCaseSensitive(), 
t_key, *t_recursive_result))
                 return;
         }
     }

     r_result = MCValueRetain(*t_result);
}

This is, as far as I'm aware 'bug free' - in that the code respects all 
the semantics required of using all the lower-level functions it is 
built out of.

Here is a version which has a vulnerability/crash in it...

static void MCArraysDoIntersect(MCExecContext& ctxt, MCValueRef p_dst, 
MCValueRef p_src, MCArrayDoIntersectOp p_op, MCValueRef& r_result)
{
     if (!MCValueIsArray(p_dst))
     {
         r_result = MCValueRetain(p_dst);
         return;

     }

     if (!MCValueIsArray(p_src))
     {
         if (p_op != kMCArrayDoIntersectOpDifference)
         {
             r_result = MCValueRetain(kMCEmptyString);
         }
         else
         {
             r_result = p_dst;
         }
         return;
     }

     MCArrayRef t_dst_array;
     t_dst_array = (MCArrayRef)p_dst;

     MCArrayRef t_src_array;
     t_src_array = (MCArrayRef)p_src;

     MCAutoArrayRef t_result;
     if (!MCArrayMutableCopy(t_dst_array, &t_result))
         return;

     MCNameRef t_key;
     MCValueRef t_src_value;
     MCValueRef t_dst_value;
     uintptr_t t_iterator;
     t_iterator = 0;

     while(MCArrayIterate(t_dst_array, t_iterator, t_key, t_dst_value))
     {
         bool t_key_exists;
         t_key_exists = MCArrayFetchValue(t_src_array, ctxt . 
GetCaseSensitive(), t_key, t_src_value);

         if (t_key_exists == (p_op == kMCArrayDoIntersectOpDifference))
         {
             if (!MCArrayRemoveValue(*t_result, ctxt . 
GetCaseSensitive(), t_key))
             {
                 ctxt . Throw();
                 return;
             }
         }
         else if (p_op == kMCArrayDoIntersectOpIntersectRecursively)
         {
             MCAutoValueRef t_recursive_result;
             MCArraysDoIntersect(ctxt, t_dst_value, t_src_value, p_op, 
&t_recursive_result);

             if (ctxt . HasError())
                 return;

             if (!MCArrayStoreValue(*t_result, ctxt . GetCaseSensitive(), 
t_key, *t_recursive_result))
                 return;
         }
     }

     r_result = MCValueRetain(*t_result);
}

Can you immediately see the error?

Of course I've given you the correct and incorrect versions, so its easy 
to determine by line-by-line.

However, now imagine you don't know what the correct implementation is, 
and you are reviewing the code for inclusion for the first time. Would 
you have noticed the error (even assuming you actually designed/wrote 
most of the lower level operations which are being used here - which I 
did, in this case)?

Warmest Regards,

Mark.

P.S. The above bug I introduced actually causes one of the most 
pernicious sort - the effect of the bug is unlikely to be seen at the 
point of effect - it will only be seen later - potentially after many 
many other operations are run. For those that are interested - it 
introduces a reference-count error on one of the values - which would 
cause a value to be released whilst there are still references to it. 
They only have an effect *when* all true references are gone, *and* the 
block of memory which the value uses hasn't been re-used for the same 
value type (which frequently happens because the engine uses a small 
pool-allocator for values of distinct types meaning repeated freeing and 
creation of the same value type has almost zero cost).

P.P.S Fortunately we have tests and tools and such which help us to find 
such things - and experience in knowing where to look when reviewing. 
However, it is all too easy for such 'subtle' things to be missed. 
Particularly when there is a large amount of code involved.

-- 
Mark Waddingham ~ mark at livecode.com ~ http://www.livecode.com/
LiveCode: Everyone can create apps