[fixed]Ring of Xathrid crash upon equipping token

[gnomefry]

Describe the Bug:

The game crashes when attempting to equip a creature token with Ring of Xathrid

Which card behaved improperly?

Ring of Xathrid

Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
version 10-2020 ee04575f - gauntlet

What exactly should be the correct behavior/interaction?

It can equip creature tokens

Are any other cards possibly affected by this bug?

Unknown

[drool66]

Confirmed in your save. Doesn't cause a problem in a clean game equipping spirit tokens from Abzan Ascendancy, nor the few other tokens I tried it on.

[Korath]

Without debug data, I can't tell for sure if this is the specific problem here; but causing an equipment to leave the bf after activating it to equip but before resolution consistently segfaults. The Sundering Growth in the AI's hand is sufficient, if it tries to destroy your Ring of Xathrid in response.

The problem is that equip_target_creature() doesn't verify the equipment is still valid before calling call_card_function(...EVENT_EQUIPMENT_ATTACHED) with it. The quick fix is to change call_card_function() and call_card_function_i() to check if instance->internal_card_id == -1, and use instance->backup_internal_card_id instead if so.

call_card_fn() should really pop up a backtrace the way get_card_instance() does if it's called with address==NULL or something else that's not in Magic.exe's or ManalinkEh.dll's ranges, too.

[drool66]

Did all the above, which included moving the show_backtrace() declaration to manalink.h. I wasn't sure if "SetVoidPtr already_displayed" should be declared globally or not (or maybe the card_instance_t and call_card_function stuff moved to the same file), so call_card_fn looks in part like this:

Code: Select all

SetVoidptr has_been_displayed = NULL;

int call_card_fn_impl(CardFunction address, card_instance_t* instance, int player, int card, event_t event);   // never call directly
// Puts instance into esi, then calls address(player, card, event).
int call_card_fn(CardFunction address, card_instance_t* instance, int player, int card, event_t event)
{

  if ((card < 0 || card >= 150 || player < 0 || player > 1)
     && SetVoidptr_insert(&has_been_displayed, __builtin_return_address(0)))
   {
     char buf[100];
     sprintf(buf, "call_card_fn(%d, %d)", player, card);
     show_backtrace("bad parameters", buf);
   }

  int old_trigger_condition = trigger_condition;
  etc...

[Korath]

Checking player and card might make sense in call_card_function(), if we'd been seeing that called with bad values for them, but we haven't been.

On the other hand, it's not necessarily incorrect to call call_card_fn() with player or card outside their normal ranges, or even with instance being NULL. (get_pt_for_creature_with_variable_pt(), for example, does two of those three.) Its only precondition is that address is a callable function pointer. The closest we can come to testing that - short of just trying to call it anyway and segfaulting, like we currently are - is to check it's in a sane range: either between 0x401000 and 0x4ea000 for Magic.exe, or between 0x2001000 and &etext for ManalinkEh.dll.

edit: You've got to declare etext weird in 32-bit Windows builds, since the ABI normally prepends symbols with an underscore:

Code: Select all

extern char etext __asm__("etext");

Also no need to track which addresses have been found to be invalid, since it's about to crash anyway.

[drool66]

Thanks for that, probably saved me about an hour.

So this:

Code: Select all

SetVoidptr has_been_displayed = NULL;
extern char etext __asm__("etext");

int call_card_fn_impl(CardFunction address, card_instance_t* instance, int player, int card, event_t event);   // never call directly
// Puts instance into esi, then calls address(player, card, event).
int call_card_fn(CardFunction address, card_instance_t* instance, int player, int card, event_t event)
{

  if ((address < (CardFunction)0x401000 || (address > (CardFunction)0x4ea000 && address < (CardFunction)0x2001000) || address > (CardFunction)&etext))
   {
     char buf[100];
     sprintf(buf, "call_card_fn(%p)", address);
     show_backtrace("bad parameters", buf);
   }

Gives me an etext value of 2579F8c which seems pretty reasonable to me for a final address.

[drool66]

Committed in d96155c