Jump to content

Photo

ghost.zh


  • Please log in to reply
636 replies to this topic

#631 ZoriaRPG

ZoriaRPG

    The Timelord

  • ZC Developers
  • Gender:Unspecified
  • Location:Prydon Academy

Posted 15 July 2019 - 09:16 AM

tempGhostData is created just before it's assigned, at the beginning of Ghost_WaitframeLight(), and the pointer is cleared at the end of that function. The array only exists until the script resumes in the next frame; Set and GetEnemyProperty() are meant to be called from other scripts during that time.

It's a bit complicated, but there's nothing out of the ordinary going on there. As long as array pointers are still integers, it should be fine as long as they're not greater than 65535.


Ah-ha.

Moosh filed a report, bringing it to my attention, and when I tried using it within a ghost script to test the issue, I could easily replicate it.

Essentially, it doesn't have proper access to the pointer that it is trying to use, if called at the wrong time, and it floods the log with invalid array errors.

The behaviour is still spotty. If it's only intended to be used after the array generated by ghost_waitframe is called, that's fine-enough.

I think it breaks when a script with a higher priority than the ghosted ffc enemy that you are trying to affect calls it, as at that time the array pointer isn't established.


I sanity guarded it as follows:
 

void SetEnemyProperty(npc enemy, int property, float newValue)
{
	//LogPrint("Setting ghost property: %d\n", property);
	
	if((enemy->Misc[__GHI_NPC_DATA]&0x10000)!=0)
	{
		float ptr=enemy->Misc[__GHI_NPC_DATA]&0xFFFF;
		//LogPrint("ptr is: %f \n", ptr);
		
		if ( ptr > 0 )
		{
			//LogPrint("ptr size is: %f \n", SizeOfArray(ptr));
			if ( SizeOfArray(ptr) == 24 )
			{
				//TraceS("SET Pointer validated.\n");
				ptr[property]=newValue;
			}
		}
		//else TraceS("SET Pointer not validated.\n");
	}
	
	switch(property)
	{
		case ENPROP_X: enemy->X=newValue; return;
		case ENPROP_Y: enemy->Y=newValue; return;
		case ENPROP_Z:  enemy->Z=newValue; return;
		case ENPROP_JUMP: enemy->Jump=newValue; return;
		case ENPROP_DIR: enemy->Dir=newValue; return;
		case ENPROP_HP: enemy->HP=newValue; return;
		default: enemy->CSet=newValue; return;
	}
}

float GetEnemyProperty(npc enemy, int property)
{
	if((enemy->Misc[__GHI_NPC_DATA]&0x10000)!=0)
	{
		float ptr=enemy->Misc[__GHI_NPC_DATA]&0xFFFF;
		//LogPrint("ptr is: %f \n", ptr);
			
		if ( ptr > 0 )
		{
			//LogPrint("ptr size is: %f \n", SizeOfArray(ptr));
			if ( SizeOfArray(ptr) == 24 )
			{
				return ptr[property];
			}
		}
	
		else
		{
			//TraceS("GET Pointer not validated.\n");
			switch(property)
			{
				case ENPROP_X: return enemy->X;
				case ENPROP_Y: return enemy->Y;
				case ENPROP_Z: return enemy->Z;
				case ENPROP_JUMP: return enemy->Jump;
				case ENPROP_DIR: return enemy->Dir;
				case ENPROP_HP: return enemy->HP;
				default: return enemy->CSet;
				
			}
		}
	}
    
	else
	{
		switch(property)
		{
			case ENPROP_X: return enemy->X;
			case ENPROP_Y: return enemy->Y;
			case ENPROP_Z: return enemy->Z;
			case ENPROP_JUMP: return enemy->Jump;
			case ENPROP_DIR: return enemy->Dir;
			case ENPROP_HP: return enemy->HP;
			default: return enemy->CSet;
			
		}
	}
}
I used switch-case here purely for efficiency. I was thinking of going in and making the whole header more efficient in 2.55 (ZASM output), but this same if statements would work in 2.50.x, to verify the array pointer, prior to trying to access it.

#632 Saffith

Saffith

    IPv7 user

  • ZC Developers
  • Gender:Male

Posted 15 July 2019 - 10:02 AM

It already does that. That's what if((enemy->Misc[__GHI_NPC_DATA]&0x10000)!=0) is for. But I do see how an invalid access might be possible; AutoGhost() sets enemy->Misc[__GHI_NPC_DATA] to 1 briefly so it's nonzero without being marked as in use. I'll see about fixing that.

Edit: Wait, no it doesn't. Apparently, I forgot how numbers work. Well, I'll deal with that, then.

#633 ZoriaRPG

ZoriaRPG

    The Timelord

  • ZC Developers
  • Gender:Unspecified
  • Location:Prydon Academy

Posted 16 July 2019 - 09:32 AM

It already does that. That's what if((enemy->Misc[__GHI_NPC_DATA]&0x10000)!=0) is for. But I do see how an invalid access might be possible; AutoGhost() sets enemy->Misc[__GHI_NPC_DATA] to 1 briefly so it's nonzero without being marked as in use. I'll see about fixing that.

Edit: Wait, no it doesn't. Apparently, I forgot how numbers work. Well, I'll deal with that, then.

 

In some of my tests, the value of that Misc index was -1. I'm not too familiar with the internals of ghost.zh, but I found this quite off. This bypassed the nonzero check, too. It seems that you went only with ptr>0, which still doesn't validate that the value is an actual array. (SizeOfArray() does, however ensure that it is an array. I should probably add an internal IsValidArray() at some point.)

 

Anyway, see my comments on the header in the database.



#634 Saffith

Saffith

    IPv7 user

  • ZC Developers
  • Gender:Male

Posted 16 July 2019 - 12:23 PM

In some of my tests, the value of that Misc index was -1. I'm not too familiar with the internals of ghost.zh, but I found this quite off.

There's nothing that should ever set it to a negative number. Where did you see that happen? Are you sure it wasn't done by some other script?
 

SizeOfArray() does, however ensure that it is an array.

But in 2.50, at least, it also logs an error if it's not. That's not something a validation function should do.

#635 ZoriaRPG

ZoriaRPG

    The Timelord

  • ZC Developers
  • Gender:Unspecified
  • Location:Prydon Academy

Posted 17 July 2019 - 03:42 AM

There's nothing that should ever set it to a negative number. Where did you see that happen? Are you sure it wasn't done by some other script?
 
But in 2.50, at least, it also logs an error if it's not. That's not something a validation function should do.

 
I had a series of test scripts that were setting it, and indeed, it was from another script. Moosh was trying to do thst, and it probably was never meant to work that way.
 
Ah, yes, it'll log the same array pointer issue, at ArrayH::getArray(), because of this block:
 
 
 else
        {
            if(localRAM[ptr].Size() == 0)
                return InvalidError(ptr);
 
I'm going to need to add isValidArray(untyped ptr) at some point.

How do you feel about making github repos for the ghost and tango headers? If you have archives of the different versions, I'd be willing to set it up so that each version has a commit range, and so that it'd be possible to compare against versions as I have done with ZC and std.zh.

#636 Saffith

Saffith

    IPv7 user

  • ZC Developers
  • Gender:Male

Posted 17 July 2019 - 11:48 AM

Other than fixing bugs, I'm done with them. If you want to make repos and keep working on them, go ahead. I do still have most of the older versions.
I'll be glad to send you the documentation sources, too, though there are bits of Tango's I was rewriting for the HTML version, and I'd still like to get that done first.

#637 ZoriaRPG

ZoriaRPG

    The Timelord

  • ZC Developers
  • Gender:Unspecified
  • Location:Prydon Academy

Posted 18 July 2019 - 08:21 AM

Other than fixing bugs, I'm done with them. If you want to make repos and keep working on them, go ahead. I do still have most of the older versions.
I'll be glad to send you the documentation sources, too, though there are bits of Tango's I was rewriting for the HTML version, and I'd still like to get that done first.


Sure. Please ZIP up the different versions, as many as you have, and put them up somewhere.

HTML version of Tango?


1 user(s) are reading this topic

0 members, 1 guests, 0 anonymous users