ParseMQ2DataPortion() Usage and Issues

[teabag]

I've been trying to keep the updated MQ2BuffQueue Plugin standardized to MQ2. I've noticed some code
bouncing around plugins setting "NULL" by hand and returning true (in the event of TypeMembers not found
and the like). However MQ2 also has support for this within ParseMacroData(). Getting this to work as
intended seems to be another story; I'm looking for some clarity.

MQ2DataAPI.cpp L465

Code: Select all

if (!Result.Type->GetMember(Result.VarPtr,pStart,pIndex,Result))
{
	if (!Result.Type->FindMember(pStart) && !Result.Type->InheritedMember(pStart))
	{
		MQ2DataError("No such '%s' member '%s'",Result.Type->GetName(),pStart);
	}
	return FALSE;
}

/echo ${BuffQueue.Blah}
*CRASH* - the crash is happening on the FindMember() call. (debug info below)


My breakdown thus far ..

You cannot return TRUE from a dataXXX routine unless the Type is set.

What is minimum requirements of the Ret.Ptr within the datablock? Obviously it points to our data; but what if we have no dynamic data and point to specific data per typemember. Most plugins that work under this need will set DWord=1 however they also memcpy "NULL" before a TRUE return; presumably to avoid this crash problem. This works - but I want whats right.

I don't doubt this is something on my end ; but seeing as so many others are trying to work around this problem as I have, I felt posting on it in the hopes of resolving the "Why's" might be of benefit.

Relevant to my code:

Code: Select all

if (!Result.Type->GetMember(Result.VarPtr,pStart,pIndex,Result))
	if (!Result.Type->FindMember(pStart) && !Result.Type->InheritedMember(pStart))

pStart = "Blah" (TypeMember) which does NOT exist .
pIndex = 0;     ( [] Index ) ConfigType does not use an index.

// Cutout of relevant code ${BuffQueueConfig.Enabled}
BOOL dataBuffQueueCfg(PCHAR szIndex, MQ2TYPEVAR &Ret)
{
  Ret.Ptr=0;           // We don't need any Indexes; only a Typemember.  however
                       // this _can become a pointer to ToString() no?
  Ret.Type=pBuffQueueConfigType;
  return true;
}
// ${BuffQueue[username].ID}
BOOL dataBuffQueue(PCHAR szIndex, MQ2TYPEVAR &Ret)
  if (ISINDEX())
    if (! ISNUMBER() )
      if (Ret.Ptr=GetBQQueueByName( GetSpawnByName( GETFIRST()), NULL)) {
        Ret.Type=pBuffQueueType;
        return true;
      }
  return dataBuffQueueCfg(szIndex,Ret);
}

I am aware the above might look wrong; calling one datablock from another, and it might be. This crash happens calling the Config datatype directly as well.

Code: Select all

[MQ2]MQ2BuffQueueConfigType::GetMember(Blah)()
(690.b90): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
MQ2Main!std::_Tree<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,unsigned long>,std::map<std::basic_string<char,std::char_traits<char>,std::alloca38fa3d6f+0x46:
00000000`02f304f6 8b4610 mov eax,[esi+0x10] ds:002b:00000000`00000010=????????
0:000:x86> r
eax=00000001 ebx=00000000 ecx=00000000 edx=00000004 esi=00000000 edi=00127e00
eip=02f304f6 esp=00127d78 ebp=0b7a04e0 iopl=0         nv up ei ng nz na pe cy
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010283
MQ2Main!std::_Tree<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,unsigned long>,std::map<std::basic_string<char,std::char_traits<char>,std::alloca38fa3d6f+0x46:
00000000`02f304f6 8b4610 mov eax,[esi+0x10] ds:002b:00000000`00000010=????????
0:000:x86> kv
ChildEBP          RetAddr           Args to Child                                         
00127db0 02f349cb 00127de0 00127e00 0012864c MQ2Main!std::_Tree<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,unsigned long>,std::map<std::basic_string<char,std::char_traits<char>,std::alloca38fa3d6f+0x46 (FPO: [Uses EBP] [2,11,4]) (CONV: thiscall) [C:\Program Files (x86)\Microsoft Visual Studio\VC98\INCLUDE\xtree @ 222]
00127e1c 02f347a3 00128682 00128687 0012aee1 MQ2Main!MQ2Internal::MQ2Type::FindMember+0xab (FPO: [Uses EBP] [1,20,4]) (CONV: thiscall) [C:\Documents and Settings\username\My Documents\MQ2\MQ2-Current\MQ2Main\MQ2Internal.h @ 636]
00128648 02f34bbd 00128678 00128670 001296d6 MQ2Main!ParseMQ2DataPortion+0x7f3 (FPO: [Uses EBP] [2,517,4]) (CONV: cdecl) [C:\Documents and Settings\username\My Documents\MQ2\MQ2-Current\MQ2Main\MQ2DataAPI.cpp @ 236]
00128e74 02f5b616 0012aed0 00000000 03058e80 MQ2Main!ParseMacroData+0x15d (FPO: [Uses EBP] [1,516,4]) (CONV: cdecl) [C:\Documents and Settings\username\My Documents\MQ2\MQ2-Current\MQ2Main\MQ2DataAPI.cpp @ 565]
00128e84 02f26069 109d0ae0 0012aed0 ffffffff MQ2Main!ParseMacroParameter+0x26 (FPO: [2,0,1]) (CONV: cdecl) [C:\Documents and Settings\username\My Documents\MQ2\MQ2-Current\MQ2Main\MQ2ParseAPI.cpp @ 125]
0012b6d8 005358c7 109d0ae0 0012bc4c 0000001c MQ2Main!CCommandHook::Detour+0x639 (FPO: [Uses EBP] [2,2574,4]) (CONV: thiscall) [C:\Documents and Settings\username\My Documents\MQ2\MQ2-Current\MQ2Main\MQ2CommandAPI.cpp @ 266]
WARNING: Stack unwind information not available. Following frames may be wrong.
00000000 00000000 00000000 00000000 00000000 eqgame+0x1358c7

PMQ2TYPEMEMBER FindMember(PCHAR Name)
{
  unsigned long N=MemberMap[Name]; <<<
  if (!N)
    return 0;
  N--;
  return Members[N];
}

xtree line: 222 (vb6)
template definition: _Pairib insert(const value_type& _V)

      _Ans = key_compare(_Kfn()(_V), _Key(_X)); <<<

[dont_know_at_all]

VS6?

[Lax]

I've been trying to keep the updated MQ2BuffQueue Plugin standardized to MQ2. I've noticed some code
bouncing around plugins setting "NULL" by hand and returning true (in the event of TypeMembers not found
and the like). However MQ2 also has support for this within ParseMacroData(). Getting this to work as
intended seems to be another story; I'm looking for some clarity.

Uhhhh... whoever is doing that should be shot, and they should fix their code.

You cannot return TRUE from a dataXXX routine unless the Type is set.

You can ONLY return TRUE from an object retrieval function (such as GetMember) if there is a valid object. A valid object, yes, requires both the value, AND the type. For example, if you want to give a string object containing "Boobies", this is a valid object, and can be done like so:

Code: Select all

Result.Ptr="Boobies";
Result.Type=pStringType;
return true;

If there is no valid object retrieved, you NEED to return FALSE. Nobody should be setting the data to the string "NULL" and returning true.

That's really the only rules: false if no valid object, true if valid object.

I'll explain it in the terms of the current implementation of the system, which is LavishScript (which is not present in MQ2). This part of "MQ2Data" translates 1 to 1.

What you're calling "the datablock" is called MQ2TYPEVAR in MQ2, or LSOBJECT in LavishScript. The MQ2 name is kind of ambiguous and confusing, but LSOBJECT is far more accurate. This structure is broken into exactly two pieces: The data (which is in the form of MQ2VARPTR in MQ2, or LSOBJECTDATA in LS), and the type (which is a pointer to an MQ2Type or LSTypeDefinition).

So:
MQ2Type=LSTypeDefinition
MQ2VARPTR=LSOBJECTDATA
MQ2DATAVAR=LSOBJECT

LSOBJECTDATA is a 32-bit value which can be interpreted in ANY way possible. It's just a number, without context. It might be an unsigned int, it might be a pointer to a string, it might be a pointer to a buff, it might be a pointer to a spawn, it might be a timestamp, it might be a pointer to some data structure you dont know, and so on. It can be ANYTHING. Nobody knows what it is until it is given context. Given a value 0x401000 you dont know if it's just a number, or if it's a pointer to a string, or just the number 4198400.
LSOBJECT is a LSOBJECTDATA, plus a LSTypeDefinition to give it context. This is the ONLY way the parser is going to be able to know how to access the value. Any time GetMember returns TRUE, there damn well better be a value and a context, because you're telling it there's a valid object+context.

So, when you're setting Ret.DWord (or Ret.Ptr, and so on), you're setting the object DATA. If you don't set Ret.Type, the return type is undefined, and it might even be pointing to a random memory location. If that's the case and you return TRUE, you're inviting a crash.

Reiterating:
GetMember should ONLY return TRUE if your operation was SUCCESSFUL and you have returned a valid VALUE and CONTEXT (type) for the value. If ANY of those is not TRUE, you MUST return FALSE.

[teabag]

I appreciate the informative response. While this is already what I expected; I was overanalysing it and losing track.

Uhhhh... whoever is doing that should be shot, and they should fix their code.

You might be suprised by how many plugins are currently doing this. After exploring the source, I expect they were saving time by doing it a way that was effective; handled by their own code. After my own research; the relevant code was a bit awkward to follow; not that anything is wrong with it. :)

Code: Select all

  // Wrong but works.  find the solution 
  strcpy(Temps,"NULL");
  Dest.Ptr=&Temps[0];
  Dest.Type=pStringType;
  return true;

Code: Select all

  Dest.DWord=1; 
  Dest.Type=pStructureType; 

This combination ; where pStructureType is NOT of type DWord; is very common amongst Plugins at this time.

In my case, I expected it might have been how I was changing MQ2TYPE within the GetMember() of another. After reexamining the source; and your information; as long as the MQ2VARPTR is pointing to something compatible with the MQ2TYPE (ie: MQ2SpawnType <> _SPAWNINFO), changing Type is acceptable. Effectively, All MQ2TYPE's should be accompanied by a typedef struct then? Dest.Ptr points to something of that typedef, Type points to the MQ2Type to handle that struct .. ?

This might seem like a trivial question; I'm not just asking for myself. I'm working under the assumption that others don't know; since others are doing it differently.

[Lax]

// Wrong but works. find the solution
strcpy(Temps,"NULL");
Dest.Ptr=&Temps[0];
Dest.Type=pStringType;
return true;

This only works by coincidence. This is absolutely invalid, unless you REALLY want to give a string containing NULL. In LavishScript, you can test if any given object exists by casting to the exists type (which simply always reduces to TRUE). The above would give TRUE, whereas it should give NULL. Giving "NULL" is not the responsibility of any GetMember or even TLO function. The parser gives NULL as soon as an object does not exist, as is explained many times over in the "MQ2Data reference". Not a string containing "NULL", just NULL.

This combination ; where pStructureType is NOT of type DWord; is very common amongst Plugins at this time.

That's perfectly acceptable, and it's done in MQ2 itself. Sometimes you only have one of any given type of object, such as math, macroquest, and so on, so it's not important to give the actual pointer to the data structure -- you know which one it is anyway.

. Effectively, All MQ2TYPE's should be accompanied by a typedef struct then? Dest.Ptr points to something of that typedef, Type points to the MQ2Type to handle that struct .. ?

Yes. The data is just a 32-bit value that can be accessed in the ways described by the given type. That's how it works in assembly, so that's how it's done in LavishScript.

[teabag]

Quote:
This combination ; where pStructureType is NOT of type DWord; is very common amongst Plugins at this time.

That's perfectly acceptable, and it's done in MQ2 itself. Sometimes you only have one of any given type of object, such as math, macroquest, and so on, so it's not important to give the actual pointer to the data structure -- you know which one it is anyway.

This however is where the crash seemed to be happening. Without returning "NULL" yourself a crash will occur in every case I have tried.
Perhaps circumstancial use is acceptable, however:

Code: Select all

if (!ParseMQ2DataPortion(szCurrent,Result) || !Result.Type->ToString(Result.VarPtr,szCurrent))
			strcpy(szCurrent,"NULL");

Anotherwards, if either is false, we strcpy().
Unless the first is false, the 2nd will be called.

Code: Select all

if (!Result.Type)
{
	if (PMQ2DATAITEM DataItem=FindMQ2Data(pStart))
	{
		if (!DataItem->Function(pIndex,Result))
		{
			return FALSE;
		}
	}
...

DataItem will be found, Function will be true.
We then parse . and the TypeMember

Code: Select all

	if (!Result.Type->GetMember(Result.VarPtr,pStart,pIndex,Result))
	{
		if (!Result.Type->FindMember(pStart) && !Result.Type->InheritedMember(pStart))
		{
			MQ2DataError("No such '%s' member '%s'",Result.Type->GetName(),pStart);
		}
		return FALSE;

Result.Type->GetMember will be false
Result.Type->FindMember(pStart) would in theory be false, but we crash here doing the map lookup ...pStart is valud text but not a valid TypeMember ..

And as a note:

.. (MQ2TypeType::)

Code: Select all

   bool ToString(MQ2VARPTR VarPtr, PCHAR Destination)
   {
	  strcpy(Destination,((MQ2Type*)VarPtr.Ptr)->GetName());
      return true;
   }

VarPtr.DWord=1
if (VarPtr.Ptr->GetName())

We can call VarPtr.Ptr->GetName() when it has not been specified?

Ok, I'll admit it; I ooze rust when it comes to coding, but something just isn't right unless I'm missing something, which is quite likely I'll admit.. :)

[Lax]

Result.Type->GetMember will be false
Result.Type->FindMember(pStart) would in theory be false, but we crash here doing the map lookup ...pStart is valud text but not a valid TypeMember ..

My bad. I will SLIGHTLY change what I said earlier.

1. Never return TRUE unless you have both a valid object, and a valid context (object type).
2. Never set the context (object type) without a valid object.

We can call VarPtr.Ptr->GetName() when it has not been specified?

Absolutely not. You can call MQ2Type::GetName() on a valid type, which is what the call you're asking about is doing. It is known to be valid in the ToString call. For the type type, the VALUE is the pointer to an MQ2Type (the data is discarded by the cast to the type type).

This all works perfectly fine as demonstrated in MQ2 itself (and even LavishScript), I dont nkow where someone got the idea that they should do it any differently than is plastered all over the MQ2 code...

[teabag]

We can call VarPtr.Ptr->GetName() when it has not been specified?

Absolutely not.

I know, that was my cynical point :). If I'm reading the code right, the only way you can set Ret.DWord=1 is if your SURE that ParseMacroDataPortion() will return FALSE. If it returns TRUE, the right side of the || will be called; causing the Ret.Ptr->ToString() to be called with an invalid Ret.Ptr value.

Of course calling ParseMacroDataPortion() with the intent of returning false is useless unless your GetMember() changes the contents of the Ret.Ptr and Ret.Type at some point. (it must or crash)

In effect, setting Ret.DWord (or anything ! Ptr ) should only be done if GetMember() is guaranteed to change the values of the MQ2TYPEVAR or return false.

I believe this is related to whats happening for me; although I'm still unclear how a crash is happening on the map lookup within FindMember(), Particularly with what appears to be a valid char* .

[Lax]

I know, that was my cynical point :). If I'm reading the code right, the only way you can set Ret.DWord=1 is if your SURE that ParseMacroDataPortion() will return FALSE. If it returns TRUE, the right side of the || will be called; causing the Ret.Ptr->ToString() to be called with an invalid Ret.Ptr value.

ParseMacroDataPortion has nothing to do with it, you dont need to read that code -- It will return FALSE if GetMember returns FALSE. Changing the data value also has nothing to do with it. Your cynical point involving the type type's ToString value was irrelevant to this discussion.

Of course calling ParseMacroDataPortion() with the intent of returning false is useless unless your GetMember() changes the contents of the Ret.Ptr and Ret.Type at some point. (it must or crash)

Why would you call the function with the intent of returning false? The point is that ParseMQ2DataPortion takes an arbitrary data sequence and turns it into an object, in the form of Result. If it returns false, there is no object in order to reduce to text. If it returns true, then any TLO, member function, etc is expected to have returned TRUE and set the object accordingly. The only way the second half of the if statement runs is if there is a final object. Thusly, if there is a final object, the second half retrieves the "to string" from the given object type, with the given object. If there is no object, or "to string" failed, then the reduced value is NULL.

I've already explained the rules, I'm not sure why questions remain (or at least, why the same question is being asked over and over). If you return TRUE without setting both the object and the context, you are setting up a crash. If you change the context without returning TRUE, you are setting up a crash. These are rules 1 and 2 as explained in my last post.

[teabag]

-- It will return FALSE if GetMember returns FALSE. Changing

Unfortunately its not in this case - that was my first question; and problem.

If you change the context without returning TRUE, you are setting up a crash.

oh? That wouldn't seem to make sense to me, but i'll look into it on my own time.

My appologies, I didn't mean to offend, I'm simply trying to solve 2 problems; one being crashes within FindMember(); which rather than create issues; I'll deal with on my own. The second proper handling of this code. The latter of which is not being done by the vast majority of the Plugins for MQ2 per your pointing out returning "NULL". And I agree; your response in this regard puts that question to closure.

[Lax]

-- It will return FALSE if GetMember returns FALSE. Changing

Unfortunately its not in this case - that was my first question; and problem.

Yes it is

If you change the context without returning TRUE, you are setting up a crash.

oh? That wouldn't seem to make sense to me, but i'll look into it on my own time.

Re-read this line:

Code: Select all

if (!Result.Type->GetMember(Result.VarPtr,pStart,pIndex,Result)) 

Notice that it uses Result as both input and output. Then the next line uses Result.Type -- which is assumed to be valid at all times -- but does not access the data itself, because that is not assumed to be valid.

You quoted the part that deals with GetMember, and it is clearly returning FALSE if GetMember returns false. How is what I said "not the case"? Obviously, you are either returning true, or modifying the type such that it does not point to a valid type definition. (note: the given crash info in the original post indicates that the type pointer may be invalid, such as after the type object has been destroyed)

[eqjoe]

-- It will return FALSE if GetMember returns FALSE. Changing

Unfortunately its not in this case - that was my first question; and problem.

If you change the context without returning TRUE, you are setting up a crash.

oh? That wouldn't seem to make sense to me, but i'll look into it on my own time.

My appologies, I didn't mean to offend, I'm simply trying to solve 2 problems; one being crashes within FindMember(); which rather than create issues; I'll deal with on my own. The second proper handling of this code. The latter of which is not being done by the vast majority of the Plugins for MQ2 per your pointing out returning "NULL". And I agree; your response in this regard puts that question to closure.

Dude..... You seem to be a nice enough guy, but why oh why would you argue with the guy the wrote the damn thing?

Noobs! Who needs em?

-j

[teabag]

I'm not arguing, I'm trying to clarify a topic since the majority of Plugins in VIP and public are doing exactly what Lax said is wrong. :)

[Lax]

Unfortunately people copy from each other's crap, assuming it's right, instead of doing what is demonstrated in examples that actually are right (e.g. built into MQ2, or in official plugins). Someone apparently made something up, it seemed to work, or solved some problem that they thought existed but really didnt exist. I would assume it started from someone who was trying to do "something.Equal[NULL]" when that's never been valid to do, and then people ended up copying them.

I've never made a habit of reviewing MQ2 plugins for correctness, and I dont plan on starting anytime soon

[teabag]

Nor is it expected imo. Let you focus on what you do best. This however is why I made this post. To clarity what is right for those that may not know. Your the best person to answer such questions; not many others are in a position to.

I've spent 3 weeks doing Code review so I could do just what you said; do it right by whats already in MQ2.

If you return TRUE without setting both the object and the context, you are setting up a crash. If you change the context without returning TRUE, you are setting up a crash. These are rules 1 and 2 as explained in my last post.

This is a solid General rule to work with; and hopefully clarifies it for people. I was merely trying to inquire as to why I was having a crash on a FindMember() command with an apparently valid parameter (see debug info in my origional post. I'll sort this out on my own; I've taken up enough of your time; my primary concern was clarification to resolve the "NULL" returns by plugins.

Thank you.