hbwin/olecore.c: pending return value can be overwritten when an OLE object is released (IDispatch::Release pumps messages)

[hmpaquito]

Environment

Harbour 3.2.0dev (r2006301601), BCC 7.3, FiveWin 17.05, Excel automation (out-of-process server), Windows Server with Terminal Services.

Symptom
Sometimes the value returned by a function or method that has just worked with OLE reaches the caller as a 2-element array {0, NNNNNNN} (second element is a large number, looks like a pointer) instead of the real value. It happens rarely and at random, and the same code usually works. Example:

::oWorkBook := ::oExcel:WorkBooks:Open( cFile ) ... ::oWorkBook:SaveAs( cFile ) // Error BASE/1004 Message not found: ARRAY:SAVEAS

A workaround that avoids it: don't return the value with RETURN. The function stores it in a by-reference parameter and returns NIL.

Analysis

1. RETURN x stores the value in the stack return item. After that, hb_stackOldFrame() clears the locals of the frame, and for a method call also the temporary Self (in the example, the WorkBooks object returned by ::oExcel:WorkBooks).
2. If one of those items holds the last reference to an OLE object, hb_gcRefFree() calls hb_ole_destructor() at once, which calls IDispatch::Release().
3. For an out-of-process server, the last Release() on the proxy is an ORPC call (IRemUnknown::RemRelease, see MS-DCOM). On an STA thread, COM runs a modal message loop during outgoing calls and dispatches window messages; re-entrancy is not prevented (see "Descriptions and workings of OLE threading models").
4. So the GUI library's window procedure can run PRG code at that moment, while the return value is still pending. If that callback doesn't save and restore the return item, the caller gets whatever the callback left there.
5. It is intermittent because it only happens when a message is waiting while the Release() call is in progress.

Harbour already protects this case in other places: hb_objDestructorCall() and hb_oleDispatchToItem() use hb_vmRequestReenter() / hb_vmRequestRestore(), and olecore.c has the comment "pItem actually can be stack's return item!". The GC destructors in olecore.c call Release() without that protection.

I could not check whether FiveWin's window procedure saves the return item (its C source is not public). But since Release() can dispatch messages to any GUI library, it seems safer to protect it in olecore.c.

Proposed patch (contrib/hbwin/olecore.c)

static HB_GARBAGE_FUNC( hb_ole_destructor ) ... /* Release() of an out-of-process proxy may pump messages and reenter HVM: preserve pending return value */ if( hb_vmRequestReenter() ) { HB_VTBL( pDisp )->Release( HB_THIS( pDisp ) ); hb_vmRequestRestore(); } else HB_VTBL( pDisp )->Release( HB_THIS( pDisp ) );

The same change goes in hb_oleenum_destructor() (around pEnum->Release()) and hb_olevariant_destructor() (around VariantClear()).

Status
The analysis is based on reading the Harbour sources (estack.c, garbage.c, itemapi.c, classes.c, olecore.c) and the Microsoft COM/DCOM documentation. The patch has not been tested in production yet. I will report the results.