[OpenSmalltalk/opensmalltalk-vm] There is no bound check when marshalling FFI atomic integer arguments (#251)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[OpenSmalltalk/opensmalltalk-vm] There is no bound check when marshalling FFI atomic integer arguments (#251)

David T Lewis
 

When marshalling integer values, the value is first extracted from the Smalltalk oop via the method ffiIntegerValueOf: .

  • SmallInteger -> SmallInteger value (signed value on 31 or 61 bits on 32bits and 64bits VM)
  • false/true -> 0/1
  • nil -> 0
  • Character -> Character value (may be more than 8 bits long!)
  • LargePositiveInteger -> 32bits positive integer value (or fail) (or 64 bits if ThreadedFFIPlugin)
  • LargeNegativeInteger -> 32bits signed integer value (or fail) (or 64bits if ThreadedFFIPlugin)

Then the value is cast to the expected value in ffiArgByValue: which will have the effect of silently masking potential errors...
(1<<31 could be passed to a signed int and re-interpreted as -(1<<31) for example).

IMO we could enforce bound checks and raise an Error...
We should at least do this in the debug version if ever there is a concern about speed (but I don't think so).


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"There is no bound check when marshalling FFI atomic integer arguments (#251)"}],"action":{"name":"View Issue","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/issues/251"}}}</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] There is no bound check when marshalling FFI atomic integer arguments (#251)

David T Lewis
 

Note: we also have a tolerance for passing a pointer on integer types of equal size but wrong signedness... See code like(atomicType >> 1) = (specType >> 1) in

  • ffiValidateExternalData:AtomicType: for passing agrument to an aliased type by reference
  • ffiAtomicArgByReference:Class: which has the tolerance for signed/unsigned char/byte only

If we are enforcing bound checks, we might have unconsistent behavior
And we might have code that depend on this liberality due to platforms where char are signed...


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@nicolas-cellier-aka-nice in #251: Note: we also have a tolerance for passing a pointer on integer types of equal size but wrong signedness... See code like` (atomicType \u003e\u003e 1) = (specType \u003e\u003e 1)` in\r\n\r\n- `ffiValidateExternalData:AtomicType:` for passing agrument to an aliased type by reference\r\n- `ffiAtomicArgByReference:Class:` which has the tolerance for signed/unsigned char/byte only\r\n\r\nIf we are enforcing bound checks, we might have unconsistent behavior\r\nAnd we might have code that depend on this liberality due to platforms where char are signed..."}],"action":{"name":"View Issue","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/issues/251#issuecomment-382298579"}}}</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] There is no bound check when marshalling FFI atomic integer arguments (#251)

David T Lewis
In reply to this post by David T Lewis
 

We are talking about Foreign Function Interface, so we have to bear other's choice.
But we also can take full control. For this, we support 4 different FFI types:

  • sbyte and byte if the intention is to handle 8 bit integer (signed and unsigned respectively)
  • schar and char if the intention is to handle 8 bit character code (signed and unsigned respectively)

With those types, we can re-interpret precisely the intentions of external library thru our <cdecl: returnType "function_prototype"(argType)> declarations.

The question raised is more about changing the contract and making a Smalltalk module fail when it previously worked: such precise control was not always necessary in the past.

The main usage is (const char*), where the foreign expectation is receiving a NULL terminated string.
Since char is interpreted as unsigned by FFI - whatever the underlying machine/compiler dependent interpretation, and our character code are unsigned, the main usage should not be a problem.

Note that encoding has to be handled at an upper level, it's not explicitely described in FFI prototype, FFI is encoding-agnostic.

Note that we can pass a ByteString that will automatically be copied so as to have a NULL terminator.
Most ByteString are already Null terminated in Spur, unless their size is a multiple of 8, so we could optimize further and avoid a copy, but that's another subject.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@nicolas-cellier-aka-nice in #251: We are talking about Foreign Function Interface, so we have to bear other's choice.\r\nBut we also can take full control. For this, we support 4 different FFI types:\r\n\r\n- `sbyte` and `byte` if the intention is to handle 8 bit integer (signed and unsigned respectively)\r\n- `schar` and `char` if the intention is to handle 8 bit character code (signed and unsigned respectively)\r\n\r\nWith those types, we can re-interpret precisely the intentions of external library thru our `\u003ccdecl: returnType \"function_prototype\"(argType)\u003e` declarations. \r\n\r\nThe question raised is more about changing the contract and making a Smalltalk module fail when it previously worked: such precise control was not always necessary in the past.\r\n\r\nThe main usage is (const char*), where the foreign expectation is receiving a NULL terminated string.\r\nSince char is interpreted as unsigned by FFI - whatever the underlying machine/compiler dependent interpretation, and our character code are unsigned, the main usage should not be a problem.\r\n\r\nNote that encoding has to be handled at an upper level, it's not explicitely described in FFI prototype, FFI is encoding-agnostic.\r\n\r\nNote that we can pass a ByteString that will automatically be copied so as to have a NULL terminator.\r\nMost ByteString are already Null terminated in Spur, unless their size is a multiple of 8, so we could optimize further and avoid a copy, but that's another subject."}],"action":{"name":"View Issue","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/issues/251#issuecomment-382371286"}}}</script>