[Vm-dev] Fwd: Merging FilesAttributesPlugin

Alistair Grant akgrant0710 at gmail.com
Thu Jan 4 15:06:13 UTC 2018


Hi Eliot,

Yet another lesson (for me) on why I shouldn't rush emails out just
before going to bed.

The crash is due to #pointerFrom: only checking for a byte type
object, so sending a random string of the correct length will cause
the VM to crash.  I've tightened the parameter checking, and neither
the 32 bit or 64 bit images crash now.

Updated version:

Name: FileAttributesPlugin.oscog-AlistairGrant.25
Author: AlistairGrant
Time: 4 January 2018, 4:02:11.405148 pm
UUID: 5b76e8bb-69e1-4db4-b529-9de1249e63e0
Ancestors: FileAttributesPlugin.oscog-eem.24

Version 1.2.3:

- Ensure that #pointerFrom: is given a ByteArray, not just a byte
object, which can be a string.


Thanks,
Alistair



On 4 January 2018 at 00:05, Alistair Grant <akgrant0710 at gmail.com> wrote:
> Hi Eliot,
>
> I'm able to reliably crash the 64 bit VM by passing a string to
> primitiveClosedir in the FileAttributesPlugin (which should never be
> done in practice, I've got it as a test for the parameter checking):
>
>
> primitiveClosedir
>     "Close the directory stream for dirPointerOop. Answer
> dirPointerOop on success.
>     Raise PrimErrBadArgument if the parameter is not a ByteArray
> length size(void *).
>     If closedir() returns an error raise PrimitiveOSError."
>
>     | dirPointerOop dirStream result |
>     <export: true>
>     <var: 'dirStream' type: 'osdir *'>
>     dirPointerOop := interpreterProxy stackValue: 0.
>
>
>     ">>>> Putting the following back prevents the crash..."
>     (interpreterProxy is: dirPointerOop KindOf: 'ByteArray') ifFalse:
>         [^interpreterProxy primitiveFailFor: PrimErrBadArgument].
>     "<<<<"
>
>     dirStream := self pointerFrom: dirPointerOop.
>     dirStream ifNil:
>         [^interpreterProxy primitiveFailFor: PrimErrBadArgument].
>     result := self closedir: dirStream dp.
>     result = 0 ifFalse:
>         [^interpreterProxy primitiveFailForOSError: self unableToCloseDir].
>     self free: dirStream.
>     interpreterProxy pop: 2 thenPush: dirPointerOop
>
>
> I removed the explicit is:KindOf: ByteArray check as you said that the
> checks in pointerFrom: would be sufficient.
>
> Would you mind letting me know what your preferred solution is (put
> the explicit ByteArray check back in, or look to fix the checks in
> pointerFrom:, or ...)?
>
> Just for reference, pointerFrom: is:
>
>
> pointerFrom: directoryPointerBytes
>     "Answer the machine address contained in anExternalAddressOop."
>
>     | ptr addressUnion idx |
>     <returnTypeC: 'void *'>
>     <var: 'ptr' type: 'unsigned char *'>
>     <var: 'addressUnion' type: 'union {void *address; unsigned char
> bytes[sizeof(void *)];}'>
>     ((interpreterProxy isBytes: directoryPointerBytes) and:
>         [(interpreterProxy stSizeOf: directoryPointerBytes) = self
> sizeOfPointer])
>         ifFalse: [^ nil].
>     ptr := interpreterProxy arrayValueOf: directoryPointerBytes.
>     idx := 0.
>     [idx < self sizeOfPointer] whileTrue:
>         [self cCode: 'addressUnion.bytes[idx] = ptr[idx]'.
>         idx := idx + 1].
>     ^ self cCode: 'addressUnion.address' inSmalltalk: [addressUnion]
>
>
> Thanks,
> Alistair
>
>
>
> On 3 January 2018 at 20:45, Alistair Grant <akgrant0710 at gmail.com> wrote:
>> Hi Eliot,
>>
>> On 3 January 2018 at 20:20, Alistair Grant <akgrant0710 at gmail.com> wrote:
>>> Hi Eliot,
>>>
>>> On 3 January 2018 at 19:04, Eliot Miranda <eliot.miranda at gmail.com> wrote:
>>>>
>>>> Hi Alistair,
>>>>
>>>> On Tue, Jan 2, 2018 at 2:42 AM, Alistair Grant <akgrant0710 at gmail.com> wrote:
>>>>>
>>>>>
>>>>> Hi Jakob,
>>>>>
>>>>> On 2 January 2018 at 06:46, Jakob Reschke <forums.jakob at resfarm.de> wrote:
>>>>> >
>>>>> >
>>>>> > Am 02.01.2018 04:43 schrieb "Eliot Miranda" <eliot.miranda at gmail.com>:
>>>>> >
>>>>> >  ... inst vars such as sCOFfn are set up in initialiseModule.  You're querying the SecurityPlugin on every access as in
>>>>> >
>>>>> >
>>>>> > canStatFilePath: aPathCString length: length
>>>>> > ...
>>>>> > (hasSecurityPlugin = 0) ifTrue: [^ true].
>>>>> > sCOFfn := interpreterProxy ioLoadFunction: 'secCanOpenFileOfSizeWritable' From: 'SecurityPlugin'.
>>>>> > sCOFfn ~= 0
>>>>> > ...
>>>>> >
>>>>> >
>>>>> > I have never looked at this before, so: can this variable name be spelled out, please, so it becomes legible for the uninitiated folk?
>>>>>
>>>>> I've made this change in my image, so it will flow through on the next update.
>>>>
>>>>
>>>> I don't think it's necessary.  The long vars are unwieldy and I'm sure Jakob understands the convention now.
>>>
>>> OK.
>>>
>>>
>>>> I just did a minor edit on the plain.  I wonder if it's time to move the package to the VMMaker repository.  In any case I'll start including it in builds.
>>>
>>> Yes, please :-)
>>>
>>> My build system has suddenly decided that it can't find
>>> libpulse-simple.  I can't see that any of the changes you've made will
>>> impact it, but I'll work on getting my build working again and running
>>> it through the test suite.
>>
>> Deleting the running docker container and going back to the clean
>> image fixed the libpulse-simple problem.
>>
>> And all the automated tests pass (FileAttributesPlugin.oscog-eem.22,
>> Ubuntu 16.04).
>>
>> Thanks!
>> Alistair


More information about the Vm-dev mailing list