Fix buffer overflow with argument parsing (fixes #1219).

[ali-mhamza]

Summary

Given enough comma-separated arguments (in lists, functions, etc.), a buffer overflow is triggered when emitOp (called from within emitShortArg from callSignature) is called (eventually).

Root Cause

The calls to validateNumParameters in multiple locations unconditionally increments the signature arity (signature->arity). While this does prevent the function from reporting a "Methods cannot have more than 16 parameters" error more than once for a particular parsing phase, it causes the arity field to increase without bound.

In callSignature called from subscript, this arity is then added to the instruction CODE_CALL_0, which has integer equivalent value 24. With enough commas, the arity in the signature can increase to 53, which causes this sum to equal 77. The sum is then used in emitOp called from emitShortArg (called from callSignature) as an index into stackEffects, which only contains 77 elements. Thus, any arity value of 53 or higher leads to an out-of-index buffer overflow error.

In the Wren test script used in the issue, the arity reaches 54 before the buffer overflow occurs.

Fix

The main issue here stems from the unconditional increment for signature->arity, even after reporting a maximum-parameter error. Thus, validateNumParameters now returns a Boolean flag indicating whether or not an error was reported (even if it was from similar errors previously reported in the same parsing phase). If an error had been reported, we set the arity to -1.
This has two purposes:

1. It resets the arity once more, preventing any future increments from causing its value to increase (let alone without bound).
2. It indicates to validateNumParameters that an error has been reported to silence it from reporting further errors in the same parsing phase.

Testing

• All tests pass (ran test script: util/test.py).
• ASAN no longer reports the buffer overflow when running the provided C + Wren test harness (I personally confirmed the overflow by using ASAN).

[ali-mhamza]

After opening this PR, I found a simpler approach to fix this bug/vulnerability. While the approach in the commit works, it involves multiple moving parts and changes multiple functions.

Instead, we can simply place a bounds-check on the instruction used as an index into stackEffects, as follows (added lines marked with ***):

static void emitOp(Compiler* compiler, Code instruction)
{
  emitByte(compiler, instruction);

  #define EFFECTS_SIZE sizeof(stackEffects) / sizeof(stackEffects[0]) ***
  
  if (instruction < EFFECTS_SIZE) ***
  { ***
    // Keep track of the stack's high water mark.
    compiler->numSlots += stackEffects[instruction];
  } ***

  if (compiler->numSlots > compiler->fn->maxSlots)
  {
    compiler->fn->maxSlots = compiler->numSlots;
  }

  #undef EFFECTS_SIZE ***
}

• This only requires changing one function, and minimally so.
• All the tests still pass, and the overflow is completed eliminated.
• The only other place where the arity is also used with an instruction to form an index in such a way is when emitShortArg is called from createConstructor, which would also be addressed by this change.

However, this solution introduces an extra conditional check for each instruction emitted within the compiler, which may lead to some (minor) compile-time overhead.