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/FileAttri...
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@gmail.com wrote:
Hi Eliot,
On 4 January 2018 at 21:25, Eliot Miranda eliot.miranda@gmail.com wrote:
Hi Alistair,
On Jan 4, 2018, at 7:06 AM, Alistair Grant akgrant0710@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@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