Login
User Name:

Password:



Register
Forgot your password?
Vote for Us!
tintin++ ogg sound player script for linux
Author: Robert Smith
Submitted by: Vladaar
6Dragons ogg Soundpack
Author: Vladaar
Submitted by: Vladaar
6Dragons 4.4
Author: Vladaar
Submitted by: Vladaar
LoP 1.46
Author: Remcon
Submitted by: Remcon
LOP 1.45
Author: Remcon
Submitted by: Remcon
Users Online
CommonCrawl, dbnu

Members: 1
Guests: 2
Stats
Files
Topics
Posts
Members
Newest Member
481
3,734
19,366
618
Micheal64X
Today's Birthdays
There are no member birthdays today.
Related Links
» SmaugMuds.org » General » General Discussions » Safe STRFREE/DISPOSE ?
Forum Rules | Mark all | Recent Posts

Safe STRFREE/DISPOSE ?
< Newer Topic :: Older Topic > Weaving the safety net....

Pages:<< prev 1 next >>
Post is unread #1 Mar 6, 2005, 3:01 pm   Last edited May 7, 2005, 1:58 am by Samson
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,643
JoinedJan 1, 2002

I was looking through some old posts on the Smaug mailing list and something caught my eye. Eos had mentioned he put safeguards in his STRFREE and DISPOSE macros to help catch instances of them being used on the wrong strings - and to correct itself by freeing the string properly.

So I set out to see how hard this might be, and I'm stuck on a snag. Here's what I have:

Eos' in_hash_table function:

/*
 * str must be the actual pointer you want to know about, it cannot be a copy
 * of a pointer, or a variable.
 */
bool in_hash_table( char *str )
{
   register int len, hash, psize;
   register struct hashstr_data *ptr;

   len = strlen( str );
   psize = sizeof( struct hashstr_data );
   hash = len % STR_HASH_SIZE;
   for( ptr = string_hash[hash]; ptr; ptr = ptr->next )
      if( len == ptr->length && str == ( (char *)ptr + psize ) )
         return true;
   return false;
}


My modifications to our STRFREE and DISPOSE macros:

#define STRFREE(point)                                            
do                                                                
{                                                                 
   if((point))                                                    
   {                                                              
      if( !in_hash_table( (point) ) )                             
      {                                                           
         log_printf( "&RSTRFREE called on str_dup pointer: %s, line %d", __FILE__, __LINE__ ); 
         log_string( "Attempting to correct." );                  
         free( (point) );                                         
      }                                                           
      else if( str_free((point)) == -1 )                          
         log_printf( "&RSTRFREEing bad pointer: %s, line %d", __FILE__, __LINE__ ); 
      (point) = NULL;                                             
   }                                                              
} while(0)


#define DISPOSE(point)  
do                      
{                       
   if((point))          
   {                    
      if( in_hash_table( (point) ) ) 
      {                              
         log_printf( "&RDISPOSE called on STRALLOC pointer: %s, line %d", __FILE__, __LINE__ ); 
         log_string( "Attempting to correct." ); 
         if( str_free((point)) == -1 )           
            log_printf( "&RSTRFREEing bad pointer: %s, line %d", __FILE__, __LINE__ );
      }                 
      else              
         free((point)); 
      (point) = NULL;   
   }                    
} while(0)


Since AFKMud handles most of it's struct/class memory using new/delete, I only ran into a couple of spots where the modified DISPOSE macro complained, which I solved this way:

#define DELETE(point) 
do                    
{                     
   if((point))        
   {                  
      free((point));  
      (point) = NULL; 
   }                  
} while(0)


Basically renamed the original DISPOSE. The problem of course should become obvious to anyone who's not using our code. The DISPOSE macro is used not only to free up strings, but also to free up memory used by all the structs in Smaug.

Is there a way to make DISPOSE intelligent enough to know it's not touching something that isn't a string? I'm not entirely sure it can be done, but I don't want to dismiss it entirely. This would be a very valueable safeguard to have if it could be perfected.

BTW: Valgrind made no complaints about my test case which deliberately triggered the error messages. So it looks like it works - just need to figure out the last little bit.
       
Post is unread #2 Mar 6, 2005, 4:06 pm
Go to the top of the page
Go to the bottom of the page

Greven
Magician
GroupMembers
Posts204
JoinedMar 5, 2005

Now this is entirely untested, and I just a quick thought, but in C++ there is the typeid operator, and with some minor work you could adapt this into it I'm sure.

Looking at http://msdn.microsoft.com/library....tor.asp and http://msdn.microsoft.com/library....ss.asp, you could do something like
 if ( typeid(pointer) == typeif(char )
That may need to be char *, but I dunno. If I get a chance to test, I'll post the results.



       
Post is unread #3 Mar 6, 2005, 4:34 pm   Last edited May 7, 2005, 2:00 am by Samson
Go to the top of the page
Go to the bottom of the page

Greven
Magician
GroupMembers
Posts204
JoinedMar 5, 2005

Ok, so a little testings, and I had this

CMDF do_testtype(CHAR_DATA *ch, char *argument)
{
	char buf[MSL];
	char *string;
	CHAR_DATA *wch;

	argument = NULL;
	send_to_char(typeid(buf).name(), ch);
	send_to_char("", ch);
	send_to_char(typeid(string).name(), ch);
	send_to_char("", ch);
	send_to_char(typeid(*string).name(), ch);
	send_to_char("", ch);
	send_to_char(typeid(wch).name(), ch);
	send_to_char("", ch);
	send_to_char("typeid(buf) == typeid(string): ", ch);
	send_to_char(typeid(buf) == typeid(string) ? "True" : "False", ch);
	send_to_char("typeid(char[]) == typeid(buf): ", ch);
	send_to_char(typeid(char) == typeid(buf) ? "True" : "False", ch);
	send_to_char("typeid(char *) == typeid(string): ", ch);
	send_to_char(typeid(char *) == typeid(string) ? "True" : "False", ch);
	send_to_char("typeid(char *) == typeid(wch): ", ch);
	send_to_char(typeid(char *) == typeid(wch) ? "True" : "False", ch);
}


Now, the results as such:

A4096_c
Pc
c
P9char_data
typeid(buf) == typeid(string): False
typeid(char[]) == typeid(buf): False
typeid(char *) == typeid(string): True
typeid(char *) == typeid(wch): False


Now, for the names, I have to assume that the "A" means array, the 4096 is the size of the array(hey, this could be handy for array catching...), and the _c means character. the Pc I assume is "pointer to character", the next since its dereferenced is character, and the last is "pointer of size 9 bytes names char_data". Then the results, you can see that you can test against a character pointer.
       
Post is unread #4 Mar 6, 2005, 4:57 pm
Go to the top of the page
Go to the bottom of the page

Greven
Magician
GroupMembers
Posts204
JoinedMar 5, 2005

Actually, this won't work. typeid is a run time functions and still gives compile errors. Bleh, well, worth a shot
       
Post is unread #5 Mar 6, 2005, 6:30 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,643
JoinedJan 1, 2002

Interesting. I don't suppose something like this will work in a straight C environment will it? I'll have to fiddle a bit and see what my code has to say about this.
       
Post is unread #6 Mar 6, 2005, 7:21 pm
Go to the top of the page
Go to the bottom of the page

Greven
Magician
GroupMembers
Posts204
JoinedMar 5, 2005

The problem that I can across that was because its done at run time, the compiler gives errors about how it may be supplying the wrong data type into the in_hash_table function, same problem that you were having. I know that it would never reach that function unless it was a NULL terminated string, but it doesn't so...
       
Post is unread #7 Mar 6, 2005, 7:22 pm   Last edited Mar 25, 2005, 9:43 pm by Samson
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,643
JoinedJan 1, 2002

After some fiddling I was able to come up with this for DISPOSE:

#define DISPOSE(point)                         
do                                             
{                                              
   if( (point) )                               
   {                                           
      if( typeid((point)) == typeid(char*) )   
      {                                        
         if( in_hash_table( (char*)(point) ) ) 
         {                                     
            bug( "&RDISPOSE called on STRALLOC pointer: %s, line %d", __FILE__, __LINE__ ); 
            log_string( "Attempting to correct." ); 
            if( str_free( (char*)(point) ) == -1 ) 
               bug( "&RSTRFREEing bad pointer: %s, line %d", __FILE__, __LINE__ ); 
         }                                     
         else                                  
            free( (point) );                   
      }                                        
      else                                     
         free( (point) );                      
      (point) = NULL;                          
   }                                           
} while(0)


This works perfectly for C++ code, but as you can probably imagine, no dice when used as C. Such a nice fix too.
       
Post is unread #8 Mar 6, 2005, 8:41 pm   Last edited Mar 25, 2005, 9:39 pm by Samson
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,643
JoinedJan 1, 2002

Alright. It would seem a working solution came about that works in C - though I'm not 100% sure it's as safe as the C++ method, it did survive my test case under Valgrind. So to recap what we have:

In hashstr.c, the function provided on the SML by Eos ( modified to compile on FUSS ):
/*
 * str must be the actual pointer you want to know about, it cannot be a copy
 * of a pointer, or a variable.
 */
int in_hash_table( char *str )
{
   register int len, hash, psize;
   register struct hashstr_data *ptr;

   len = strlen( str );
   psize = sizeof( struct hashstr_data );
   hash = len % STR_HASH_SIZE;
   for( ptr = string_hash[hash]; ptr; ptr = ptr->next )
      if( len == ptr->length && str == ( (char *)ptr + psize ) )
         return 1;
   return 0;
}

The in_hash_table function needs to be declared in mud.h

The new STRFREE and DISPOSE macros:

#define DISPOSE(point)                         
do                                             
{                                              
   if( (point) )                               
   {                                           
      if( in_hash_table( (char*)(point) ) )    
      {                                        
         bug( "&RDISPOSE called on STRALLOC pointer: %s, line %d", __FILE__, __LINE__ ); 
         log_string( "Attempting to correct." ); 
         if( str_free( (char*)(point) ) == -1 ) 
            bug( "&RSTRFREEing bad pointer: %s, line %d", __FILE__, __LINE__ ); 
      }                                        
      else                                     
         free( (point) );                      
      (point) = NULL;                          
   }                                           
} while(0)

#define STRFREE(point)                           
do                                               
{                                                
   if((point))                                   
   {                                             
      if( !in_hash_table( (point) ) )            
      {                                          
         bug( "&RSTRFREE called on str_dup pointer: %s, line %d", __FILE__, __LINE__ ); 
         log_string( "Attempting to correct." ); 
         free( (point) );                        
      }                                          
      else if( str_free((point)) == -1 )         
         bug( "&RSTRFREEing bad pointer: %s, line %d", __FILE__, __LINE__ ); 
      (point) = NULL;                            
   }                                             
} while(0)


When the following block of code is executed:
  if( !str_cmp( "badhash", argument ) )
  {
   char *hashstr = STRALLOC( "Hash test" );
   char *nohashstr = str_dup( "Non-hashed test" );

   DISPOSE( hashstr );
   STRFREE( nohashstr );
   return;
  }


The results:


Log: [*****] BUG: DISPOSE called on STRALLOC pointer: act_wiz.c, line 107

Log: Attempting to correct.
Log: [*****] BUG: STRFREE called on str_dup pointer: act_wiz.c, line 108

Log: Attempting to correct.


Valgrind did not complain about invalid access, and the mud did not complain about bad pointers. So I think this works.
       
Post is unread #9 Mar 7, 2005, 4:55 am
Go to the top of the page
Go to the bottom of the page

Terell

GroupMembers
Posts2
JoinedMar 5, 2005

Excellent work! This should be a big bonus in memory management.

I know a friend of mine who didn't know the difference between STRALLOC, str_dup, STRFREE and DISPOSE and he is now encountering some big problems in his MUD. I'm going to provide him with a link for sure. Once again, awesome job.
       
Post is unread #10 Mar 8, 2005, 9:01 am   Last edited May 7, 2005, 1:56 am by Samson
Go to the top of the page
Go to the bottom of the page

Quixadhal
Conjurer
GroupMembers
Posts398
JoinedMar 8, 2005

It's a pity that the string_hash system uses linked lists instead of realloc'd arrays of pointers. If it used the latter we could say that IF we knew which bucket the string WOULD be in if it were, in fact, a string... we could know what values the pointer would have. Namely, >= string_hash[i] and <= string_hash[i][n] where n is the number of pointers in bucket i.

The big problem is that this system uses strlen() to place the string, and strlen() will cause seg-faults if passed a block of memory which contains no NULL bytes.

The only way I can think of to do this in plain C is to walk the whole linked-list structure in each bucket of string_hash and compare point against each data element from the list. Essentially, write a new version of in_hash_table that doesn't try to figure out which bucket, but walks through all of them. It'll be slower, but perhaps if only used in debugging it would still be handy?
       
Post is unread #11 Mar 8, 2005, 6:33 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,643
JoinedJan 1, 2002

Yeah, I'm aware there are probably some nasty pitfalls to the approach of the new DISPOSE macro in C. It's part of why it hasn't been placed in the FUSS packages yet, since I'm not certain it's safe to use on a wide scale. At least with the C++ macro you can use typeid for some assurance that what you're looking for is in fact a string.

This whole thing is all basically due to the original Smaug devs deciding to use DISPOSE as a generic deallocator instead of making a separate DELETE macro or something like I had initially thought of using.
       
Post is unread #12 Mar 13, 2005, 7:15 am
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,643
JoinedJan 1, 2002

I don't suppose anyone has had a chance to see how safe this fix is in C?
       
Post is unread #13 Mar 15, 2005, 8:32 pm
Go to the top of the page
Go to the bottom of the page

Greven
Magician
GroupMembers
Posts204
JoinedMar 5, 2005

I've installed it into SWRFUSS with no issues, been running in valgrind for 4 days with no issues at all.
       
Pages:<< prev 1 next >>