Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dangerous dictionary address #130

Closed
fredrikr opened this issue Aug 10, 2021 · 11 comments
Closed

Dangerous dictionary address #130

fredrikr opened this issue Aug 10, 2021 · 11 comments

Comments

@fredrikr
Copy link

The Inform 6 standard library does something dangerous, in that it tries to distinguish between the return values 'verb' and -'verb' from a grammar property routine. If the dictionary spans the $8000 mark, and the first word in the dictionary starts at address a where ($8000 - a) % dictionary_entry_length == 0, the test to see if the value is a negative dictionary word may fail.

I can't see a way to fix this in the standard library, but the compiler could save the day by making sure the dictionary does not start at an address where this could become a problem. E.g. for a z5 game the dictionary entry size is 9, and so the first dictionary word should not start on 32768 - 9, or 32768 - 18 etc.

@erkyrath
Copy link
Contributor

This is this test:

        if ((i ~= 0 or 1) &&
            (UnsignedCompare(i, dict_start) < 0 ||
             UnsignedCompare(i, dict_end) >= 0 ||
             (i - dict_start) % dict_entry_size ~= 0)) {
            usual_grammar_after = j;
            i=-i;
        }

@erkyrath
Copy link
Contributor

I don't see how this is a problem? It doesn't matter whether the dictionary crosses address $8000, because we're only looking at the relative position in the dictionary. It will fail if the dictionary is more than $8000 bytes long, but your fix doesn't address that.

@fredrikr
Copy link
Author

Unless I'm mistaken (and this is definitely a possibility), this code is a bit tricky to write because if the dictionary crosses $8000, it's hard to say whether the user has returned a dictionary word or a dictionary word with a unary minus in front of it. The code tries to catch this case by checking that the address % dict_entry_size ~= 0, and this makes it work eight times out of nine.

Consider the case where we have dictionary word 'get' on address 32759 and 'go' on address 32777. The binary representation of the address 32777 is 1000 0000 0000 1001, which is the same as the signed 16-bit integer -32759. So if we get the return value -32759, there is no way to tell if this means 'go' or -'get'.

@erkyrath
Copy link
Contributor

Ok, thanks -- I get it now.

It should be possible to fix this by changing the grammar spec to "return true, false, 'verb', or ('verb'+1)". The library code (quoted above) will detect this just as well. (dict_entry_size can't be 1). And the sign bit doesn't affect the outcome.

Right?

Given that, I don't think it's necessary to adjust the compiler.

@fredrikr
Copy link
Author

Yes, that would be possible. It would of course mean that the information in DM4 becomes incorrect, and old code that has always worked may break.

@erkyrath
Copy link
Contributor

It's not a library code change, so old game code will continue to work the same as it has. If it ran into this bug before, updating the game code is sufficient. If you want to recompile old buggy game code exactly, then the output should be bug-for-bug compatible. :)

@fredrikr
Copy link
Author

Detection will still work, but i = -i; won't have the desired effect.

@erkyrath
Copy link
Contributor

Hah, whoops, right. Silly of me.

Okay, it's a library change. I still think it's preferable to adjusting the compiler.

@fredrikr
Copy link
Author

I created a library issue for this: https://gitlab.com/DavidGriffith/inform6lib/-/issues/124

@DavidGriffith
Copy link
Contributor

I'm tying up the loose ends of a library fix, which was submitted by @fredrikr and finish up a caveats file to show authors how to avoid trouble. Could I get some commentary from @erkyrath on why fixing this in the compiler is not a good idea?

@erkyrath
Copy link
Contributor

erkyrath commented Feb 1, 2022

I guess my comment is "It's a bad idea to put an extra constraint on the compiler because of bad library design." The library can be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants