inseven / opolua

A compiled-OPL interpreter for iOS written in Lua
https://opolua.org
MIT License
12 stars 0 forks source link

Internal Error: memory.lua:253: Attempt to setStringLength beyond maxLen #219

Closed jbmorley closed 2 years ago

jbmorley commented 2 years ago

Description

I was pleasantly surprised to see that Pebbles now runs, only to discover it errors on second opening when (I assume) attempting to read in its ini file. i suspect this might be another example of a out of bounds but the app was getting away with.

Details

memory.lua:253: Attempt to setStringLength beyond maxLen
PEBBLES\CFREAD:000011F3: 57 [CallFunction] 0x0F (IoRead)
PEBBLES\INITCONFIG%:000004CE: 53 [RunProcedure] 0x0012 (name="CFREAD" nargs=0)
PEBBLES\INITIALISE%:00000283: 53 [RunProcedure] 0x0012 (name="INITCONFIG%" nargs=0)
PEBBLES\PEBBLES:00000209: 53 [RunProcedure] 0x0192 (name="INITIALISE%" nargs=0)
stack traceback:
    [C]: in function 'assert'
    memory.lua:253: in method 'setStringLength'
    memory.lua:431: in upvalue 'applyDataToSimpleVar'
    memory.lua:463: in method 'write'
    fns.lua:364: in function 'fns.IoRead'
    ops.lua:1006: in function 'ops.CallFunction'
    runtime.lua:816: in function <runtime.lua:801>
    [C]: in function 'xpcall'
    runtime.lua:933: in method 'pcallProc'
    runtime.lua:1288: in function 'runtime.runOpo'
tomsci commented 2 years ago

This is a(nother) interesting one. Short answer is, it's a bug in the program, or at least a misuse of variables, but one that the new memory model can tolerate. The long answer is, the program is doing something like this:

LOCAL tempString$(16)
...
IOREAD(handle%, ADDR(tempString$), 16)

with the intention to read 16 bytes of data from the preferences file into tempString$. However, that's not quite what that code does - ADDR() on a string returns the address of the string's length byte, not that of the first data byte.

So the code is overwriting the length byte of the temporary with the first byte of the result data. Normally this would cause some sort of issue when tempString$ was subsequently used - in this program however it never is used (because the first 16 bytes of the preferences file seems to be UIDs that aren't checked).

The correct thing do to would be something like:

IOREAD(handle%, ADDR(tempString$) + 1, 16)
POKEB ADDR(tempString$), 16 :REM this sets the string length to 16

The current (old) memory model expects that any write to the length byte of a string should be setting a valid length, and in this case 0x50 is not (its max length is only 16 bytes). The new Chunk-based memory model doesn't care and lets you do what you like to the underlying storage, just as EPOC itself did, with the same caveat that it may cause unexpected behaviour down the line.

tomsci commented 2 years ago

This is fixed on main now the new mem model has merged, Pebbles works fine for me.

jbmorley commented 2 years ago

For me too!