[Vm-dev] VM Maker: VMMaker.oscog-eem.2338.mcz

[Levente Uzonyi]

Hi Eliot,

On Thu, 12 Apr 2018, Eliot Miranda wrote:

> [I'm sorry, I apparently forgot to hit send some weeks ago]

Well, I had started to rewrite the methods myself along with adding tests, 
but you were quicker and pushed the changes.
I think there are still things that should be fixed. When time allows, 
I'll check my tests and merge them with the existing 
MiscPrimitivePluginTest.

>> primitiveDecompressFromByteArray
>> - bm is int*, but it should probably be unsigned int* as the method 
stores unsigned ints (data) into it
>> - index should be bounds checked (1 .. end)
>
> It gets checked anyway with the comparison against pastEnd

Unfortunately that's not the case. i is compared to end, not pastEnd.
If index, which is an unnecessary variable btw, is less than or equal to 
0, then the value of i will be negative, and the input will be processed 
no matter what the value of end is, hence random data from memory will be 
read and processed:

Smalltalk garbageCollect.
ba := ByteArray new: 256.
bm := Bitmap new: 1024.
passes := OrderedCollection new.
success := true.
[
 	0 to: -1000000 by: -1 do: [ :start |
 		success := true.
 		bm decompress: bm fromByteArray: ba at: start.
 		success ifTrue: [ passes add: start ] ] ]
 	on: Error
 	do: [ :error |
 		success := false.
 		error return: nil ].
passes

>> - i, k, end and pastEnd should be unsigned int
>
> That's OK.  In neither V3 nor Spur can objects have more than 1Gb fields so in practice indices can never go negative.

If i's type were unsigned int, the above code wouldn't work, because i < end would be false.

Levente