Smalltalk › Squeak › Squeak VM

SmartSyntaxInterpreterPlugin code generation issue

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

2 messages Options

Levente Uzonyi

SmartSyntaxInterpreterPlugin code generation issue

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.

Levente

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

Eliot Miranda-2

Re: SmartSyntaxInterpreterPlugin code generation issue

Hi Levente,

> On Dec 17, 2018, at 5:37 AM, Levente Uzonyi <[hidden email]> 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)