Re: [CODE] VisionMUD class_spells_index buffer overflows

From: Peter Ajamian (peter@pajamian.dhs.org)
Date: 08/08/00


Torgny Bjers wrote:
>
> -----Original Message-----
> From: Peter Ajamian <peter@pajamian.dhs.org>
>
> but it still overflows the
> string somehow,

Are you quite ceartain that it's a buffer overflow?  From what I'm
seeing it's looking less and less like an overflow and more like a
coding error (or two or three).

> when I debug it prints out the top rows (the header and
> the ---- line) and starts on level 1, prints out about three items, and then
> it goes down one line to print out the other spells/skills, and it truncates
> it after like 3-10 chars and then it refuses to add anything further to the
> string (built in security check I presume).

I know of no such "built in security check".
>
> So, in the code, it is obviously somewhere around the for loop...

It would appear that way, but nothing is "obvious" yet, at least not to
me.

> I would
> without any programming knowledge guess that it is sprintf(str +
> strlen(str), "%s%-22s", buf1, spells[spellnum]); that does it, when it adds
> buf1 to str it messes up somehow...

I would venture to guess that it's more likely that it's not trying to
add the other spells to str at all.
>
> Here is the code again, with modifications:

There are still some suggestions I gave that were not addressed (at
least you did not indicate to everything I suggested earlier).  Rather
than repreating all of those I'll simply mention those things I didn't
see last time, since last time I only gave it a cursory scan I'll look
in greater detail this time...
>
> ----------------snip---------------------------------------
> void class_spells_index(int chclass, char *str)
> {
>   char buf1[MAX_STRING_LENGTH];
>   int i, spellnum, num;
>   int n_spells, n_skills;
>   *str = '\0';
>   sprinttype(chclass, pc_class_types, buf1);
>   sprintf(str,"Spells and Skills available for %s.\r\n", buf1);
>   strcat(str,
> "---------------------------------------------------------------------------
> --\r\n");
>   strcat(str, "Level          Spell/Skill   Name\r\n");
>
>   n_spells = 0;
>   n_skills = 0;

Everything up to this point looks fine...

>   for (i = 1; i <= MAX_MORT_LEVEL; i++) {
>     sprintf(str + strlen(str), "%2d   ", i);
>     num = 0;
>     for (spellnum = 1; spellnum < TOP_SPELLS; spellnum++) {
>       if (SINFO.min_level[chclass] == i) {
>         if (num >= 3) strcat(str, "\r\n     ");
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Get rid of that line, it has to go down below the point where the spell
is added to the output, and in a slightly different form.

>         if (spellnum >= 1 && spellnum <= MAX_SPELLS) {
              ^^^^^^^^^^^^^^^^

Again, this is redundant (spellnum will always be >=1), use...

if (spellnum <= MAX_SPELLS) {

>           strcpy(buf1, "");
            ^^^^^^^^^^^^^^^^^

Two things here, first of all, this is a very inefficient way of
clearing out a character array, a much better way is...

*buf1 = 0;

However, Looking further down in the code, I can't see any place where
buf1 is used except to copy an empty string in as part of an sprintf, so
rather than going through all the trouble to copy nothing, just get rid
of it alltogether.

>           n_spells++;
>         } else if (spellnum > MAX_SPELLS  && spellnum < START_NON_PLAYER_SPELLS) {
                     ^^^^^^^^^^^^^^^^^^^^^^^^^

This is also redundant, we know that spellnum > MAX_SPELLS because it
already failed the test for <=.  Simply use...

} else if (spellnum < START_NON_PLAYER_SPELLS) {

>           strcpy(buf1, "");
            ^^^^^^^^^^^^^^^^^

You can get rid of this also, same reason as before.

>           n_skills++;
>         } else
            ^^^^
>           strcpy(buf1, "");
            ^^^^^^^^^^^^^^^^^

You can get rid of the entire else clause here.

>         sprintf(str + strlen(str), "%s%-22s", buf1, spells[spellnum]);
                                      ^^      ^^^^^^

Since all we would be doing with buf1 is adding an empty string we can
just get rid of it alltogether as follows...

sprintf(str + strlen(str), "%-22s", spells[spellnum]);

This is where we check the number of spells on the line and add the
"\r\n     " as follows...

if (!(++num % 3))
  strcat(str, "\r\n     ");

>         num++;
          ^^^^^^

Get rid of this, num has already been incremented (it needed to be
incremented before it was checked).

>       }
>     }
>     strcat(str,"\r\n");
      ^^^^^^^^^^^^^^^^^^^

You only want to add "\r\n" here if it wasn't just added, so do it like
this...

if (num % 3)
  strcat(str, "\r\n");

>   }
>   strcat(str, "\r\n");
>   sprintf(str + strlen(str), "Spells: %d, Skills: %d, Total:%d\r\n",
>     n_spells, n_skills, n_spells+n_skills);
>   return;
> }
> ----------------snip---------------------------------------

Regards, Peter


     +------------------------------------------------------------+
     | Ensure that you have read the CircleMUD Mailing List FAQ:  |
     |  http://qsilver.queensu.ca/~fletchra/Circle/list-faq.html  |
     +------------------------------------------------------------+



This archive was generated by hypermail 2b30 : 04/11/01 PDT