#573 TFileDocument::GetProperty (StorageSize): Buffer overflow

[Vidar Hasfjord]

The buffer used for the string conversion for property StorageSize in TFileDocument::GetProperty (and likewise in TStorageDocument::GetProperty) is too small for large file sizes with the maximum 10 digits (null-termination is not accounted for).

Also, the format string used to convert the size is wrong: "%ld" should be "%lu".

Also note that there is a wider issue here: File sizes can be larger than 4 GB, requiring even more digits to represent. However, TDocument::FileLength is declared as a 32-bit owl::ulong and assigned to the result of GetFileSize. The latter is deprecated since it returns only the lower 32 bits of the true 64-bit value (with the upper half accessible through an optional out-parameter). It has been replaced by GetFileSizeEx, which returns the whole 64-bit value. Note that FileLength is a private member, and after assignment in TFileDocument::OpenThisFile, it is not used outside of GetProperty.

The same issue manifests itself in TStorageDocument::GetProperty, where the 64-bit value STATSTG::cbSize (returned by IStorage::Stat) is truncated to a 32-bit owl::ulong.

Proposed solutions:

1. For a simple fix, increase buffer size to 11 and correct the format string.
2. For a long-term fix, expand the size to 64-bit and implement robust text conversion, e.g.:

const auto d = static_cast<LPTSTR>(dest); CHECK(d);
const auto s = to_tstring(FileLength);
const auto n = static_cast<int>(s.size() + 1);
if (textlen < n)
{
  WARN(true, _T("TFileDocument::GetProperty (StorageSize): Insufficient buffer size: ") << textlen);
  *d = _T('\0');
  return n;
}
copy_n(s.c_str(), n, d); // Includes the null-terminator.
return n;

Note that (2) can be challenging, since just updating the bit-width would be a silent breaking change due to the void-pointer use in the GetProperty function signature. To make the change noisy or non-breaking, another property could be introduced, e.g. StorageSize64 or StorageSizeEx, while deprecating (or downright removing) the old property.

For an even better noisy breaking change, I propose making the function signature type-safe by e.g. returning std::any, thereby replacing the use of the void-pointer out-parameter. However, changing the signature is challenging because the function is virtual. This can be solved by introducing the new virtual function in parallel, while deprecating the old (using the [[deprecated]] attribute), or by replacing it by a dummy virtual function with an incompatible return type (which will generate compilation errors for old overrides). Deprecation would allow clients to use old overrides for another while, e.g. during porting, or until a replacement is required in the next version, but is messier and requires parallel implementations (duplication).

[Vidar Hasfjord]

The implementation of the "Doc/View properties overhaul" [feature-requests:#242] in Owlet [r6910] circumvents this issue by replacing the current property functions in TDocument and TView by a mix-in class providing a type-safe property API for both. Take a look and let me know what you think. Is it viable for the trunk?

[Ognyan Chernokozhev]

The simple fix for the potential buffer overflow has been merged to the branches 7 (in [r8760], 6.44 (in [r8761]) and 6.36 (in [r8762]).