Re: [Vm-dev] Fwd: Merging FilesAttributesPlugin

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@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@gmail.com wrote:

...

Hi Eliot,

On 3 January 2018 at 20:20, Alistair Grant akgrant0710@gmail.com wrote:

...

Hi Eliot,

On 3 January 2018 at 19:04, Eliot Miranda eliot.miranda@gmail.com wrote:

...

Hi Alistair,

On Tue, Jan 2, 2018 at 2:42 AM, Alistair Grant akgrant0710@gmail.com wrote:

...

Hi Jakob,

On 2 January 2018 at 06:46, Jakob Reschke forums.jakob@resfarm.de wrote:

...

Am 02.01.2018 04:43 schrieb "Eliot Miranda" eliot.miranda@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