14 Nov, 2006, Aidan wrote in the 1st comment:
Votes: 0
Okay, so I was brave. :P I ripped the rom memory engine out and replaced it with calloc/free calls. Mud runs *much* faster now, however, running under valgrind to check for leaks, I have this error.

Mon Nov 13 20:12:38 2006 :: Aidan left the game
==7894==
==7894== Invalid write of size 1
==7894== at 0x80961D7: game_loop(int) (comm.c:494)
==7894== by 0x80965E3: main (comm.c:235)
==7894== by 0x40363916: __libc_start_main (in /lib/libc-2.3.2.so)
==7894== by 0x8049660: ??? (start.S:81)
==7894== Address 0x45C7AF02 is 4126 bytes inside a block of size 6212 free'd
==7894== at 0x4002AEBC: free (vg_replace_malloc.c:231)
==7894== by 0x8095B5D: close_socket(descriptor_data*) (comm.c:752)
==7894== by 0x804DABC: do_quit(char_data*, char*) (act_comm.c:1258)
==7894== by 0x80BA368: interpret(char_data*, char*) (interp.c:850)

Now, the odd thing about this error is that it only does it once. After the first character logs out, it never returns.

d->incomm[0] = '\0'; <–comm.c line 494
free(dclose); <–comm.c line 752

Anyone have any ideas/suggestions?

Thanks
14 Nov, 2006, Omega wrote in the 2nd comment:
Votes: 0
the descriptor is being called after its removed :)

that would be my guess, because the way rom's system was designed, was so that you could pull the data after it was ('freed') case and point, fighting, when a kill happens, (raw_kill) if you find where it is called from, you will find that it continues to use the victim data after raw_kill, which calls extract_char, which in-turn, calls free_char to remove the character from the mud.

they designed the recycling as a method to prevent crashing from their bad coding techniques, and so that they could get away with being lazy. Thats my opinion on it.

My recommendation for you would to be, assess all the functions around the error, and ensure that once the free is called, the data isn't used again. This, is the difficult thing todo, cuz rom's code is a giant ass-hat of problems. I know, i did it myself years ago.
14 Nov, 2006, Aidan wrote in the 3rd comment:
Votes: 0
Thats the odd thing Darien. I'm nulling the data after free( )'ing it, and the descriptor loops are checked to make sure that d != null.
14 Nov, 2006, Omega wrote in the 4th comment:
Votes: 0
Okay, do your loops look like this.

for(d = descriptor_list; d != NULL; d=d_next)
{
d_next = d->next;
// rest of code here.
}


That is done to that instead of d = d->next because d->next would not exist, thus giving you rather funky messages.

Also, you should use a macro like…

#define CleanData(point) \
do \
if(point) \
{ \
free((void *)point); \
} \
else \
log_string("CleanData: Trying to free Null point!"); \
point = NULL; \
while(0);


Or something like that, just so that it makes point = NULL, and takes care of it in one little function call :) so you eat up less lines of code and know that the code is taken-care-of :)
14 Nov, 2006, Aidan wrote in the 5th comment:
Votes: 0
Heh. I'm a dumbass. I swapped the d->next to d_next and that fixed it.

I'll make the macro, thats a lot faster than the way I was doing it. Thanks for the advice. :)

I'll learnded somethin one day!
14 Nov, 2006, Omega wrote in the 6th comment:
Votes: 0
Ya, that tends to fix it :)

The d_next should be done in EVERY loop, nomatter what, that way it secures the loop, for 'just in case' thats what i believe, i nolonger do the d = d->next method in the for-loop, cuz it can bust.

I also prefer using that macro (well, one similar)
#define CleanData(point) \
do \
{ \
if(point) \
{ \
if(typeid(point) == typeid(char *) || typeid(point) == typeid(const char *)) \
{ \
delete [] point; \
} \
else \
{ \
free( (void *)point);\
} \
(point) = NULL; \
}\
else \
{ \
debug("CleanData: Trying to clean null Data from tile %s: function: %s: lines: %d", __FILE__, __PRETTY_FUNCTION__, __LINE__); \
(point) = NULL; \
} \
} while(0)


but my mud is C++…. so ya.. you can't use typeid :P or delete[]point :P
14 Nov, 2006, kiasyn wrote in the 7th comment:
Votes: 0
that hurts to look at
14 Nov, 2006, Aidan wrote in the 8th comment:
Votes: 0
Yeah, my main project is to slowly but surely rip all the rom code out. I hate it, its ugly, stupid, and evil. *sage* I suppose this just kinda pushed up some of the work. :)

Thanks again for the help.


Darien said:
Ya, that tends to fix it :)

The d_next should be done in EVERY loop, nomatter what, that way it secures the loop, for 'just in case' thats what i believe, i nolonger do the d = d->next method in the for-loop, cuz it can bust.

I also prefer using that macro (well, one similar)
#define CleanData(point) \
do \
{ \
if(point) \
{ \
if(typeid(point) == typeid(char *) || typeid(point) == typeid(const char *)) \
{ \
delete [] point; \
} \
else \
{ \
free( (void *)point);\
} \
(point) = NULL; \
}\
else \
{ \
debug("CleanData: Trying to clean null Data from tile %s: function: %s: lines: %d", __FILE__, __PRETTY_FUNCTION__, __LINE__); \
(point) = NULL; \
} \
} while(0)


but my mud is C++…. so ya.. you can't use typeid :P or delete[]point :P
15 Nov, 2006, Guest wrote in the 9th comment:
Votes: 0
kiasyn said:
that hurts to look at


If you think that hurt, look at this :)
#define DISPOSE(point)                         \
do \
{ \
if( (point) ) \
{ \
if( typeid((point)) == typeid(char*) || typeid((point)) == typeid(const char*) ) \
{ \
if( in_hash_table( (char*)(point) ) ) \
{ \
log_printf( "&RDISPOSE called on STRALLOC pointer: %s, line %d\n", __FILE__, __LINE__ ); \
log_string( "Attempting to correct." ); \
if( str_free( (char*)(point) ) == -1 ) \
log_printf( "&RSTRFREEing bad pointer: %s, line %d\n", __FILE__, __LINE__ ); \
} \
else \
delete[] (point); \
} \
else \
free( (point) ); \
(point) = NULL; \
} \
else \
(point) = NULL; \
} while(0)
#endif
15 Nov, 2006, kiasyn wrote in the 10th comment:
Votes: 0
thats' actually easier to read. decent indentation, no unnecessary { } and the \ are better placed.
15 Nov, 2006, Aidan wrote in the 11th comment:
Votes: 0
Okay, so I'm getting another fun error. Any ideas on this one?

void show_list_to_char( OBJ_DATA * list, CHAR_DATA * ch, bool fShort,
bool fShowNothing )
{
char buf[MAX_STRING_LENGTH];
char **prgpstrShow;
int *prgnShow;
char *pstrShow;
OBJ_DATA *obj;
OBJ_DATA *obj_next;
int nShow;
int iShow;
int count;
bool fCombine;

if ( ch->desc == NULL )
return;

/*
* Alloc space for output lines.
*/
count = 0;
for ( obj = list; obj != NULL; obj = obj_next )
{
obj_next = obj->next_content;
count++;
}

/*
* If there were no objects in the list, return and do nothing.
*/
if ( count <= 0 )
return;

prgpstrShow = (char **)calloc(count,sizeof(char *));
prgnShow = (int *)calloc(count,sizeof(int)); //crashes here
nShow = 0;

/*
* Format the list of objects.
*/
for ( obj = list; obj != NULL; obj = obj_next )
{
obj_next = obj->next_content;

if(obj->wear_loc != LOC_NONE)
continue;

if (can_see_obj(ch, obj) && !IS_SET(obj->extra_flags, ITEM_NOLONG))
{
pstrShow = format_obj_to_char( obj, ch, fShort );
fCombine = FALSE;

if ( IS_NPC( ch ) || IS_SET( ch->comm, COMM_COMBINE ) )
{
/*
* Look for duplicates, case sensitive.
* Matches tend to be near end so run loop backwords.
*/
for ( iShow = nShow - 1; iShow >= 0; iShow– )
{
if ( !strcmp( prgpstrShow[iShow], pstrShow ) )
{
prgnShow[iShow]++;
fCombine = TRUE;
break;
}
}
}

/*
* Couldn't combine, or didn't want to.
*/
if ( !fCombine )
{
prgpstrShow[nShow] = str_dup( pstrShow );
prgnShow[nShow] = 1;
nShow++;
}
}
else
;
}

/*
* Output the formatted list.
*/
for ( iShow = 0; iShow < nShow; iShow++ )
{
if ( IS_NPC( ch ) || IS_SET( ch->comm, COMM_COMBINE ) )
{
if ( prgnShow[iShow] != 1 )
{
sprintf( buf, "(%2d) ", prgnShow[iShow] );
page_to_char( buf, ch );
}
else
{
page_to_char( " ", ch );
}
}
page_to_char( prgpstrShow[iShow], ch );
page_to_char( "\n\r`w", ch );
freeData(prgpstrShow[iShow] );
}

if ( fShowNothing && nShow == 0 )
{
if ( IS_NPC( ch ) || IS_SET( ch->comm, COMM_COMBINE ) )
send_to_char( " ", ch );
send_to_char( "Nothing.\n\r`w", ch );
}

/*
* Clean up.
*/
freeData(prgpstrShow);
free(prgnShow);
return;
}


It's kinda got me stumped, though I hate that function and will probably rewrite it one of these days. I'm still trying to work the bugs out since I swapped memory systems and this is one I just cant track down.

Any ideas?
16 Nov, 2006, Conner wrote in the 12th comment:
Votes: 0
What's the error?
16 Nov, 2006, Omega wrote in the 13th comment:
Votes: 0
Thanks smaug for this code

#define CREATE(result, type, number) \
do \
{ \
if (!((result) = (type *) calloc ((number), sizeof(type)))) \
{ \
perror("malloc failure"); \
fprintf(stderr, "Malloc failure @ %s:%d\n", __FILE__, __LINE__ ); \
abort(); \
} \
} while(0)


use that as your basic calloc.

Change your prgnShow = (int *)calloc(count,sizeof(int)); to

CREATE(prgnShow, int, sizeof(int));

Should fix your problem, it did when i did my conversion to callocs.
16 Nov, 2006, Aidan wrote in the 14th comment:
Votes: 0
Thanks Darien, I'll give that a go. Thanks a ton for the assists. :)

I'm learning more about memory handling than I wanted to, but eh, its worth it.
17 Nov, 2006, kiasyn wrote in the 15th comment:
Votes: 0
uhhhhh no, CREATE( prgnShow, int, 1 ); otherwise you're going to do it the sizeof(sizeof(int))
17 Nov, 2006, Davion wrote in the 16th comment:
Votes: 0
Quote
uhhhhh no, CREATE( prgnShow, int, 1 ); otherwise you're going to do it the sizeof(sizeof(int))


Or ya know… do it right and use CREATE(prgnShow, int, count ); :grinning:
17 Nov, 2006, Tyche wrote in the 17th comment:
Votes: 0
Aidan said:
I'm learning more about memory handling than I wanted to, but eh, its worth it.


You might want to explore why one would want to use the slower calloc in preference to the faster malloc and memset. And since structure initialization is already done in these muds, either manually or through coping of static structures why one would do it twice.

:unclesam: :ghostface: :robot: :cyclops:
17 Nov, 2006, Aidan wrote in the 18th comment:
Votes: 0
Tyche said:
Aidan said:
I'm learning more about memory handling than I wanted to, but eh, its worth it.


You might want to explore why one would want to use the slower calloc in preference to the faster malloc and memset. And since structure initialization is already done in these muds, either manually or through coping of static structures why one would do it twice.

:unclesam: :ghostface: :robot: :cyclops:


I was under the impression that all data structures had to be initialized in the beginning, which is what I was attempting to do. I basically stripped the built-in memory handler out and redid it all with calloc, as that is what I was told was best to use since it initialized everything at 0 automatically.

However, due to massive instability and pulling my hair out, I have temporarily given up on it.
17 Nov, 2006, Omega wrote in the 19th comment:
Votes: 0
calloc is safer then malloc, and new is better then them both ;)

(hugs c++)
18 Nov, 2006, Guest wrote in the 20th comment:
Votes: 0
new is only better if you follow it up with a proper initialization in the constructor. By itself, new does not do anything more than create space to use, much like malloc does. I spent many a day fixing up the mess I made thinking that new was the answer to all of that, only to have Valgrind spend its time bitching at me about all the uninitialized values it was using.
0.0/22