[Vm-dev] SmartSyntaxInterpreterPlugin code generation issue

[Eliot Miranda]

Hi Levente,

> On Dec 17, 2018, at 5:37 AM, Levente Uzonyi <leves at caesar.elte.hu> wrote:
> 
> Hi All,
> 
> I found a bug in SocketPlugin which will crash the VM when triggered. While tracking the bug down, I found that SocketPlugin is a subclass of SmartSyntaxInterpreterPlugin and the cause of the bug is flawed code generation.
> 
> This line of smalltalk code (from SocketPlugin >> #primitiveSocket:connectTo:port:)
> 
>    self primitive: 'primitiveSocketConnectToPort' parameters: #(#Oop #ByteArray #SmallInteger ).
> 
> is translated to[1]
> 
>    socket = stackValue(2);
>    success(isBytes(stackValue(1)));
>    address = ((char *) (firstIndexableField(stackValue(1))));
>    port = stackIntegerValue(0);
>    if (failed()) {
>        return null;
>    }
> 
> The problem here is that the code checks if stackValue(1) is a bytes object, but the result of the check is only used after all arguments are read and converted.
> So even if the second argument is not a bytes, the third line of the snipper above will treat is as a bytes object and firstIndexableField will cause segmentation fault.
> 
> I presume that other SmartSyntaxInterpreterPlugins have the same argument
> validation issues, so it would be best if the code generator were fixed.

Indeed!  Thanks Levente; I shall fix this ASAP today.  The generated code sucks in a performance level too; calling success and then testing failure is slow.  This pattern is much better:

    if (!((validate(arg[0])
     && ...
     && (validate(arg[N]))) {
          primitiveFailedFor(PrimErrBadArg);
          return nil;
     }
     v0 = marshall(arg[0]);
     ...
     vN = marshall(arg[N]);

So I shall aim for this pattern, but I suspect that’s a bigger fix than the obvious and slower guarding of firstIndexableField with a test of failed.

> Levente
> 
> [1] https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/src/plugins/SocketPlugin/SocketPlugin.c#L1137

Eliot
_,,,^..^,,,_ (phone)