[aspell-devel] Memory overrun bug in aspell 0.60's String class

[Brett Wilson]

Hi,

I've discovered a bug in the memory allocation of the String class.
This seems pretty significant because this class is used everywhere in
the library. Here's the function in question (common/string.cpp:23):

  void String::reserve_i(size_t s)
  {
    size_t old_size = end_ - begin_;
    size_t new_size = (storage_end_ - begin_) * 3 / 2;
    if (new_size < 64) new_size = 64;
    if (new_size + 1 < s) new_size = s + 1;  <=========
    if (old_size == 0) {
      if (begin_) free(begin_);
      begin_ = (char *)malloc(new_size);
    } else {
      begin_ = (char *)realloc(begin_, new_size);
    }
    end_ = begin_ + old_size;
    storage_end_ = begin_ + new_size;
  }

The problem is if the initial buffer is small (<64 so it is expanded
to 64 in the 3rd line) and s (the length we need to make room for) is
64 or 65. In this case, new_size will still be 64, not leaving room
for the last character of s and possibly the null terminator. Yikes!

What was really meant was:

    if (new_size - 1 < s) new_size = s + 1;


Also, in String operator+ (string.hpp:397) the names of the two
arguments are switched. The function works correctly, as the names are
used consistently wrong, but I was pretty confused when I stepped
through this function while tracking down the above error. I take
'lhs' and 'rhs' to mean "Left/Right Hand Side", but 'rhs' actually
ends up on the left side of the concatenated string.

Brett Wilson