[Vm-dev] Fwd: Merging FilesAttributesPlugin

Wed Jan 10 07:06:16 UTC 2018

Hi Eliot,

Could I ask you to update FileAttributesPlugin.c on github in
OpenSmalltalk/opensmalltalk-vm to:

Name: FileAttributesPlugin.oscog-AlistairGrant.26
Author: AlistairGrant
Time: 5 January 2018, 9:02:46.033079 am
UUID: 07c4e352-8193-452a-a80a-440f0811d43f
Ancestors: FileAttributesPlugin.oscog-AlistairGrant.25

This version compiles on Windows and fixes a problem with DST handling
on Windows.

http://smalltalkhub.com/#!/~Alistair/FileAttributesPlugin/versions/FileAttributesPlugin.oscog-AlistairGrant.26

If you'd prefer that I generate the C file and submit a PR, let me know.

Thanks!
Alistair

On 5 January 2018 at 09:21, Alistair Grant <akgrant0710 at gmail.com> wrote:
> Hi Eliot,
>
> On 4 January 2018 at 21:25, Eliot Miranda <eliot.miranda at gmail.com> wrote:
>>
>> 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.
>
> The current design (copied from David's DirectoryPlugin) is that the
> pointer returned by opendir() is saved in a ByteArray (there's nothing
> special about this object, it isn't pinned as far as I know, and no
> malloc/free calls involved :-)).  The pointer is then retrieved and
> used in subsequent operations.  Being C, if a bad pointer is passed in
> to readdir() or closedir() the result is a core dump.  So to minimise
> the chance of that happening I'm making the parameter checking as
> strict as possible, and ensuring that a string (of the correct length)
> isn't passed in by accident.  As you say, the chances of that
> happening are tiny, but the results are severe (a core dump).  It
> appears to be working correctly in the latest version.
>
> I've also rebuilt the plugin on Windows 32bit and had to make a few
> more changes:
>
> - The combination of #cppIf:ifTrue:[ifFalse:] and more agressive
> inlining means that smalltalk #ifTrue:#ifFalse: message sends are
> translated to C ternary conditional statements (?:).  If the smalltalk
> #ifTrue:#ifFalse: contains a return (^), this can end up generating
> invalid C code, sometimes trying to return from within the ternary
> conditional, and sometimes trying to goto out of the conditional.
> - There was a bug in the time zone handling on Windows, which I've fixed.
>
> The latest version is now:
>
> Name: FileAttributesPlugin.oscog-AlistairGrant.26
> Author: AlistairGrant
> Time: 5 January 2018, 9:02:46.033079 am
> UUID: 07c4e352-8193-452a-a80a-440f0811d43f
> Ancestors: FileAttributesPlugin.oscog-AlistairGrant.25
>
> Version 1.2.4:
>
> - Adjust for #cppIf:ifTrue:[ifFalse:] and inlining behaviour
> -- Smalltalk #ifTrue:#ifFalse: messages appear to be translated to C
> ternary operator instead of a if[else] statement.
> - Remove call to sqFileInit() (accidentally copied across from FilePlugin).
> - Fix stat() time zone adjustments on WIN32
>
>
> Thanks!
> Alistair
>
>
>
>>> 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