[Vm-dev] Fwd: Merging FilesAttributesPlugin

Eliot Miranda eliot.miranda at gmail.com
Thu Jan 4 20:25:35 UTC 2018


Hi Alistair,

> On Jan 4, 2018, at 7:06 AM, Alistair Grant <akgrant0710 at gmail.com> wrote:
> 
> 
> 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.

So was the problem that you passed in a string as the argument for one of the directory enumeration primitives and that the primitive interpreter the string as a pointer to a direct struct?

If so consider this redesign:
The result of the opendir primitive is a pinned ByteArray that is the entire direct struct, ideally with a session id either prepended or appended.  There is then a very small possibility of passing in some malformed argument, and even if one did manage to spoof the primitive's session id validity check the primitive wouldn't crash, but the directory support routines might, and hopefully would answer an error.

You can then eliminate malloc/free usage and leave it to the GC to collect the dirent ByteArrays.


> 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