Re: Bug when running Uninvited

[Dannii Willis]

As I said here (http://www.intfiction.org/forum/viewtopic.php?f=38&t=7006), can you please try reducing your sourcecode to a minimal example with the bug?

On Thursday, March 21, 2013 9:43:55 PM UTC+10, David Griffith wrote:

There's a bug in how Parchment works with Uninvited and I'd like to get to the bottom of what the problem is.  The trouble can be seen by visiting http://iplayif.com/?story=http%3A%2F%2Fwww.ifarchive.org%2Fif-archive%2Fgames%2Fzcode%2Funinvited.z5.  Notice that instead of "glovebox", you see "glovebo".  All dictionary items seem to have their names truncated to five characters.  This doesn't happen with other interpreters.  I'm sure the bug is being tickled by some change made in the Inform6 compiler or library.

[David Griffith]

Oops.  I forgot about that post.

Checking further, I see that Parchment has no problem with something compiled with 6.31 and 6.32. With 6.33 beta1, that's when the dictionary name bug appears.  Below are links to the test code and three binaries.

http://661.org/if/ptest.inf

iplayif.com/?story=http://661.org/if/ptest631.z5

iplayif.com/?story=http://661.org/if/ptest632.z5

iplayif.com/?story=http://661.org/if/ptest633.z5

[Andrew Plotkin]

On Thu, 21 Mar 2013, David Griffith wrote:

> Checking further, I see that Parchment has no problem with something

> compiled with 6.31. It refuses to load something compiled with 6.32. With

> 6.33 beta1, that's when the dictionary name bug appears. Below are links
> to the test code and three binaries.

Your ptest632.z5 loads fine in Parchment for me.

Your ptest633.z5 displays the bug. This is the only version that uses
6/12pre. As I just mentioned in the intfiction post, I see the same
problem when compiling with 6/12pre.

--Z

--
"And Aholibamah bare Jeush, and Jaalam, and Korah: these were the borogoves..."
*

[Matthew Wightman]

On Mar 21, 8:14 pm, Andrew Plotkin <zgoo...@eblong.com> wrote:
> Your ptest633.z5 displays the bug. This is the only version that uses
> 6/12pre. As I just mentioned in the intfiction post, I see the same
> problem when compiling with 6/12pre.

Bisecting, the change to the library that results in the change in
behaviour happened in commit 2b2e4f313423fcc41c659fa9be8344d51884827a
"Prevention of buffer overflow in PrefaceByArticle()":

=====
@@ -6489,11 +6491,21 @@ Array StorageForShortName -> WORDSIZE +
SHORTNAMEBUF
#Endif; ! LanguageContractionForms

#Ifdef TARGET_ZCODE;
- if (standard_interpreter ~= 0 && findout) {
- StorageForShortName-->0 = SHORTNAMEBUF_LEN;
+ if (standard_interpreter && findout) {
+ i = (HDR_OBJECTS-->0+124+o*14)-->0; ! Property address of
object
+ findout = 0; ! To find the first char
of the
+ ! object's short-name, we
need
+ ! (worst case) to unpack
the fi
! two words
+ if (i->0 > 2) { ! If more than two
words...
+ i++; ! Start of packed name
+ findout = i-->1; ! Save second word for
restorat
i-->1 = i-->1 | WORD_HIGHBIT; ! then arbitrarily truncate
+ }
@output_stream 3 StorageForShortName;
if (pluralise) print (number) pluralise; else print (PSN__)
o;
@output_stream -3;
+ if (findout) i-->1 = findout; ! Restore truncated short-
name

=====

The problem appears to be that assigning the old value of i-->1 back
into i-->1 isn't un-truncating the name?

[David Griffith]

On Thursday, March 21, 2013 1:14:22 PM UTC-7, Andrew Plotkin wrote:

On Thu, 21 Mar 2013, David Griffith wrote:

> Checking further, I see that Parchment has no problem with something
> compiled with 6.31.  It refuses to load something compiled with 6.32. With
> 6.33 beta1, that's when the dictionary name bug appears.  Below are links
> to the test code and three binaries.

Your ptest632.z5 loads fine in Parchment for me.

Your ptest633.z5 displays the bug. This is the only version that uses
6/12pre. As I just mentioned in the intfiction post, I see the same
problem when compiling with 6/12pre.

The refusal to load seems to have been a silly goof on my part.

[Dannii Willis]

Thanks for chasing down the change Matthew! I wasn't sure if this was a Parchment/ZVM bug or not, because it works in Gnusto: http://iplayif.com/?story=http://661.org/if/ptest633.z5&vm=gnusto

After this, I'm still not sure.

[David Griffith]

This change was originally made by Roger Firth back in December of 2004.  (see  79451739094357c0e80f72fb977f6ced94023f87 in the inform-2006 repo).  I can't make any sense of it regarding how to fix the problem.

[Andrew Plotkin]

On Thu, 21 Mar 2013, David Griffith wrote:

>> The problem appears to be that assigning the old value of i-->1 back
>> into i-->1 isn't un-truncating the name?
>>
>
> This change was originally made by Roger Firth back in December of 2004.
> (see 79451739094357c0e80f72fb977f6ced94023f87 in the inform-2006 repo). I
> can't make any sense of it regarding how to fix the problem.

It looks like this is altering (and then restoring) the contents of the
object property table. This sounds like a terrible idea on the face of
things, and the code looks buggy on top of that -- what if i-->1 is zero
to begin with? (It will be altered but not restored.) Why is this even
performing surgery on the object's hardware name, when PSN__(o) is more
likely to print the short_name property?

I do not like this patch.

[Dannii Willis]

On Friday, 22 March 2013 14:42:30 UTC+10, Andrew Plotkin wrote:

It looks like this is altering (and then restoring) the contents of the
object property table. This sounds like a terrible idea on the face of
things, and the code looks buggy on top of that -- what if i-->1 is zero
to begin with? (It will be altered but not restored.) Why is this even
performing surgery on the object's hardware name, when PSN__(o) is more
likely to print the short_name property?

Are you saying that it's modifying the short name in the property table header? (Spec section 12.4)

If that's the case, then it's probably running into trouble with ZVM caching that text. This would be the first case where the caching breaks things. Now it's my responsibility to fix such a bug, but I'd also recommend leaving the property table header alone.

[David Griffith]

On Thursday, March 21, 2013 9:42:30 PM UTC-7, Andrew Plotkin wrote:

On Thu, 21 Mar 2013, David Griffith wrote:

>> The problem appears to be that assigning the old value of i-->1 back
>> into i-->1 isn't un-truncating the name?
>>
>
> This change was originally made by Roger Firth back in December of 2004.
> (see  79451739094357c0e80f72fb977f6ced94023f87 in the inform-2006 repo).  I
> can't make any sense of it regarding how to fix the problem.

It looks like this is altering (and then restoring) the contents of the
object property table. This sounds like a terrible idea on the face of
things, and the code looks buggy on top of that -- what if i-->1 is zero
to begin with? (It will be altered but not restored.) Why is this even
performing surgery on the object's hardware name, when PSN__(o) is more
likely to print the short_name property?

I do not like this patch.

Do you remember what bug Roger was attempting to fix?

[Fredrik Ramsberg]

I don't know about a bug id, but it looks like the code change is there to prevent a buffer overrun when printing an object name which is more than 160 characters long. Since the code only needs the very first character of the name anyway, it truncates the name string after the second word of encoded text.

/Fredrik

 

[David Griffith]

So, should I revert the relevant code in the Library or wait for a fix to Parchment?

[Dannii Willis]

I have fixed some bugs in Parchment, and your code works.

You should still consider reverting the library code however, as quite a number of people said it was bad.

[David Griffith]

 
Removed.