Hey everyone,
In the course of implementing persistent achievements for the zmachine using the extended @save and @restore opcodes, I’ve discovered an issue with unix frotz: when an auxiliary file is @saved or @restored, frotz ignores the filename suggested by the program and instead suggests an empty or corrupted string as the default filename; in some cases frotz crashes with a SIGABRT at this time.
I’ve finally had the time to investigate the issue, and I’d like to lay out what I’ve found.
First, a brief test program that demonstrates the error:
Constant Story "AUXILIARY FILES";
Constant Headline "^An Interactive Investigation^";
#ifndef TARGET_ZCODE;
Message fatalerror "This program uses z-machine specific opcodes.";
#endif;
Include "Parser";
Include "VerbLib";
Object Storage_Chamber "Storage Chamber"
with description "Type ~mysave~ to save data and ~myload~ to load data.",
has light;
[ Initialise;
location = Storage_Chamber;
];
Include "Grammar";
Constant SaveDataLen = 32;
Array savedata buffer SaveDataLen;
[ MySaveSub len prompt result;
@output_stream 3 savedata;
print "This is some data.^";
@output_stream -3;
len = savedata-->0 + WORDSIZE;
@save savedata len "mysavefile" prompt -> result;
print "MySave: result = ", result, "^";
];
Verb 'mysave'
* -> MySave;
[ MyLoadSub len prompt result;
len = SaveDataLen + WORDSIZE;
@restore savedata len "mysavefile" prompt -> result;
print "MyLoad: result = ", result, "^";
];
Verb 'myload'
* -> MyLoad;Next, a transcript of the test program running. In the transcript, we attempt to save to a new auxiliary file and then read from it.
After reading through the frotz source code, wielding gdb, and checking out the z-machine spec, I believe that I’ve found the root cause of the issue.
In src/common/fastmem.c, the @save and @restore opcodes are implemented by z_save() and z_restore() respectively. Both of these functions call get_default_filename() to determine what default filename to suggest to the user when prompting during a save or restore, passing in the address of a caller-provided filename if it exists. get_default_filename() looks like this:
/*
* get_default_name
*
* Read a default file name from the memory of the Z-machine and
* copy it to a string.
*
*/
static void get_default_name (char *default_name, zword addr)
{
if (addr != 0) {
zbyte len;
int i;
LOW_BYTE (addr, len);
addr++;
for (i = 0; i < len; i++) {
zbyte c;
LOW_BYTE (addr, c);
addr++;
if (c >= 'A' && c <= 'Z')
c += 'a' - 'A';
default_name[i] = c;
}
default_name[i] = 0;
if (strchr (default_name, '.') == NULL)
strcpy (default_name + i, ".AUX");
} else strcpy (default_name, f_setup.aux_name);
}/* get_default_name */If an address has been provided (the case that we’re interested in), it reads a byte from zmp[addr] and treats it as a length (len). Then it reads len characters, one per byte, lowercasing them. It null terminates the string and, if no file extension is present, it adds .AUX.
So the initial thing that baffled me here was that addr is supposed to be the address of a string containing a filename, and I thought that string literals on the zmachine were encoded as zstrings (3 chars per 16-bit word), not as string arrays with a length byte followed by a sequence of one-byte characters. Pursuing this idea, I figured out how the suggested filename in my program would be encoded as a zstring and took a look at location addr in the zmachine memory and did not find the zstring that I had computed and was expecting.
I was at a loss until I reread the sections of the zmachine spec on strings and memory addressing and came up with the hypothesis that addr was a packed address rather than a byte address. I took a look at 4*addr (since I was testing on a z5 file), and there I discovered the zstring corresponding with the filename that the test program had passed to @save/@restore.
So, to summarize, get_default_name is doing 2 things wrong:
- treating ‘addr’ as a byte address rather than a packed address, and
- decoding the string found at ‘addr’ as if it were a string array rather than a zstring.
This behavior leads to the corrupted filenames and crash that we saw. If the “length” byte that the function erroneously reads has a value greater than MAX_FILE_NAME (80), it will copy data beyond the bounds of the stack-allocated default_name array that z_save() or z_restore() passed in. Regardless, it will be copying len garbage bytes into the array to act as the filename.
I implemented a rough draft of a fix to confirm these conclusions. The diffs are attached at the bottom of this post.
The main idea behind it is to make decode_text() in text.c (and its associated string type enum) public and use it from get_default_name().
I did a couple of things with decode_text that I probably wouldn’t do in a “real” fix.
First, I added a FILENAME string type to the enum that means “this is a high string and you should decode it to memory (as with the VOCABULARY type) rather than printing it out.” I think that this is broken and that string type and whether to decode to memory or the screen are coupled in a way that they shouldn’t be, but it was expedient.
Second, I added a global char *, defined in fastmem.c and visible in text.c, that decode_text() uses for FILENAME strings in a similar way as it uses the static ‘decoded’ buffer in text.c for VOCABULARY strings. I think that changing decode_text() to accept a buffer as an optional argument would be better, but I wanted to minimize changes for this prototype fix.
Here’s a transcript of the test program running with the patch applied (again, saving a file and then reading it).
Here, we see that frotz correctly offers “mysavefile” as a default file name and completes the @save and @restore without crashing.
Please let me know what you guys think and if I’m overlooking anything here.
decode-patch.txt (4.22 KB)