Crash due to referencing object deleted by drop_near()

[calris]

Rather than clutter the Vanilla board, I'll discuss my findings here. Original thread is here

In summary, there is a repeatable crash if a monster picks up an object and that object is destroyed when the monster dies due to lack of floor space. The 'birth_stacking' flag seems to be ignored (the save file in the referenced thread has stacking on'.

In obj-pile.c in drop_near() there are a couple of chunks of code like this:

		/* Message */
		msg("The %s %s.", o_name,
			VERB_AGREEMENT(dropped->number, "disappears", "disappear"));

		/* Debug */
		if (player->wizard) msg("Breakage (no floor space).");

		/* Failure */
		if (dropped->known) {
			delist_object(cave_k, dropped->known);
			object_delete(&dropped->known);
		}
		delist_object(c, dropped);
		object_delete(&dropped);
		return;

and in delist_object() we have:

	/* Don't delist an actual object if it still has a listed known object */
	if ((c == cave) && cave_k->objects[obj->oidx]) return;

	c->objects[obj->oidx] = NULL;
	obj->oidx = 0;

I'm thinking that when an object is picked up by a monster, it's not being removed from the 'cave' completely.

In mon_move.c in process_monster_grab_objects(), the code which performs the pick-up is

square_excise_object(c, ny, nx, obj);
monster_carry(c, mon, obj);
square_note_spot(c, ny, nx);
square_light_spot(c, ny, nx);

square_excise_object() just calls pile_excise() - but nowhere does the code seem to touch cave_k->objects[obj->oidx]

Hopefully this helps track down this nasty

[Nick]

Originally Posted by calris

I'm thinking that when an object is picked up by a monster, it's not being removed from the 'cave' completely.

In mon_move.c in process_monster_grab_objects(), the code which performs the pick-up is

square_excise_object(c, ny, nx, obj);
monster_carry(c, mon, obj);
square_note_spot(c, ny, nx);
square_light_spot(c, ny, nx);

square_excise_object() just calls pile_excise() - but nowhere does the code seem to touch cave_k->objects[obj->oidx]

Hopefully this helps track down this nasty

OK, thanks for this - I think I have tracked it down, eventually. It's actually kind of the opposite of this. The process_monster_grab_objects() code is fine, but the drop_near() code needed a call to square_excise_object() to remove the about-to-be-deleted object from the place where the player thought it was...

So what actually happened was this:

1. Master Rogue looted the vault, in the process picking up a Ring of Resist Poison which the character had already detected
2. Master Rogue wanders into LoS and gets fire bolted
3. The masses of loot he has collected doesn't fit in the small space available, even with stacking, so stuff gets deleted, including the ring
4. The bug occurs - the character sees the ring drop, but the character's knowledge of the ring's new whereabouts doesn't update, and it's still recorded as sitting inside the vault room
5. While the map is being redrawn, a check is done of what the character thinks is there, and comes up with an invalid memory call, because it's been wiped.

Bug is fixed in development

[PowerWyrm]

No idea if it's related to this bug at all, since this happened in PWMAngband and not in Vanilla Angband, but the code is roughly the same. I was clearing a mixed n/M pit when the game crashed in drop_near() during monster death:

    /* Drop objects being carried */
    obj = mon->held_obj;
    while (obj)
    {
        next = obj->next;

        /* Object no longer held */
        obj->held_m_idx = 0;
        pile_excise(&mon->held_obj, obj);

        ...

        drop_near(p, c, obj, 0, mon->fy, mon->fx, true, DROP_FADE);
        obj = next;
    }

    /* Forget objects */
    mon->held_obj = NULL;

Traking down the problem in debug mode, it seems that the "obj" pointer is invalid and probably points to something that has already been freed. No idea why this happens...

[Nick]

Originally Posted by PowerWyrm

Traking down the problem in debug mode, it seems that the "obj" pointer is invalid and probably points to something that has already been freed. No idea why this happens...

This seems likely to be the same as in V - see my previous post. All the cases of

		if (dropped->known) {
			delist_object(cave_k, dropped->known);
			object_delete(&dropped->known);
		}

in drop_near() need to be

		if (dropped->known) {
			if (dropped->known->iy && dropped->known->ix)
 				square_excise_object(cave_k, dropped->known->iy, dropped->known->ix, dropped->known);
			delist_object(cave_k, dropped->known);
			object_delete(&dropped->known);
		}

[PowerWyrm]

Originally Posted by Nick

This seems likely to be the same as in V - see my previous post.

It's not the case (especially since I didn't implement object lists like in V). I think I found the problem (except from drop_near):

	/* Give it to the floor */
	if (!floor_carry(c, by, bx, dropped, false)) ...

	/* Sound */
	sound(MSG_DROP);

	/* Message when an object falls under the player */
	if (verbose && (c->squares[by][bx].mon < 0))
		/* Check the item still exists and isn't ignored */
		if (c->objects[dropped->oidx] && !ignore_item_ok(dropped))
			msg("You feel something roll beneath your feet.");

What happened is that the monster dropped something (probably gold since there were a lot of Ms) and didn't have space on the spot so it rolled under the character and got combined with a similar object.

	/* Scan objects in that grid for combination */
	for (obj = square_object(c, y, x); obj; obj = obj->next) {
		/* Check for combination */
		if (object_similar(obj, drop, OSTACK_FLOOR)) {
			/* Combine the items */
			object_absorb(obj, drop);

			/* Result */
			return true;
		}

		/* Count objects */
		n++;
	}

The problem is that when two objects are combined, the second object is deleted:

	/* Add together the item counts */
	obj1->number = (total < z_info->stack_size ? total : z_info->stack_size);

	object_absorb_merge(obj1, obj2);
	object_delete(&obj2);

The result is that object "dropped" has been wiped in drop_near(), but is referenced to display the "You feel something roll beneath your feet." message. IIRC this was working in 4.0.4: the code was using some "ignorable" variable to keep state prior to object deletion.

[PowerWyrm]

FYI: commit ec27572 introduced the bug.