MessageTrans 'abuse' by applications

Hiya,

Trying to debug an application last week, I discovered that it was using MessageTrans in an unexpected manner.

The normal way to use MessageTrans is to call `MessageTrans_OpenFile`, call `MessageTrans_(|GS|Error)Lookup` or `MessageTrans_MakeMenus` and eventually `MessageTrans_CloseFile`. This was not what was being done.

Instead, this application was calling `MessageTrans_OpenFile` to open the file and then assuming that the data that was placed in the file buffer was the data from the file that had been opened, and then any time it needed a lookup it would search the file buffer itself. In fact, the application would have been better off just calling `OS_File` to load the file, rather than invoking MessageTrans at all, as it wasn’t using any of the features of the module ¹.

Under RISC OS Pyromaniac this failed, because the buffer is populated with an optimised version of the content parsed out of the Messages file, not the raw content from the original file. This allows it to use hashed lookups of tokens, much more efficiently than if it had to search the entire messages file. The application could never find any of its tokens and aborted during startup due to unexpected results from the system.

Whilst I’ve been forced to make the format of the buffer match the implementation used by this application, I consider this an abuse of the buffer, whose content is not defined. The file format is defined, but the buffer belongs to MessageTrans and shouldn’t be assumed to be in any particular format.

Optimisations, such as the one I was using, are significantly harder when the internal implementation is treated as the interface. Fortunately, I can get away with stashing information in the host system which is never seen by RISC OS, but this is not true of pure ARM implementations.

So… if you’re doing that kind of thing… Please stop it and stick to the defined interfaces :-)

¹ Not strictly true; MessageTrans will decompress the file if it is a Squash file ², so there is a small benefit there.

² There was a module in the dim past that used to use the fact that the content would be decompressed implicitly by MessageTrans when it was opened, in order to load its sprites. Do not do this.

I discovered that it was using MessageTrans in an unexpected manner […] any time it needed a lookup it would search the file buffer itself.

As someone guilty of ‘abusing’ MessageTrans a bit ¹, I find that one very odd. ;-)

Under RISC OS Pyromaniac […] the buffer is populated with an optimised version of the content parsed out of the Messages file …

That is very interesting and I would applaud such an improvement in RO itself, but it is only Pyromaniac, isn’t it?

As things stand, I see that the buffer content is, indeed, not defined, but it is quite strongly implied to be the raw file content. This comes to mind immediately:

• a pointer to a message is returned with the line terminator of the file, indicating direct access to the data

Please stop it and stick to the defined interfaces :-)

A worthy sentiment, except that the ‘defined interface’ says you can put the descriptor block in application memory. As it is in a linked list used by MessageTrans, that can be a bit of a problem. ;-)

¹ a very long time ago, about the time I was having a little correspondence with JF about the iniquities of MessageTrans and ResourceFS, I worked out the content of the descriptor block. I could look it up in the source now.

Since then I have been a very naughty boy. ;-(

In an application, I construct a ‘dummy’ descriptor block pointing to the loaded file data, but ignoring the housekeeping information. I do a very similar thing with files loaded into ResourceFS with my RFSFiles module or in modules.

I can use the various lookups and there is no call for OpenFile or CloseFile.

I really ought to stop this, but I have been using it for well over two decades and it does avoid the pitfalls of the ‘proper’ way.

That is very interesting and I would applaud such an improvement in RO itself, but it is only Pyromaniac, isn’t it?

Yes, but that’s kinda irrelevant to the issue in this case. You can very easily see a version of MessageTrans that does this being used in RISC OS – and the reason that you can see such a version being used is that Pyromaniac does it relatively well… with the exception of this case.

This is where Pyromaniac is useful (IMO) – you can try stuff out and see if it will work. There’s always going to be problems with people that have done things that are wrong according to the definition, and sometimes you can make the implementation handle those abuses, and other times you just have to say ‘well, tough, you’ve done it wrong and that’s what you get’. There’s always a fine line between changing the existing behaviour and retaining everything exactly the way that it was.

There’s no reason why anyone else cannot implement a replacement which used these sorts of techniques ².

As things stand, I see that the buffer content is, indeed, not defined, but it is quite strongly implied to be the raw file content.

Implied, I’ll agree, but not stated. It says that the content is the ‘file data’. Not that it is the file itself. Given that 99.99% ⁴ of people will use MessageTrans the way that was expected – by calling the MessageTrans SWIs – this won’t really affect them.

This come to mind immediately: a pointer to a message is returned with the line terminator of the file, indicating direct access to the data

The direct access to the pointers is retained in the Pyromaniac, because as you say that’s a feature of the API. In the Pyromaniac version before this change, the “file data” that was stored in the buffer was just those strings. The Messagetrans buffer didn’t contain any token names at all, because MessageTrans is never required to return pointers to them directly (EnumerateTokens uses a buffer that is populated, rather than pointers into the cached file data). It’s because of this that the application had a problem – it was expecting the file data to containg `(token):(value)` sequences, but was just seeing a sequence of `(value)` strings. I’ve updated the code to now so that it populates the buffer with the token names as well. This allows the application to work, and could still be retained in another implementation.

The internal implementation of the MessageTrans class is very simple; the following comprises the bulk of the class (names omitted to protect the guilty):

    def __init__(self, ro, filename, filebuffer):
        self.ro = ro
        self.filename = filename
        self.token_order = []
        self.tokens = {}
        self.allocated = None
        self.token_offset = {}
        self.wildcards = []

        if not filebuffer:
            # No filebuffer means allocate memory in RMA
            # Find the size of the file
            (objtype, loadaddr, execaddr, length, attr) = self.ro.kernel.api.os_file_read_info(filename)
            self.allocated = self.ro.kernel.da_rma.allocate(length + 1)
            filebuffer = self.allocated
        self.token_memory = filebuffer

        offset = 0
        tokens = []
        with self.ro.kernel.api.open(filename, 'r') as fh:
            for line in fh:
                # Must strip off the newline from the end
                line = line.rstrip('\n')
                if line == '' or line[0] == '#':
                    # Skip this line
                    continue

                if ':' not in line:
                    # This line doesn't have a : in it, so this is a continuation
                    tokens.extend(line.split('/'))
                    continue

                (toks, value) = line.split(':', 1)
                tokens.extend(toks.split('/'))

                # Some users, ie <redacted>, will parse the memory buffer themselves, so we cannot
                # skip writing out the tokens before the string itself. Earlier versions of this
                # MessageTrans wouldn't do this, which resulted in failed lookups in <redacted>.
                tokens_string = '/'.join(token for token in tokens if token)
                filebuffer.write_string(tokens_string + ':', offset=offset)
                offset += len(tokens_string) + 1

                value_offset = offset
                filebuffer.write_string(value, offset=offset)
                offset += len(value) + 1

                self.token_order.extend(tokens)
                for token in tokens:
                    if not token:
                        continue
                    if '?' in token:
                        self.wildcards.append(token)
                    self.tokens[token] = value
                    self.token_offset[token] = value_offset
                tokens = []

    def lookup_token(self, token):
        """
        Given a token name, find the token that matches it, or None.

        @param token:   The token name supplied

        @return:    Actual token to use, or None if token does not match
        """
        if token in self.tokens:
            return token
        for wild in self.wildcards:
            match = fnmatch.fnmatch(token, wild)
            if match:
                return wild
        return None

    def lookup_value(self, token):
        """
        Given a token name, return its value, or None.

        @param token:   The token name supplied

        @return:    Token's value
        """
        token = self.lookup_token(token)
        if not token:
            return None
        return self.tokens[token]

That’s pretty much it – there’s a few other methods for housekeeping and enumeration in that class). The rest of the source deals with managing multiple of these files and the OS APIs.

Here, I’m using the host python storage to manage the dictionaries for lookups. In a native implementation you’d instead be forced to place this hash information in the buffer – and (for this frustrating use in the applications) you’d have to protect your hash information from interfering with the lookups that it might have performed. Probably you could get away with placing the hash information at the end of the text buffer – but THEN you’re relying on people calling MessageTrans_FileInfo to get the size of the buffer required, and not just calling OS_File and assuming that the size of the buffer is the size of the file. OR you place your hash data in private workspace (similar to my dictionary) and have to track the uses so that memory is freed along with the messagetrans descriptor, even if that close is implicit by the client going away silently (application death, module death without a close, etc).

a very long time ago, about the time I was having a little correspondence with JF about the iniquities of MessageTrans and ResourceFS

Was that in reference to “The MessageTrans Bug” where if you closed a TaskWindow at just the wrong time, the system exploded badly? ¹

I also remember some discussions about how scarey the use of MessageTrans and ResourceFS can be when ShareFS is actively being updated ³.

In an application, I construct a ‘dummy’ descriptor block pointing to the loaded file data, but ignoring the housekeeping information. I do a very similar thing with files loaded into ResourceFS with my RFSFiles module or in modules.

This will not work in Pyromaniac. The implementation of this block is entirely different. If you attempt to construct a descriptor that isn’t managed by MessageTrans it will check the content and it won’t match the format MessageTrans expects and it will report that it’s not valid – because you could have (and did) passed a garbage pointer to it. In particular doint this would fall afowl of the following code:

    def lookup_mtdesc(self, mtdesc):
        if mtdesc[0].word != self.MAGIC:
            raise self.errors('Syntax', 'Not a messages descriptor (bad magic word)')
        desc_id = mtdesc[4].word
        if desc_id not in self.mapping:
            raise self.errors('Syntax', 'Not a messages descriptor (unknown descriptor id; may have been closed?)')

        return self.mapping[desc_id]

• The former error reports cases where the block is not in the form that we expect.
• The latter error reports cases where the block has been copied, closed and then attempts to be used again. When the file is closed, the magic word will be cleared, so you would have to have copied the block first.

¹ As I recall, MessageTrans had chained the descriptors which used automatic allocation together in such a way that if you allocated the descriptor on the SVC stack, you could end up in a situation where the MessageTrans chain ended up passing through memory which had subsequently been paged out. It might not have been the SVC, it might have been another area of memory, but that’s my recollection. If I recall right, this was identified and fixed by Stewart Brodie.

² I believe that Stewart Brodie, in addition to ¹, also added a certain level of hashing to locate first entry to search for in the content, or something similar. Might have been Kevin Bracey, though. My memory sucks. I can’t even remember if that made it into mainline OSs.

³ ShareFS registers the discs that it knows about in ResourceFS. As remote machines come and go, this causes ResourceFS to be updated. This causes services to be sent around to notify users that the ResourceFS content may have changed. MessageTrans notices this and may notify its clients that messages file content might have changed. That’s all great, except that few clients pay attention to these messages, and it’s a lot of overhead triggered by a user elsewhere on the network rebooting their machine (or whatever). The problem here lies with abusing the ResourceFS system as a mechanism for presenting shared discs, OR with having direct access registered content within the ResourceFS files. The reason that the latter exists at all is because originally RISC OS was significantly memory constrained and copying data from ROM was inefficient as it’s already directly accessible. Such issues really shouldn’t be a concern these days.

⁴ Figure is made up.

This will not work in Pyromaniac.

I did not expect it to.

In particular doing this would fall afowl of the following code:

I think I see that descriptors are not held in a linked lists here. I recall that you expressed strong feelings about broken lists. It also appears that normal MT is more robust now, as well.

Despite what I said, my serious modules do open and close Messages correctly. ;-)

I have a question about Messages files already held in memory ¹, either in ROM or in RMA in modules. As it stands, these are used directly. Does Pyromaniac take these and produce its own modified versions? That may be answered in the code above, but I have missed it.

¹ I realise that ‘in memory’ has a different meaning for P’.

I think I see that descriptors are not held in a linked lists here. I recall that you expressed strong feelings about broken lists. It also appears that normal MT is more robust now, as well.

Yeah, they’re not linked here – instead we use the identifier only as the reference to the which internal structure we’re accessing. This means that if you copy descriptors it won’t break. The downside to this is that it means a lookup in a hash table for every instance (in Python this is a reasonably efficient operation). The upside is that it means you’re not providing way to get at other people’s data from within your messagetrans block – if you dereference anything within the block you won’t be able to a) break another messagetrans client, or b) snoop on them.

The Classic MessageTrans now (IIRC, and ‘now’ being circa ‘99) spots that you’ve placed your descriptor in application space or SVC space and if so it creates a real descriptor block in RMA and makes your appspace/SVC descriptor block point to the real descriptor block in RMA – essentially proxying to the new block. This means that the page-out-able block of memory is never in the chain, and thus you cannot break MessageTrans by badness happening there. The worst you can do is leak the 16 byte RMA descriptor that was created on your behalf.

I have a question about Messages files already held in memory, either in ROM or in RMA in modules. As it stands, these are used directly. Does Pyromaniac take these and produce its own modified versions? That may be answered in the code above, but I have missed it.

I assume you’re talking about the ResourceFS sourced messages files. In Pyromaniac’s version of MessageTrans ResourceFS (and the collusion that’s necessary to find their in memory address) are not handled specially at all. You gave it a filename and it handled it as if the file was on the filesystem. This means that if you provided a buffer of 0 (meaning access directly if you can, or load into RMA if not), the ‘access directly’ option is never exercised by the Pyromaniac implementation – it always allocates RMA for it.

Memory is cheap and faffing with different code paths for different implementations is way more work than I care for (at least at the moment). Because users who have put their resources into ResourceFS are still expected to handle the case where the files are not accessed directly, the implications of this are not (I think) significant.

Memory is cheap and faffing with different code paths for different implementations is way more work than I care for …

That is very true, but it hurts a little bit after trying to save every byte and millisecond for a long time. ;-(

The worst you can do is leak the 16 byte RMA descriptor that was created on your behalf.

Thanks for that, and all the other information. I am persuaded to be a good boy from now on. ;-)

it hurts a little bit after trying to save every byte and millisecond for a long time. ;-(

Steve, prepare to be broken.

https://blog.davetcode.co.uk/post/21st-century-emulator/

Note the runtime image sizes in the table. A little piece of me died whilst reading them…

https://blog.davetcode.co.uk/post/21st-century-emulator/

“He considered this for a moment… “Death’s too good for them,” he said.”

Death by lowering very slowly into a woodchipper…

I would like to see a pessimised version of qemu (or similar) that absolutely refuses to automatically update caches and memory until it has been forced to, and notices accesses from other cores/devices to memory/caches that haven’t been flushed, just to make sure you’ve included every dsb and cache maintenance instruction.

A little piece of me died whilst reading them

Rick, you do realise that it is a joke. ;-)

The degree of sarcasm is unmistakable – however, over half a gigabyte to implement a MOV instruction.

It’s awesome that such a crazy thing is even possible, but FFS… my first dozen computers couldn’t even dream of the amount of memory that this stuff is casually wasting.

Back on topic…

Trying to debug an application last week, I discovered that it was using MessageTrans in an unexpected manner.

I’ve seen some shocking code over the years that “technically” follows spec, but as the spec in question invariably doesn’t cover everything, some folk take it upon themselves to essentially extend the spec to fit their particular requirement.

The problem us emulatory folk have is it then breaks our code as we come across something unexpected that isn’t in the spec that we can’t cater for. I see it all the time in games.

At this point we have three routes open to us, patch our code to match what would happen under the original OS, patch the poorly written user code or just not support it.

In my case I usually go for the second option and patch the game, falling back to modifying my emulation if it’s something that’s commonly done. This is fine as I’m working to a fixed list of games that’s codebase isn’t going to change.

In your case, I think I’d attempt to detect the rogue code, report it as an error and simply not support it as your users are likely to be the developers themselves and can correct their ways.

An interesting topic though and one of the reason I constantly highlight that the Wiki needs to be maintained, up to date and include more detail to prevent folk making the spec up to fit their requirements.

making the spec up to fit their requirements.

This is the problem with an inadequately defined spec. One of the problems with attempting to compile BASIC is that the spec defines what the keywords do and how you define variables etc, but it doesn’t define how the language actually behaves, so it is quite common to see a lot of horrible code that is bad but is legitimate and accepted by the interpreter.

My go-to example is a function with multiple entry points that can return an integer or a string.

Another very common example is side effect abuse of the form:

SYS "XOS_GenerateError", some!block% TO mystring$

If one leaves gaps in the spec, somebody will find them. Like 6502 undefined opcodes, if those gaps in the spec are useful, somebody will use them.

As things stand, I see that the buffer content is, indeed, not defined, but it is quite strongly implied to be the raw file content. This comes to mind immediately:
a pointer to a message is returned with the line terminator of the file, indicating direct access to the data

It also says “This call opens a message file for subsequent use by the MessageTrans module” (my emphasis). An app shouldn’t be using the handle for its own purposes.

The degree of sarcasm is unmistakable

And he says: “… I’m sure you’ve realised that the whole project is something of a joke. That said it is also an interesting intellectual exercise …”

Another very common example is side effect abuse of the form: SYS "XOS_GenerateError", some!block% TO mystring$

Not ‘abuse’ as this is a documented use in the Wiki, if not in the original PRM. It does serve to underline a shortcoming in BASIC itself, though.

It also says “This call opens a message file for subsequent use by the MessageTrans module” (my emphasis)

I’ll bite. Putting Pyromaniac to one side, it also says “This call opens a message file for subsequent use by the MessageTrans module” (my emphasis)

No it does not. If the file is not already in memory it loads it. If it were to open it it would not be altered. If you did not know how it works you could assume that is so. The terminology is confusing.

Not ‘abuse’ as this is a documented use in the Wiki, if not in the original PRM.

So the “abuse” is commonplace enough that it got documented as a known behaviour. This doesn’t automatically make it right, though.

It does serve to underline a shortcoming in BASIC itself, though.

Indeed it does. The null terminated string convention was known about right from the beginning (there’s a Write0 call, not a WriteD call, for example), and the SYS command automatically converts OS style strings to BASIC style. It seems strange that there wasn’t an extension to the $block% notation to perform an automatic conversion.

No it does not. If the file is not already in memory it loads it.

Deleted, see following message.

I thought a little more about this, and the PRM does give information on the file being loaded, either into application space or into the RMA, and also states Furthermore, if R2=0 on entry to this SWI, and the application uses direct pointers into the file (rather than copying the messages out) or uses MessageTrans_MakeMenus, it should also trap Service_MessageFileClosed, in case the file is unloaded.

So…?

So…?

So, you missed my point completely, but best leave it there. ;-)