Crossfire Mailing List Archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
CF: bug fix for create_missile
Here is a bug fix for spell_effect.c. It fixes the "create missile"
spell, which was crashing the game when read from a scroll. Tracking
down this bug was a nightmare for me and I spent more than five hours
trying to understand what was wrong. I could have written a one-line
fix and a short message explaining it, but since I spent so much time
trying to find the bug, I will go for the long version. :-)
I noticed several times that the game was crashing shortly (but not
immediately) after I read a scroll of create missile, but I didn't
understand why. Crossfire was crashing in different ways: "free
object on active list", "object has no speed", or straight core dump
in the middle of some function. I put lots of debugging statements in
the code, but I found nothing unusual. Then I ran Crossfire through
Purify and I finally discovered what was happening: one of the
pointers in the active list was pointing to an area of the stack! Of
course, some "interesting" things happened in process_events(), when
that object was used or modified... with annoying consequences...
I eventually found where that problem came from: when a player or
monster casts a spell (a "known spell"), the function cast_spell()
uses the caster object directly. But if you cast a spell by using an
object (scroll, wand, etc.), cast_spell() uses a temporary copy of the
player or monster, in order to be able to modify the caster's level
and other stuff according to the level of the object. This means that
all functions that are called within cast_spell() get either a pointer
to the caster object, or a pointer to the temporary copy allocated on
the stack.
Bad Things happen if one of these functions modifies the object
(without knowing if it is the real one or a temporary copy of it) or
puts a pointer to that object in some other object. If the spell is
used from a scroll or a wand, you will be using a pointer to an area
that will not be valid after cast_spell() returns. That's what
happened with cast_create_missile(), which was calling pick_up() if
the arrows were created under the player (this happens if the spell is
invoked from a scroll or if there is a wall in front of the player).
And pick_up() was using a reference to the temporary copy of the
player, which had disastrous consequences when the active list was
processed later.
NOTE: If you are a programmer and you ever add or modify a spell, be
extremely careful with the value of "op" inside cast_spell() and all
functions called from it! Fixing bugs that corrupt the stack is a
nightmare...
So here is my patch to cast_create_missile() in spell_effect.c. The
only important difference is at the end of the function, when calling
pick_up(). But I also modified some other parts of the function so
that it is safer to use it in some other cases; I also added some
comments.
-Raphael
P.S.: Since nobody complained about the fact that I'm sending the
patches to the list, I keep on doing it. That's for the bug
fixes only. When I finish my patches for new features, I will
send these to Mark Wedel directly.
---------- cut here ---------- cut here ---------- cut here ----------
*** server/spell_effect.c.orig Sun Apr 21 07:07:39 1996
--- server/spell_effect.c Fri Jul 19 12:14:42 1996
***************
*** 1416,1441 ****
* Sets the plus based on the casters level. It is also settable with the
* invoke command. If the caster attempts to create missiles with too
* great a plus, the default is used.
! * The # of arrows create also goes up with level, so if a 30th level mage
* wants LOTS of arrows, and doesn't care what the plus is he could
* create nonnmagic arrows, or even -1, etc...
*
! * Written by Ben Fennema (huma@netcom.com)
*/
int cast_create_missile(object *op, int dir, char *stringarg)
{
! int missile_plus=0, missile_nrof;
! char *missile_name = NULL;
! object *tmp, *missile=NULL, *weap=NULL;
for (tmp=op->inv; tmp != NULL; tmp=tmp->below)
if (tmp->type == BOW && QUERY_FLAG(tmp, FLAG_APPLIED))
! weap= tmp;
!
! if (weap==NULL) missile_name = "arrow";
! else if (!strcmp(weap->race,"arrows")) missile_name="arrow";
! else missile_name = "bolt";
if (stringarg)
missile_plus = atoi(stringarg);
--- 1416,1442 ----
* Sets the plus based on the casters level. It is also settable with the
* invoke command. If the caster attempts to create missiles with too
* great a plus, the default is used.
! * The # of arrows created also goes up with level, so if a 30th level mage
* wants LOTS of arrows, and doesn't care what the plus is he could
* create nonnmagic arrows, or even -1, etc...
*
! * Written by Ben Fennema (huma@netcom.com) - bugs fixed by Raphael Quinet
*/
int cast_create_missile(object *op, int dir, char *stringarg)
{
! int missile_plus=0;
! char *missile_name;
! object *tmp, *missile;
+ missile_name = "arrow";
for (tmp=op->inv; tmp != NULL; tmp=tmp->below)
if (tmp->type == BOW && QUERY_FLAG(tmp, FLAG_APPLIED))
! {
! if (strstr(tmp->race, "bolt")) /* crossbow bolts */
! missile_name = "bolt";
! break;
! }
if (stringarg)
missile_plus = atoi(stringarg);
***************
*** 1447,1466 ****
missile_plus = 4;
else if (missile_plus < -4)
missile_plus = -4;
! missile_nrof = SP_PARAMETERS[SP_CREATE_MISSILE].bdur *
! ((1 + SP_level_strength_adjust(op, SP_CREATE_MISSILE)) -
! (3 * missile_plus));
! if (find_archetype(missile_name)==NULL) {
! LOG(llevDebug, "Cast create_missile: could not find archtype %s\n", missile_name);
! return 0;
! }
missile = get_archetype(missile_name);
! missile->nrof = missile_nrof;
missile->magic = missile_plus;
! missile->value=0;
SET_FLAG(missile, FLAG_IDENTIFIED);
if (!cast_create_obj(op,missile,dir) && op->type==PLAYER)
! pick_up(op, missile);
return 1;
}
--- 1448,1477 ----
missile_plus = 4;
else if (missile_plus < -4)
missile_plus = -4;
! if (find_archetype(missile_name)==NULL)
! {
! LOG(llevDebug, "Cast create_missile: could not find archetype %s\n",
! missile_name);
! return 0;
! }
missile = get_archetype(missile_name);
! missile->nrof = SP_PARAMETERS[SP_CREATE_MISSILE].bdur
! * ((1 + SP_level_strength_adjust(op, SP_CREATE_MISSILE))
! - (3 * missile_plus));
! if (missile->nrof < 1)
! missile->nrof = 1;
missile->magic = missile_plus;
! missile->value = 0; /* it would be too easy to get money by creating
! arrows +4 and selling them, even with value = 1 */
SET_FLAG(missile, FLAG_IDENTIFIED);
if (!cast_create_obj(op,missile,dir) && op->type==PLAYER)
! {
! tmp = get_owner(op);
! if (tmp == NULL)
! pick_up(op, missile); /* op points to the real player object */
! else
! pick_up(tmp, missile); /* op points to a copy of the player object */
! }
return 1;
}
---------- cut here ---------- cut here ---------- cut here ----------