Pages:<< prev 1 next >>


Black Hand

GroupAdministrators
Posts3,706
JoinedJan 1, 2002
Version 1.56b of AFKMud has been released.
This is a bugfix/maintenance release. This will also be the last release in the 1.5 series.
Future changes to the code are extensive enough to make further patches infeasible.
Standard disclaimer type stuff: Changes in this version may or may not remain compatible with your
older support files, such as areas, commands, skills, socials, etc. If things break, you were warned.
A new patching run was used in this version that should catch more than just source code
changes. To apply this patch to a stock base, drop it into the main afkmud directory and
type:
patch -p1 < 156bpatch
This will update all the necessary files. Be advised that doing so in a non-stock setup may
result in things you weren't counting on. You should read this changelog thoroughly before
applying it.
Bugfixes this release:
All changes for Intermud-3 2.25 Client included. [Samson]
Missed a removal for the Fortify debugger in mud.h [Samson]
Non-Code related changes:
Updated helps for authorize, think, and change form. [Vyn]
Updated helps for I3. [Samson]
Fixed a targetting bug on the invisibility spell. [Samson]
This is a bugfix/maintenance release. This will also be the last release in the 1.5 series.
Future changes to the code are extensive enough to make further patches infeasible.
Standard disclaimer type stuff: Changes in this version may or may not remain compatible with your
older support files, such as areas, commands, skills, socials, etc. If things break, you were warned.
A new patching run was used in this version that should catch more than just source code
changes. To apply this patch to a stock base, drop it into the main afkmud directory and
type:
patch -p1 < 156bpatch
This will update all the necessary files. Be advised that doing so in a non-stock setup may
result in things you weren't counting on. You should read this changelog thoroughly before
applying it.
Bugfixes this release:
All changes for Intermud-3 2.25 Client included. [Samson]
Missed a removal for the Fortify debugger in mud.h [Samson]
Non-Code related changes:
Updated helps for authorize, think, and change form. [Vyn]
Updated helps for I3. [Samson]
Fixed a targetting bug on the invisibility spell. [Samson]


Conjurer

GroupMembers
Posts395
JoinedMar 8, 2005
Heh, by taunting us with "Last of the 1.5 series", I happened upon this little buglet in magic.c, around line 5599:
I believe you want a $n there, not a %n.
act( AT_MAGIC, "You age horribly!", ch, NULL, victim, TO_VICT ); act( AT_MAGIC, "%n ages horribly!", victim, NULL, NULL, TO_ROOM ); victim->pcdata->age_bonus += 10;
I believe you want a $n there, not a %n.


Black Hand

GroupAdministrators
Posts3,706
JoinedJan 1, 2002
Yes, you are quite right. Fixed, but don't expect to see it before the overhaul is complete.
BTW, thanks for the MSL/MIL patch. Used it, and was glad to have it since it would have taken much time away from other things just to go fix those.
Also - regarding your color bleed patch. Resorted to a much simpler method. Terminal reset code sent with each prompt line. That seemed far less aggressive than the insertion into the other code you had, and so far it seems to be working out nicely this way.
Jury is still out on the strlcpy/strlcat patch. May opt to use the same less complicated code the I3 client is using. It appears to do the same thing. Also, some of the comments you left in there were somewhat uncertain. Spots where you didn't know what was going on, so when the time comes, that audit will need to be done by hand.
BTW, thanks for the MSL/MIL patch. Used it, and was glad to have it since it would have taken much time away from other things just to go fix those.
Also - regarding your color bleed patch. Resorted to a much simpler method. Terminal reset code sent with each prompt line. That seemed far less aggressive than the insertion into the other code you had, and so far it seems to be working out nicely this way.
Jury is still out on the strlcpy/strlcat patch. May opt to use the same less complicated code the I3 client is using. It appears to do the same thing. Also, some of the comments you left in there were somewhat uncertain. Spots where you didn't know what was going on, so when the time comes, that audit will need to be done by hand.



Conjurer

GroupMembers
Posts395
JoinedMar 8, 2005
My pleasure. I'm currently about halfway through nailing down all the sprintf calls and making them become snprintf, or adding vararg support to whatever used to need the sprintf/temp buffer. That's a little more "contravertial" change, but it doesn't add much overhead (it actually reduces memory footprint by having far fewer temp buffers), and makes the code quite a bit simpler.
I'll shoot you a copy of that when I finish, as always, to use or not as you like
I chose strlcpy mainly because I didn't like strncpy's "usually" NULL terminated behaviour. In my mind, strings have to always be NULL terminated, or they aren't really strings. For strlcat, it was more the readability of always passing the buffer size, rather than BUFLEN-strlen-1.
I am curious, does strlcpy exist in FreeBSD (I had to use my test machine to replace a dead power supply box, so I can't check until I reinstall it on something else).
At least one of the places I didn't mess with looked like it intentially copied a fraction of a string into the middle of another string (hence you would NOT want a terminating NULL added).
If any of this results in a more stable beastie, then I'm happy
I'll shoot you a copy of that when I finish, as always, to use or not as you like
I chose strlcpy mainly because I didn't like strncpy's "usually" NULL terminated behaviour. In my mind, strings have to always be NULL terminated, or they aren't really strings. For strlcat, it was more the readability of always passing the buffer size, rather than BUFLEN-strlen-1.
I am curious, does strlcpy exist in FreeBSD (I had to use my test machine to replace a dead power supply box, so I can't check until I reinstall it on something else).
At least one of the places I didn't mess with looked like it intentially copied a fraction of a string into the middle of another string (hence you would NOT want a terminating NULL added).
If any of this results in a more stable beastie, then I'm happy

Black Hand

GroupAdministrators
Posts3,706
JoinedJan 1, 2002
Yes, just checked with a FreeBSD server I have access to. strlcpy and strlcat are both there. They are not present in Cygwin or Linux though.
My understanding of the licensing involved is that BSD code is compatible with Diku licensing. While you may be able to charge for BSD licensed products, you are also allowed to use portions of them in product which payment is forbidden for. So I may just go ahead and run the audit on those.
Already hacked away at WAY too much to use the patch. No doubt many hunks would fail
If it becomes an issue of what's best, how does something like this look to you?
My understanding of the licensing involved is that BSD code is compatible with Diku licensing. While you may be able to charge for BSD licensed products, you are also allowed to use portions of them in product which payment is forbidden for. So I may just go ahead and run the audit on those.
Already hacked away at WAY too much to use the patch. No doubt many hunks would fail

If it becomes an issue of what's best, how does something like this look to you?
void i3strncpy( char *dest, const char *src, size_t size ) { strncpy( dest, src, size-1 ); dest[size-1] = '�'; }


Conjurer

GroupMembers
Posts395
JoinedMar 8, 2005
strlcpy/strlcat return the number of characters that you wanted to copy, which allows for overflow detection by checking if retlen > len (in case that matters, perhaps in data storage or OLC routines).
If you want to use the more common strncpy/strncat routines without looping through the source string once (as the strl family does), you can do so, but it will have slightly (function call) more overhead. To also detect overflows, you'd have to call strlen, which would be not quite double the cost. In the case of strncat, you have to call strlen twice (once to see how much space is left, and once if you want the overflow check).
In any case, you'll want to add a few security checks to that, to handle bogus pointers and such:
If you want to use the more common strncpy/strncat routines without looping through the source string once (as the strl family does), you can do so, but it will have slightly (function call) more overhead. To also detect overflows, you'd have to call strlen, which would be not quite double the cost. In the case of strncat, you have to call strlen twice (once to see how much space is left, and once if you want the overflow check).
In any case, you'll want to add a few security checks to that, to handle bogus pointers and such:
size_t i3strncpy(char *dst, const char *src, size_t len) { if(!src || !dst || len < 1) return 0; if(!*src) { *dst = '�'; return 0; } strncpy(dst, src, len-1); dst[len-1] = '�'; return strlen(dst); // Change to 1 to only report success. } size_t i3strncat(char *dst, const char *src, size_t len) { register int dlen; if(!src || !*src || !dst || len = len-1) // already over return 0; strncat(dst, src, len - dlen - 1); dst[len-1] = '�'; return strlen(dst); // Change to 1 to only report success. }


Conjurer

GroupMembers
Posts395
JoinedMar 8, 2005
I think I found one more little bug in fight.c
around line 4036, in do_murder, I believe that wants to be something more like "shout Help! I am....". The murder verb may not be used very often, so this might not have really shown up.
WAIT_STATE( ch, 1 * sysdata.pulseviolence ); sprintf( buf, "Help! I am being attacked by %s!", IS_NPC( ch ) ? ch->short_descr : ch->name ); if ( !IS_PKILL(victim) ) interpret( victim, buf );
around line 4036, in do_murder, I believe that wants to be something more like "shout Help! I am....". The murder verb may not be used very often, so this might not have really shown up.


Conjurer

GroupMembers
Posts395
JoinedMar 8, 2005
and... one more little memory leak bug...
in tables.c, there's an fread_string to a temp variable around line 1603, it needs to have a companion STRFREE before it goes out of scope:
Of course, they may still get handles to them elsewhere, since I'm sure those words get used all over the olc code and such too, but at least this ref to them should be clean.
in tables.c, there's an fread_string to a temp variable around line 1603, it needs to have a companion STRFREE before it goes out of scope:
case 'F': if( !str_cmp( word, "flags" ) ) { if( version flags = fread_number( fp ); fMatch = TRUE; break; } else { char *cmdflags = NULL; char flag[MIL]; int value; /* here */ cmdflags = fread_string( fp ); while ( cmdflags[0] != '�' ) { cmdflags = one_argument( cmdflags, flag ); value = get_cmdflag( flag ); if ( value = 32 ) bug( "Unknown command flag: %s", flag ); else TOGGLE_BIT( command->flags, 1 << value ); } /* here */ STRFREE(cmdflags); fMatch = TRUE; break; } } break;
Of course, they may still get handles to them elsewhere, since I'm sure those words get used all over the olc code and such too, but at least this ref to them should be clean.


Conjurer

GroupMembers
Posts395
JoinedMar 8, 2005
Here's a possible bug in the item_ego() routine of act_obj.c:
The code above seems to want to cap the return value at 90, but if obj_ego is between 91000 and 99999, it will return 91..99 as a value.
Is that intended? It seems like you'd either want it to return 90 for anything >= 90000, or return 99 for anything >= 99000 (or possibly 100 >= 100000 if you want "perfect" scores).
if( obj_ego >= 100000 ) obj_ego = 90; else obj_ego /= 1000; return(obj_ego);
The code above seems to want to cap the return value at 90, but if obj_ego is between 91000 and 99999, it will return 91..99 as a value.
Is that intended? It seems like you'd either want it to return 90 for anything >= 90000, or return 99 for anything >= 99000 (or possibly 100 >= 100000 if you want "perfect" scores).


Black Hand

GroupAdministrators
Posts3,706
JoinedJan 1, 2002
The fight.c thing is moot. do_murder was taken out and it's PK checks consolidated into do_kill.
The object ego calc is correct, look further up. The variable obj_ego is set to obj->rent before getting to where you're looking.
The memory leak in tables.c:
==22811== 61112 bytes in 1923 blocks are still reachable in loss record 5 of 6
==22811== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==22811== by 0x80FA4E0: str_alloc (hashstr.c:52)
==22811== by 0x80D7EEF: fread_string (db.c:545)
==22811== by 0x8160128: fread_command (tables.c:1588)
This is after having done what you suggest. I've been after this one for eons. If you look at the stuff in db.c for where it loads flags, you'll see that section is the same setup. So why does just this one complain? And why doesn't adding the STRFREE( cmdflags ); solve it? I would dearly love to slay this leak, but it isn't letting me
The object ego calc is correct, look further up. The variable obj_ego is set to obj->rent before getting to where you're looking.
The memory leak in tables.c:
==22811== 61112 bytes in 1923 blocks are still reachable in loss record 5 of 6
==22811== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==22811== by 0x80FA4E0: str_alloc (hashstr.c:52)
==22811== by 0x80D7EEF: fread_string (db.c:545)
==22811== by 0x8160128: fread_command (tables.c:1588)
This is after having done what you suggest. I've been after this one for eons. If you look at the stuff in db.c for where it loads flags, you'll see that section is the same setup. So why does just this one complain? And why doesn't adding the STRFREE( cmdflags ); solve it? I would dearly love to slay this leak, but it isn't letting me



Conjurer

GroupMembers
Posts395
JoinedMar 8, 2005
I know obj_ego gets initialized to whatever obj->rent has, but what I'm puzzled by is why it gets clipped to 90 if it's over 100K, but from 91K to 99K can be 91 through 99. In other words, why not clip it at 100, or why not do the clipping for > 90K?
IOW: Why should a weapon worth 95K have a 95 ego, but a weapon worth 500K has only a 90 ego?
Hmmm, as to the memory leak... I'm assuming something else has a lock on those shared string buffers. SInce many of the commands are common words, the odds are good that some of them will end up being str_alloc'd elsewhere too. We could make that particular copy go away by changing it to a str_dup/DISPOSE pair, but the other one would (in theory) still be floating out there. If it DID go away, that means there's only one hanging reference elsewhere... which might be findable (as opposed to possibly many copies).
IOW: Why should a weapon worth 95K have a 95 ego, but a weapon worth 500K has only a 90 ego?
Hmmm, as to the memory leak... I'm assuming something else has a lock on those shared string buffers. SInce many of the commands are common words, the odds are good that some of them will end up being str_alloc'd elsewhere too. We could make that particular copy go away by changing it to a str_dup/DISPOSE pair, but the other one would (in theory) still be floating out there. If it DID go away, that means there's only one hanging reference elsewhere... which might be findable (as opposed to possibly many copies).


Black Hand

GroupAdministrators
Posts3,706
JoinedJan 1, 2002
Point taken about the ego thing. Don't know why it was capped that way. 90K it is.
As for tables.c, changed to fread_string_nohash/DISPOSE
Got the following:
Fri Oct 10, 2003 4:31:18 PM PDT :: Loading commands...
==10517== Invalid free() / delete / delete[]
==10517== at 0x400296D7: free (vg_replace_malloc.c:220)
==10517== by 0x815E21B: fread_command (tables.c:1599)
==10517== by 0x815E3EB: load_commands (tables.c:1702)
==10517== by 0x80DACE9: boot_db (db.c:2081)
==10517== Address 0x41A04B6A is 6 bytes inside a block of size 7 alloc'd
==10517== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==10517== by 0x80E0055: str_dup (db.c:5373)
==10517== by 0x80D80B7: fread_string_nohash (db.c:618)
==10517== by 0x815E1D0: fread_command (tables.c:1588)
At which point it offers to attach to GDB for debugging. Examination reveals what's expected, the cmdflags variable is empty at that time.
Releasing it to continue loading completes bootup.
Shutdown produces a new report of the results though, and exposes a new set underneath:
==10517== 2585 bytes in 375 blocks are definitely lost in loss record 5 of 7
==10517== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==10517== by 0x80E0055: str_dup (db.c:5373)
==10517== by 0x80D80B7: fread_string_nohash (db.c:618)
==10517== by 0x815E1D0: fread_command (tables.c:1588)
==10517== 55527 bytes in 1548 blocks are still reachable in loss record 6 of 7
==10517== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==10517== by 0x80FA5B4: str_alloc (hashstr.c:52)
==10517== by 0x80D7FAB: fread_string (db.c:545)
==10517== by 0x80670C4: fread_shellcommand (shell.c:1265)
So I think your theory that it's allocating tons of junk to the string hash is probably right.
As for tables.c, changed to fread_string_nohash/DISPOSE
Got the following:
Fri Oct 10, 2003 4:31:18 PM PDT :: Loading commands...
==10517== Invalid free() / delete / delete[]
==10517== at 0x400296D7: free (vg_replace_malloc.c:220)
==10517== by 0x815E21B: fread_command (tables.c:1599)
==10517== by 0x815E3EB: load_commands (tables.c:1702)
==10517== by 0x80DACE9: boot_db (db.c:2081)
==10517== Address 0x41A04B6A is 6 bytes inside a block of size 7 alloc'd
==10517== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==10517== by 0x80E0055: str_dup (db.c:5373)
==10517== by 0x80D80B7: fread_string_nohash (db.c:618)
==10517== by 0x815E1D0: fread_command (tables.c:1588)
At which point it offers to attach to GDB for debugging. Examination reveals what's expected, the cmdflags variable is empty at that time.
Releasing it to continue loading completes bootup.
Shutdown produces a new report of the results though, and exposes a new set underneath:
==10517== 2585 bytes in 375 blocks are definitely lost in loss record 5 of 7
==10517== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==10517== by 0x80E0055: str_dup (db.c:5373)
==10517== by 0x80D80B7: fread_string_nohash (db.c:618)
==10517== by 0x815E1D0: fread_command (tables.c:1588)
==10517== 55527 bytes in 1548 blocks are still reachable in loss record 6 of 7
==10517== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==10517== by 0x80FA5B4: str_alloc (hashstr.c:52)
==10517== by 0x80D7FAB: fread_string (db.c:545)
==10517== by 0x80670C4: fread_shellcommand (shell.c:1265)
So I think your theory that it's allocating tons of junk to the string hash is probably right.


Black Hand

GroupAdministrators
Posts3,706
JoinedJan 1, 2002
*Feindish laugh from the 8th level of hell*
DEATH TO LEAKS!
*Ahem*
There. That felt better. It seems I've managed a solution. Copied fread_string and named the copy fread_flagstring. Behaved identically, except only returns a static buffer instead of allocating the result each time. Appears at first look to have fit the bill nicely.
Now to hunt down the remaining little bastards who hide from me in the night and make fun of me.....
DEATH TO LEAKS!
*Ahem*
There. That felt better. It seems I've managed a solution. Copied fread_string and named the copy fread_flagstring. Behaved identically, except only returns a static buffer instead of allocating the result each time. Appears at first look to have fit the bill nicely.
Now to hunt down the remaining little bastards who hide from me in the night and make fun of me.....


Black Hand

GroupAdministrators
Posts3,706
JoinedJan 1, 2002
After slogging through a few iterations of make/grind/fix leak/repeat, I've hit an impasse that is preventing me from reaching the next batch:
==14129== 30161 bytes in 803 blocks are still reachable in loss record 4 of 5
==14129== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==14129== by 0x80FA594: str_alloc (hashstr.c:52)
==14129== by 0x80D7FF6: fread_string (db.c:618)
==14129== by 0x815B57B: load_class_file (tables.c:170)
Relevant section of load_class_file:
case 'N':
KEY( "Name", class->who_name, fread_string( fp ) );
break;
Now, since class->who_name should legitimately be getting filled like this, this shouldn't be a problem.
Later in the memory_cleanup ( handy function for leak checking btw ) we have:
/* Classes */
fprintf( stdout, "%s", "Classes.
" );
for( hash = 0; hash who_name );
DISPOSE( class );
}
Which, as you see, removed the class->who_name. Any idea why this one is tripping? It's keeping me from locating other problem areas that could benefit from using the new fread_flagstring
==14129== 30161 bytes in 803 blocks are still reachable in loss record 4 of 5
==14129== at 0x4002989A: calloc (vg_replace_malloc.c:273)
==14129== by 0x80FA594: str_alloc (hashstr.c:52)
==14129== by 0x80D7FF6: fread_string (db.c:618)
==14129== by 0x815B57B: load_class_file (tables.c:170)
Relevant section of load_class_file:
case 'N':
KEY( "Name", class->who_name, fread_string( fp ) );
break;
Now, since class->who_name should legitimately be getting filled like this, this shouldn't be a problem.
Later in the memory_cleanup ( handy function for leak checking btw ) we have:
/* Classes */
fprintf( stdout, "%s", "Classes.
" );
for( hash = 0; hash who_name );
DISPOSE( class );
}
Which, as you see, removed the class->who_name. Any idea why this one is tripping? It's keeping me from locating other problem areas that could benefit from using the new fread_flagstring
Pages:<< prev 1 next >>