[fixed] Deadbridge Chant crash during upkeep

[gnomefry]

Describe the Bug:

Deadbridge Chant crashes the game on controller's next upkeep after it comes into play.

I'll include a savegame from one turn prior, the dump txt and a screenshot of the bad parameters message that displays during upkeep (after clicking OK to this message the game CTDs)

Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)

Holidays 2021 ad1a6f8 - gauntlet

What exactly should be the correct behavior/interaction?

No crash

Are any other cards possibly affected by this bug?

[drool66]

It looks like it's due to Master Biomancer - it's responding to every event with a call to is_what(affected_card_controller, affected_card, TYPE_CREATURE) - in this case it's EVENT_REMOVED_FROM_GRAVEYARD and affected_card is a graveyard source value. Boom.
Fixed with a check for event == EVENT_CAST_SPELL || event == EVENT_ENTERING_THE_BATTLEFIELD_AS_CLONE || event == EVENT_CHANGE_TYPE

[Korath]

You're going to find there's a whole lot more cards just waiting to crash in exactly the same way. It's not quite as bad as with {player,card}, where practically everything calls get_card_instance() or is_humiliated() before looking at event, but it's close.

Shandalar can define events that accept values in {player,card}, {affected_card_controller,affected_card}, and {attacking_card_controller,attacking_card} that aren't card_instance indices because its card functions never look at any of these parameters without checking event first. Religiously. And it's tested for (directly in shandalar.cpp:run_tests()) so that if I let one slip through - as I tend to do with modal spells in particular - I know it immediately after building.

You don't have that option in Manalink because so very few cards check event before trying to dereference. You can't even test for it effectively because some of the functions doing it fail silently for a subset of invalid values (like is_humiliated() does if player or card is exactly -1) or, horrifically, encode alternate semantics for certain invalid values (like how is_what() considers card to be an iid_t instead of a card if player is exactly -1).

[gnomefry]

Thank you, Drool66 and Korath. I also see a crash on the first upkeep after Debtors' Knell comes into into play. Will the Deadbridge fix take care of that, too, or should I give it a separate write-up?

[drool66]

The problem isn't with Deadbridge Chant or Debtors' Knell. When something happens in a game, "events" can be dispatched, sometimes to all cards, sometimes to one card or a subset of cards. When that happens, there is other information that can be accessed, such as affected_player, affected_card, attacking_player/_card, etc. That data can be an actual player/card (every card in play or in hand is an "instance" that can be otherwise interpreted from two numbers - "player" and "card"), or it may be other data, depending on the event. If you try to interpret an "instance" from a pair of "player"/"card", and "player" isn't 1 or 0, or "card" isn't somewhere between 0-150, you get an error like the one you see. Other cards see these events and interpret the associated data, but they can be poorly coded by checking a "player" and "card" and trying to interpret an instance from them before they check what the event is. When you return a card from the graveyard, the "affected_card" datum isn't an actual card, but a string of data for the card being returned, which can be something as high as 2^32-1. Master Biomancer and (for the second bug you're reporting here) Enchanted Evening attempt to interpret an instance from affected_card_controller / affected_card before they check "event" (so they try to interpret an instance on every event). Many, many cards incorrectly do this same thing, which is what Korath was talking about above.

Fixed Enchanted Evening as well, pushing these soon.

Putting the check for these high on my to do list.

[Korath]

Updating card functions to handle this data safely is going to be a very long, painful, and error-prone process. It was in Shandalar, even though I'd planned for it almost from the start, I can test for it fairly reliably, and a lot of the cross-card concerns exacerbating this in Manalink like is_humiliated() aren't checked on a per-card level in Shandalar.

The short- to medium-term fix is to change the events - I think there's only a handful of them - so they only pass arbitrary data in safe parameters. Right now the only generic ones are event_result and event_param1 (which can be set and read at the same point event_result is, i.e. after push_affected_card_stack() and before pop_affected_card_stack(), in a function similar to dispatch_event_with_attacker_and_initial_event_result()). If those aren't enough, you can either add more globals that you save and restore yourself around the event dispatch like in gain_life_impl(), or replace push_affected_card_stack(), pop_affected_card_stack(), and their underlying affected_card_stack[] (which nothing else inspects) to add more generic parameters.

This isn't to say that there aren't other benefits from fixing the individual card functions. As an illustrative example, Aspect of Wolf not only calls get_card_instance() (twice), is_humiliated(), and get_hacked_subtype() for every event, but count_subtype() at O(n) too, just on the off chance it's needed for an EVENT_POWER or EVENT_TOUGHNESS affecting whatever it's attached to.

[drool66]

Fixed in 0d808dc