29 Dec, 2008, Hades_Kane wrote in the 1st comment:
Votes: 0
I can't seem to figure this one out.

We've redone a good bit of our magic system, and so we have our stock do_cast function copied over as do_cast2 and it is able to be used under the cast2 command by immortals and such. Everything was working fine, until I decided to add in an optional value on the end of the function for spell level. This crash doesn't happen very often, but enough to be annoying and a problem. When it reaches a certain part where I have it copying over "arg2" (the spot where the victim's name is usually input) over to the target_name value, strcpy likes to crash on occasion.

Here is the gdb back trace, with some frame and print values:

Program terminated with signal 11, Segmentation fault.
#0 0xb7e7b303 in strcpy () from /lib/tls/libc.so.6
(gdb) bt
#0 0xb7e7b303 in strcpy () from /lib/tls/libc.so.6
#1 0x08102ec2 in do_cast (ch=0xb6cacbe8, argument=0xbf967851 "") at magic.c:357
#2 0x080f7069 in interpret (ch=0xb6cacbe8, argument=0xbf967846 "rej diabolo", delayed=0 '\0')
at interp.c:1066
#3 0x08138222 in mpedit (ch=0xb6cacbe8, argument=0xb6da332f "rej diabolo") at olc_mpcode.c:120
#4 0x081265f1 in run_olc_editor (d=0xbf965fa0) at olc.c:59
#5 0x080a67a2 in game_loop_unix (control=4) at comm.c:927
#6 0x080a5f85 in main (argc=4, argv=0xbf967f14) at comm.c:464


Frame 1 list:
352         {
353 send_to_char( "Cast the spell on whom?\r\n", ch );
354 return;
355 }
356 else
357 strcpy(target_name,arg2);
358
359 /*if ( arg2[0] != '\0' )
360 strcpy(target_name,arg2);
361 else


The associated printing of various things in the frame:
print ch->name
$8 = 0xb790f3fd "Diablos"

print argument
$9 = 0xbf967851 ""

print arg1
$10 = "rej\000olo\000he darkness becomes oblivious white light to the sound of incredible detonation!{x
\n\rdelay 8 mob echo {RD{riabolos{D executes {R–{rS{Dou{rl {DE{rclips{De{R–{x \n\rdelay 12 bash
$r\n\rdelay 20 var "…

print arg2
$11 = "diabolo\000P\000\000\000\000\000\000\000fff\001\000\000\000\0009\000\000\000\v\000\000
\000d \000\000\000ú\000\000\000¤\001\000\000\000\000\000\000¨&ó\000{\000\000\000x\000\000
\000\177\017\177\003t`\226\000o\000\000\000\000\000\000\000C\"ç·d\000\000\000ú\000\000\000
\003\001\000\000µ½ò·µ½ò·\000\000\000\000ôßó·\001\000\000\000df\226¿L`\226¿%næ·df\226¿´
½ò·C\"ç·´½ò·\001\000\000\000þBæ·ÿÂò·ÿÂò·\000\000\000\000ôßó·þÂò·\001\000\000\000Hf
\226¿Ï¿ä·df\226¿þÂò·\001\000\000\000\000\000\000\000fa\226¿"…

print arg3
$12 = "\000\n\rdelayg* \bob echo {RD{riabolos{D executes {R–{rS{\000\000\000
\000ÿÿÿÿDE{r\000\000\000\000\000\000\000\000ÿÿÿÿ{x\n\r\000\000\000
\000D46\000¼`\226¿h$r\n\r\r\n\000\000\000\000\001\000\000\000r\\\226¿\000\000\000\000ðW
\226\000è`\226¿x8\035\b\000\000\000\000\02426\bæà·à\nô·\2068\035\b\002\000\000\000~06
\b¯:\035\b\005ó\034\b\000ó\034\b~06\b~06\b´c\033\b}c\033\bqc\033\b¯:\035\b~06\büB
\035\b"…


The entire top of the cast2 function:
/*
* The kludgy global is for spells who want more stuff from command line.
*/
char *target_name;

void do_cast( CHAR_DATA *ch, char *argument )
{
char arg1[MAX_INPUT_LENGTH];
char arg2[MAX_INPUT_LENGTH];
char arg3[MAX_INPUT_LENGTH];
char buf[MAX_STRING_LENGTH];
CHAR_DATA *victim;
OBJ_DATA *obj;
void *vo;
int mana;
int sn;
int target;
int sskill;
int level = 1;

if (!IS_NPC(ch) && ch->level < 117)
{
send_to_char("Huh?\r\n",ch );
return;
}

if (!IS_NPC(ch) && ch->pcdata->ghost && !str_cmp("life1",arg1) )
{
send_to_char("The dead can't cast spells.\n",ch );
return;
}

if (IS_AFFECTED (ch, AFF_MUTE))
{
send_to_char("You can't cast spells without your voice!\n",ch );
return;
}

if ( argument[0] == '\0' )
{
send_to_char( "Cast which what where?\r\n", ch );
return;
}

argument = one_argument( argument, arg1 );
argument = one_argument( argument, arg2 );
argument = one_argument( argument, arg3 );

if ( ( victim = get_char_room( ch, NULL, arg2 ) ) == NULL )
{
send_to_char( "Cast the spell on whom?\r\n", ch );
return;
}
else
strcpy(target_name,arg2);


I can tell something is going wrong with arg1, arg2, and arg3, and that the function is likely crashing because I'm trying to copy garbage over into the blank target_name value.

The input on the command I used was:

cast2 rej diabolo

rej being short for 'rejuvenate', a valid spell, and diabolo being short for 'diablolos' a mob I was fighting. Also, the garbage from the prints were echoes and program information on the mob I was fighting, but this has happened when cast on players, mobs with no programs, myself w/o any arguments… so I'm kind of at a loss.

Any help would be greatly appreciated.
29 Dec, 2008, David Haley wrote in the 2nd comment:
Votes: 0
Is target_name actually a valid buffer here? Where does target_name get allocated? I think your arg2 is ok – just look at where the null chars are. gdb is printing out that huge amount of text because it's trying to show you the whole array, I think.
29 Dec, 2008, ghasatta wrote in the 3rd comment:
Votes: 0
This looks to me like a misuse of strcpy(). Check the man page for that function - the first arg is a pointer to a destination for a new string. The strcpy function expects that the destination is a valid memory space. The scary global pointer 'target_name' is probably not pointing to memory that has been allocated for this purpose.

To add to what David Haley said regarding your c-string buffers: C-style strings are NUL terminated ('\0'). Any of the C-string system functions (such as strcpy) consider the first NUL character they encounter to be the end of the string. Anything after that is not considered when manipulating the string. What gdb is showing you is the contents of the full chunk of memory that was allocated as "char arg2[MAX_INPUT_LENGTH];" - the array is of size MAX_INPUT_LENGTH, regardless of the how short/long the string of characters is.
29 Dec, 2008, Hades_Kane wrote in the 4th comment:
Votes: 0
Even though the strcpy function does work -most- of the time for what I'm trying to do there, it could still be an improper use of it that's causing it to crash?

Do I need to find another way to pluck the value of "arg2" and make it target_name?

Instead of this:
argument = one_argument( argument, arg1 );
argument = one_argument( argument, arg2 );
argument = one_argument( argument, arg3 );


Would something like this be better?
argument = one_argument( argument, arg1 );
argument = one_argument( argument, target_name );
argument = one_argument( argument, arg3 );
29 Dec, 2008, David Haley wrote in the 5th comment:
Votes: 0
arg2 is fine here. Your problem is in target_name – it is most likely either pointing to bogus memory, or uninitialized memory. In gdb, can you print the value of 'target_name' when the crash happens?

The one_argument change would have the same problem if target_name doesn't point to good memory in the first place. It might help to see where target_name is initialized/allocated, so that we can make sure that's happening before this use.
29 Dec, 2008, Guest wrote in the 6th comment:
Votes: 0
/*
* The kludgy global is for spells who want more stuff from command line.
*/
char *target_name;


This evil thing? :P

Assuming the variable is still declared the same way in the code HK is now using, this would probably explain why strcpy hates it so much.

In Smaug, strcpy is never used on it, so never has this problem.
29 Dec, 2008, David Haley wrote in the 7th comment:
Votes: 0
IIRC, target_name is set to point to a whole target name string. You could in principle point it to arg2, as long as you are positive that every usage of target_name will occur before arg2 goes out of scope…
29 Dec, 2008, Scandum wrote in the 8th comment:
Votes: 0
You'd want to use char target_name[MAX_INPUT_LENGTH]; instead.
29 Dec, 2008, David Haley wrote in the 9th comment:
Votes: 0
Except that that would probably break many other uses of target_name…
29 Dec, 2008, Scandum wrote in the 10th comment:
Votes: 0
target_name = arg2; might work in that case, but since he mentioned it was related to spell level he might be better off using an integer.
29 Dec, 2008, David Haley wrote in the 11th comment:
Votes: 0
Why is target_name being used in the first place here? That might be worth looking into, HK…

Also, target_name = arg2 is very dangerous, as the arg2 memory will be invalidated as soon as the function returns – target_name will then be pointing to junk.
29 Dec, 2008, Hades_Kane wrote in the 12th comment:
Votes: 0
In this instance of the crash, I had typed:
cast2 antidote self

print target_name
$1 = 0x836319e ""



From what I can tell, is the cast function, by default, figures out what the target_name is, and then uses that in other spell functions through the file to determine the target.

In the stock function, it's written like this:

target_name = one_argument( argument, arg1 );
one_argument( target_name, arg2 );


It never seemed to cause any problems as it was above. Adding in the optional cast level option (as arg3) seemed necessary for me to have to rewrite it as I did above so it could correctly dissect "cast2 <spell> <victim> <spell level>" into the three parts I needed. At least, the use of one_argument was kind of throwing me a bit, so I played around with it until I found something that seems to have worked, except for when it crashes…
29 Dec, 2008, Scandum wrote in the 13th comment:
Votes: 0
DavidHaley said:
Also, target_name = arg2 is very dangerous, as the arg2 memory will be invalidated as soon as the function returns – target_name will then be pointing to junk.

Far from if you only use it in functions called by do_cast, which is pretty much everything in magic.c.
29 Dec, 2008, Scandum wrote in the 14th comment:
Votes: 0
You could do something like the following:

percentage = 100;

argument = one_argument(argument, arg1);

if (*arg1 != '\0' && is_number(arg1))
{
percentage = atol(arg1);

argument = one_argument(argument, arg1);

percentage = URANGE(1, percentage, 100);
}

target_name = argument;
argument = one_argument( argument, arg2 );
argument = one_argument( argument, arg3 );

….

level = level * percentage / 100 ;

if (level < 1)
{
level = 1;
}
30 Dec, 2008, David Haley wrote in the 15th comment:
Votes: 0
Hades_Kane said:
In the stock function, it's written like this:

target_name = one_argument( argument, arg1 );
one_argument( target_name, arg2 );


It never seemed to cause any problems as it was above. Adding in the optional cast level option (as arg3) seemed necessary for me to have to rewrite it as I did above so it could correctly dissect "cast2 <spell> <victim> <spell level>" into the three parts I needed. At least, the use of one_argument was kind of throwing me a bit, so I played around with it until I found something that seems to have worked, except for when it crashes…

So it looks like you're never supposed to write into target_name, but just assign pointers to it – so what Scandum suggested should work.

Scandum said:
Far from if you only use it in functions called by do_cast, which is pretty much everything in magic.c.

I'm not sure exactly what you meant, but I think we're agreeing that if it's only used below do_cast, everything is "fine". I still think it's dangerous and certainly very bad practice to use a global variable like this, but eh, what can you do… it would take a lot of work to fix now.
30 Dec, 2008, Scandum wrote in the 16th comment:
Votes: 0
DavidHaley said:
I still think it's dangerous and certainly very bad practice to use a global variable like this, but eh, what can you do… it would take a lot of work to fix now.

You could define a macro: #define DO_SPELL(spell) void spell( int sn, int level, CHAR_DATA *ch, void *vo, int target )

Next convert all spells to use:

DO_SPELL(spell_acid_blast) {

etc

Next update SPELL_FUN in merc.h and DO_SPELL to take an extra argument. That should make it fairly easy to update stuff.
30 Dec, 2008, David Haley wrote in the 17th comment:
Votes: 0
Agreed, that would be better than using a global variable. It also makes it easier to expand in the future (although I think a C++ std::map from parameter name to value would be the most versatile, at very little efficiency loss in the scheme of things).
30 Dec, 2008, Hades_Kane wrote in the 18th comment:
Votes: 0
I appreciate all of the advice in this thread.

I'm going to play around with some of the suggestions and I'm sure out of everything I'll find something that works.

Thanks!
0.0/18