#156 TResId ipstream op returns local memory and leaks

[Vidar Hasfjord]

The relevant code in "source/owlcore/wsysclsp.cpp":
....
_USES_CONVERSION;
id = TResId(_A2W(is.freadString()));
}
return is;
....

In UNICODE builds, this code will return a pointer to the converted string in local memory and leak the memory allocated by freadString. This is a clear bug.

In non-UNICODE builds, it will return an unmanaged pointer to the memory allocated by freadString. This will leak if the caller doesn't handle the memory (unclear). If the caller does so, it must be able to distinguish between local memory being returned (UNICODE builds) and unmanaged heap memory (non-UNICODE) builds. This is unclear, but regardless of how it is handled, the code is very brittle.

Alternative solutions:

1. Why convert? If we can remove the conversion, we're down to one problem; manage the heap memory.

2. Allocate a copy of the conversion on the heap. Again, we're down to one problem.

3. Make TResId capable of storing the string referred to by its encapsulated Windows resource id, e.g. add an argument to the constructor: TResId(LPCTSTR c, bool copyString = false). This constructor should throw if the passed value is not a valid string pointer (i.e. when TResId::IsInt(c) == true).

4. Like 3, but make a static pool of TResId objects. When copyString is true, allocate the string on the heap and put a copy of the pointer in the pool. Then, in the TResID destructor, lookup the pointer in the pool, and if found, deallocate the memory it points to.

The best/easiest solution depends on whether the caller code currently handles the heap memory. If so, solution 3 and 4 requires further code changes, while solution 1 and 2 do not.

Solution 3 has another drawback: It makes TResId larger than a pointer since it needs more state to indicate whether it owns the memory pointed to or not. Some code may rely on TResID and a pointer to be the same size. This is unclear.

Solution 4 doesn't have the latter drawback of solution 3, but this solution is slower and more elaborate.

Solution 3 or 4 are the most robust solutions among these. I tend to prefer solution 3, although that requires a review of how TResId is used, i.e. change any OWLNext code that assumes sizeof(LPTSTR) == sizeof(TResId). If there's a lot of such code, or we need to consider user code making this assumption, then solution 4 seems the best option.

There may be even better solutions that I haven't thought of, though.

Add your comments below.

[Vidar Hasfjord]

I've checked some of the callers of the ipstream op for TResId:

• operator >>(ipstream&, TBarDescr&); reads TResID TBarDescr::Id, but does not handle the memory allocated.

• TDialog::Streamer::Read; reads (TResId&) DialogAttr.Name, but does not handle the memory in all cases. The TDialog destructor has logic to deallocate memory pointed to by DialogAttr.Name depending on many flags, but nothing to indicate whether DialogAttr.Name has been read from a stream (and thus needs deallocation). Hence it seems likely that memory will leak in some scenarios.

• TFrameWindow::Streamer::Read; reads TFrameWindow::IconResId and TFrameWindow::IconSmResId but does not handle the memory allocated.

• operator >>(ipstream&, TMenuDescr&); reads TResId TMenuDescr::Id, but does not handle the memory allocated.

• TWindow::Streamer::Read; reads TResId TempId (casted and assigned to TWindow::Title), TWindowAttr::Menu, TWindowAttr::AccelTable and TWindow::CursorResId, but does not handle the memory allocated.

There may be more, but the conclusion seems to be clear: OWLNext code seems to be fully ignorant of the fact that reading a TResId may allocate memory.

This means that solution 1 and 2 are not enough on their own. Solution 3 or 4, or something better, is needed.

And whatever the solution, modification may be needed to some client code, such as the TDialog destructor that makes assumptions about the memory that a TResId points to.

[Ognyan Chernokozhev]

The problem with the memory leak is present back in OWL5. I'm pretty sure CodeGuard should have been able to detect such problems, and my guess is that noone really used persistent streams to store TDialog or other OWL GUI objects.

[Vidar Hasfjord]

A partial solution to this problem that fixes the local memory issue, as suggested by Jogy, is implemented in revision 1166:

http://owlnext.svn.sourceforge.net/viewvc/owlnext?view=revision&revision=1166

Note that the memory management problem is still an issue.

TODO: Ensure that memory is properly managed wherever TResId is read from a persistent stream throughout OWLNext.