fread_string and fread_string_nohash

Posted by Greven on Wed 11 Feb 2004 10:57 PM — 9 posts, 102,444 views.

Greven Canada Wed 11 Feb 2004 10:57 PM #0

Looking through lots of code, I've noticed that there seems to be a waste of memory in lots of places. Not nessecarily leaks, but just wastes.

For example, CHAR_DATA has many of its fields initialized with STALLOC(""); and str_dup("");. This is needed, of course, to make sure that there is memory, even with an empty string, in the fields so if they are accesses, you will not crash. However, this memory is rarely freed before having its pointer changed to whereever the memory from the fread_string or fread_string_nohash is. This then seems to be lost memory.

When using fread_string, it is much less of a problem than with fread_string_nohash, because all the "" take up one block of memory, but nohashed string will just be taking up extra memory.

As a point, I enter "memory check greven", and this is the appropriate data output:

Hash info on string: greven
Links: 3  Position: 53/291  Hash: 6  Length: 6

When I do "memory check", checking the empty string, I get:

Hash info on string: 
Links: 38791  Position: 1/1  Hash: 0  Length: 0

This is not nessecarily bad, but its not good, since there must be lots of memory wasted on str_dup, assuming that you haven't changed all strings to STRALLOC/STRFREE.

I have thought of 2 seperate way that this might be handled:
One, is use a different form of fread_string, perhaps fread_string_free(FILE *fp, char **string) and fread_string_nohash_free(FILE *fp, char **string), where it does not return anything, but has "if ( *string ) STRFREE(buf)" and the str_dup equivalent, and uses "*string = STRALLOC( *string );return" instead of "return STRALLOC(string);"

The other idea I had was a little different, and would probably take more work, I dunno. This would be to use 2 different KEY macros, since this is used in the majority of file reading(except areas, I know), perhaps KEYH and KEYN, something like:

#define KEYH( literal, field, value )					\
				if ( !str_cmp( word, literal ) )	\
				{					\
				    if ( field )                        \
				        STRFREE( field );               \
				    field  = value;			\
				    fMatch = TRUE;			\
				    break;				\
				}

#define KEYN( literal, field, value )					\
				if ( !str_cmp( word, literal ) )	\
				{					\
				    if ( field )                        \
				        DISPOSE( field );               \
				    field  = value;			\
				    fMatch = TRUE;			\
				    break;				\
				}

However, all these ideas need the coder to be very strict making sure they have corresponding STALLOC/fread_string and str_dup/fread_string_nohash.
Just a thought while I'm bored, and flaws/input on this idea?

Samson USA Thu 12 Feb 2004 12:50 AM #1

Heh. I long ago scrubbed my code free of all of this STRALLOC("") and str_dup("") crap. Of course it led to many crashes and long hours trakcing them all down, but in the end I feel it was well worth a few extra lines of code here and there as safety checks against NULL strings to undo the memory loss caused by all this. So far I've suffered no ill effects from having done this.

Nick Gammon Australia Forum Administrator Thu 12 Feb 2004 01:14 AM #2

Or, convert to C++ and use the 'string' type (STL), then you don't need to allocate at all, and you never overflow the bounds of the string.

I did a recent post with the diffs to convert SMAUG to C++, however it will probably not totally work, or work at all, for SMAUG FUSS - but it might be worth trying. ;)

Nick Gammon Australia Forum Administrator Thu 12 Feb 2004 02:54 AM #3

I've done some diffs to convert SMAUG FUSS to C++, if anyone wants them let me know.

Samson USA Thu 12 Feb 2004 07:16 AM #4

Oh, yes. I'd be quite interested in seeing the results of such an effort. How exactly do you mean "convert to C++" ? Just getting it to peacefully compile using g++ vs gcc? Or have you taken the next step with it and also integrated the std::string stuff?

I've held off on doing so with my own codebase partly because Valgrind was complaining about one part I tried using C++ string code in saying that it wasn't being freed properly. But I'm not entirely certain this was true. Though AFKMud does peacefully compile under g++ so I suppose I'm 90% of the way there already.

Nick Gammon Australia Forum Administrator Thu 12 Feb 2004 10:31 AM #5

It is only compiling with C++. As I commented in my earlier post about doing SMAUG (non-FUSS) there are other issues, one fairly obvious one being the need to do a "new" on things like CHAR_DATA if you are going to have imbedded strings. Otherwise, yes, you will lose memory, or even have crashes (as the constructor would not be called for the string, nor the destructor).

The file is at:



ftp://ftp.gammon.com.au/smaug/smaugfuss_cpp_diffs.txt.gz

Because my earlier patches were done *with* the MXP code added this patch will add MXP also, however you could strip that out, or muck around and accept a few missed hunks.

It should convert the recent SMAUG FUSS source to be compileable under C++.

Without making major changes you could use strings, and other STL things locally (eg. inside a single function) or in places where you will handle the allocation yourself.

One case might be "fread_word" which could simply return a string, rather than all this business of returning pointers to static variables.

Also you might get rid of a lot of the rather painfull stuff like:


skills.c:    char buf[MAX_STRING_LENGTH];
skills.c:  char buf[MAX_STRING_LENGTH];
skills.c:  char buf1[MAX_STRING_LENGTH];
skills.c:   char buf[MAX_STRING_LENGTH];
special.c:    char buf[MAX_STRING_LENGTH];
special.c:    char buf[MAX_STRING_LENGTH];
special.c:    char buf[MAX_STRING_LENGTH];
tables.c:    char buf[MAX_STRING_LENGTH];
tables.c:    char buf[MAX_STRING_LENGTH];
tables.c:    char buf[MAX_STRING_LENGTH];

There are around 362 of those. All these "max strings" seems a bit silly, and even then it fails if you happen to read more than MAX_STRING_LENGTH into them.

Here is one example that shows what I mean...


void send_ansi_title( CHAR_DATA *ch )
{
    FILE *rpfile;
    int num=0;
    char BUFF[MAX_STRING_LENGTH*2];

    if ((rpfile = fopen(ANSITITLE_FILE,"r")) !=NULL) {
      while ((BUFF[num]=fgetc(rpfile)) != EOF)
         num++;
      fclose(rpfile);
      BUFF[num] = 0;
      write_to_buffer(ch->desc,BUFF,num);
    }
}

Now if the ANSI welcome file happens to be 100 bytes, the buffer is much too large, and if it happens to be 10000 bytes (which happened to me recently) the MUD crashes, as strangely-enough there is not even a test for the limit on the read.

In fact the whole thing is quite bizarre - why choose MAX_STRING_LENGTH*2? Why not pluck another figure totally out of the air, like 8843? And having plucked a figure out of the air like that, why not test for the maximum being reached? It would be easy enough:


while ((BUFF[num]=fgetc(rpfile)) != EOF &&
       num < ((MAX_STRING_LENGTH*2) - 1) )
         num++;

David Haley USA Thu 12 Feb 2004 05:34 PM #6

Quote:

Oh, yes. I'd be quite interested in seeing the results of such an effort. How exactly do you mean "convert to C++" ? Just getting it to peacefully compile using g++ vs gcc? Or have you taken the next step with it and also integrated the std::string stuff?

Personally, I took the "next step" and have converted many of the fundamental data types to classes. Simply getting it to compile was a tedious but not overly painful process, as I recall.

To solve the hash table problem, for instance, I have std::string for non-hashed, and my own class for hashed. That way, I have type safety. Both classes free and allocate themselves, anyways, so no worries there. :)

At the moment I'm in the middle of taking this even further and basically completely throwing away much of SMAUG design in favor of a more streamlined and up-to-date OOP version of the MUD. I already threw away its network code - I made a few long posts on that some time ago - and right now I'm going even further: equipment, building, character read/write, objects (especially objects), the lot.

Edit to add:

Quote:

Now if the ANSI welcome file happens to be 100 bytes, the buffer is much too large, and if it happens to be 10000 bytes (which happened to me recently) the MUD crashes, as strangely-enough there is not even a test for the limit on the read.

SMAUG has that kind of bad coding practice up to wazoo. Much of the time I suspect they were having odd bugs, so they just threw in solutions that "worked". Then again, these solutions come and bite you in the neck later on. It's what you get for having the program cobbled together based on two previous versions; you inherit all sorts of legacy (bad) design that you really would be better off without. :)

Samson USA Thu 12 Feb 2004 06:05 PM #7

Correct me if I'm wrong in reading this, but isn't the show_file function in db.c already protected against overflow?


	while ((buf[num]=fgetc(fp)) != EOF
	&&      buf[num] != '\n'
	&&      buf[num] != '\r'
	&&      num < (MAX_STRING_LENGTH-2))
	  num++;

That part of it would seem to suggest so unless I'm missing the point of why they used MAX_STRING_LENGTH-2 there. If that's the case, can't show_ansi_title and show_ascii_title simply be obsolteted in favor of passing the files used in them to show_file instead?

Nick Gammon Australia Forum Administrator Thu 12 Feb 2004 09:53 PM #8

Even that version isn't perfect, for a couple of reasons ...

It actually does this - in part:


    if ( (fp = fopen( filename, "r" )) != NULL )
    {
      while ( !feof(fp) )
      {
        while ((buf[num]=fgetc(fp)) != EOF
        &&      buf[num] != '\n'
        &&      buf[num] != '\r'
        &&      num < (MAX_STRING_LENGTH-2))
          num++;
        c = fgetc(fp);
        if ( (c != '\n' && c != '\r') || c == buf[num] )
          ungetc(c, fp);
        buf[num++] = '\n';
        buf[num++] = '\r';
        buf[num  ] = '\0';
        send_to_pager_color( buf, ch );
        num = 0;
      }

Now what that is doing is reading until MAX_STRING_LENGTH - 2 bytes from the end of the buffer, and then adding *three* bytes to it (\n, \r, and \0). Sounds like a bug waiting to happen.

Second, by using fgetc it will be slow, as it is reading a byte at a time. That is, one operating system call per byte. My own tests have shown that this is much slower than doing a fread on the entire chunk at once.

Here is one way of doing it using C++ and STL:


void send_ansi_title( CHAR_DATA *ch )
{
string s;
ifstream f (ANSITITLE_FILE);
if (f)
  {
  while (getline (f, s, '\n'))
    {
    s += '\n';  // put delimiter back
    write_to_buffer (ch->desc, s.c_str (), -1);
    }  // end of getting a line
  }  // end of file exists
}  // end of send_ansi_title

This is reasonably easy to follow, excepting the bit about adding the delimiter back.

However I think internally that the "getline" will still read a byte at a time. Here is another approach that looks a bit messier, but reads a block:


void send_ansi_title( CHAR_DATA *ch )
{
int length;
char * buffer;
ifstream f (ANSITITLE_FILE);
if (f)
  {

  // get length of file:
  f.seekg (0, ios::end);
  length = f.tellg();
  f.seekg (0, ios::beg);

  // allocate memory:
  buffer = new char [length];

  if (buffer)
    {
    // read data as a block:
    f.read (buffer, length);

    // send buffer
    write_to_buffer (ch->desc, buffer, length);
    delete [] buffer;
    }

  }  // end of file existing
}  // end of send_ansi_title