#14293: Possible resource leak in wxAutomationObject::Invoke()
6 views
Skip to first unread message
wxTrac
May 11, 2012, 3:48:10 AM5/11/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293>
#14293: Possible resource leak in wxAutomationObject::Invoke()
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
I believe there's a possible resource leak in wxAutomationObject::Invoke()
method.
{{{
// Convert the wxVariants to VARIANTARGs
VARIANTARG* oleArgs = new VARIANTARG[noArgs];
for (i = 0; i < noArgs; i++)
{
// Again, reverse args
if (!wxConvertVariantToOle(INVOKEARG((noArgs-1) - i), oleArgs[i]))
{
delete[] argNames;
delete[] dispIds;
delete[] oleArgs;
return false;
}
}
}}}
As you can see, if wxConvertVariantToOle() fails, the already converted
VARIANTs are not properly cleared, which is bad if any of them contains
e.g. BSTR, array, or IDispatch.
Proposed fix
{{{
// Convert the wxVariants to VARIANTARGs
VARIANTARG* oleArgs = new VARIANTARG[noArgs];
for (i = 0; i < noArgs; i++)
{
// Again, reverse args
if (!wxConvertVariantToOle(INVOKEARG((noArgs-1) - i), oleArgs[i]))
{
delete[] argNames;
delete[] dispIds;
for (int j = 0; j < i; j++)
VariantClear(&oleArgs[j]);
delete[] oleArgs;
return false;
}
}
}}}
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293>
#14293: Possible resource leak in wxAutomationObject::Invoke()
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
I believe there's a possible resource leak in wxAutomationObject::Invoke()
method.
{{{
// Convert the wxVariants to VARIANTARGs
VARIANTARG* oleArgs = new VARIANTARG[noArgs];
for (i = 0; i < noArgs; i++)
{
// Again, reverse args
if (!wxConvertVariantToOle(INVOKEARG((noArgs-1) - i), oleArgs[i]))
{
delete[] argNames;
delete[] dispIds;
delete[] oleArgs;
return false;
}
}
}}}
As you can see, if wxConvertVariantToOle() fails, the already converted
VARIANTs are not properly cleared, which is bad if any of them contains
e.g. BSTR, array, or IDispatch.
Proposed fix
{{{
// Convert the wxVariants to VARIANTARGs
VARIANTARG* oleArgs = new VARIANTARG[noArgs];
for (i = 0; i < noArgs; i++)
{
// Again, reverse args
if (!wxConvertVariantToOle(INVOKEARG((noArgs-1) - i), oleArgs[i]))
{
delete[] argNames;
delete[] dispIds;
for (int j = 0; j < i; j++)
VariantClear(&oleArgs[j]);
delete[] oleArgs;
return false;
}
}
}}}
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293>
wxTrac
May 14, 2012, 3:14:12 AM5/14/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:1>
#14293: Possible resource leak in wxAutomationObject::Invoke()
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
Comment(by PB):
A bit of follow-up, even this patch won't cover all possible resource
leaks.
`wxAutomation` object can sometimes (though indirectly) modify the
parameters of its `Invoke()` method (and of its wrappers
`CallMethod/GetProperty/PutProperty`), even when these parameters are
declared as `const wxVariant&`. This happens when the `wxVariant` argument
contains an `IDispatch`: `wxConvertVariantToOle()` doesn't call `AddRef()`
on the dispatch and Invoke() calls `VariantClear()` on converted
`VARIANT`s, which releases the stored object. So you have to compensate
for this behaviour in the user code with an extra !AddRef() call, else you
might be left with a dangling pointer after calling
`wxAutomation::Invoke()`or its wrappers. This could lead to an IDispatch
resource leak: even in my patch only the already converted `wxVariant`s
are cleared, so e.g. if the `wxVariant` in argument 1 fails to convert and
`wxVariant` in argument 2 contains an `IDispatch`, it won't be cleared,
most likely preventing the COM object to be disposed, resulting in the
other program not quitting when it should. The user code can't work around
this, because it doesn't know which arguments were cleared and which were
not.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:1>
#14293: Possible resource leak in wxAutomationObject::Invoke()
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
A bit of follow-up, even this patch won't cover all possible resource
leaks.
`wxAutomation` object can sometimes (though indirectly) modify the
parameters of its `Invoke()` method (and of its wrappers
`CallMethod/GetProperty/PutProperty`), even when these parameters are
declared as `const wxVariant&`. This happens when the `wxVariant` argument
contains an `IDispatch`: `wxConvertVariantToOle()` doesn't call `AddRef()`
on the dispatch and Invoke() calls `VariantClear()` on converted
`VARIANT`s, which releases the stored object. So you have to compensate
for this behaviour in the user code with an extra !AddRef() call, else you
might be left with a dangling pointer after calling
`wxAutomation::Invoke()`or its wrappers. This could lead to an IDispatch
resource leak: even in my patch only the already converted `wxVariant`s
are cleared, so e.g. if the `wxVariant` in argument 1 fails to convert and
`wxVariant` in argument 2 contains an `IDispatch`, it won't be cleared,
most likely preventing the COM object to be disposed, resulting in the
other program not quitting when it should. The user code can't work around
this, because it doesn't know which arguments were cleared and which were
not.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:1>
wxTrac
May 15, 2012, 5:46:15 AM5/15/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:2>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
-----------------------------------------------------+----------------------
Changes (by vadz):
* priority: normal => low
* status: new => confirmed
* patch: 1 => 0
Comment:
It looks like the correct thing to do would be to call `AddRef()` in
`wxConvertVariantToOle()` and then always clear the variants in `Invoke()`
(would be nice to have some RAII class doing this automatically).
Could you please make a [HowToSubmitPatches patch] doing this? TIA!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:2>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 0 | Blocking:
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
-----------------------------------------------------+----------------------
Changes (by vadz):
* priority: normal => low
* status: new => confirmed
* patch: 1 => 0
Comment:
It looks like the correct thing to do would be to call `AddRef()` in
`wxConvertVariantToOle()` and then always clear the variants in `Invoke()`
(would be nice to have some RAII class doing this automatically).
Could you please make a [HowToSubmitPatches patch] doing this? TIA!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:2>
wxTrac
May 15, 2012, 11:16:35 AM5/15/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:3>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
-----------------------------------------------------+----------------------
Comment(by PB):
Replying to [comment:2 vadz]:
> It looks like the correct thing to do would be to call `AddRef()` in
`wxConvertVariantToOle()` and then always clear the variants in `Invoke()`
Sorry, I can not agree with making such fundamental change in the
implementation of wxConvertVariantToOle() so late in the game. I assume
there are other applications that learned to add an extra !AddRef() call
to the dispatches being passed as arguments to
wxAutomationObject::Invoke() and its wrappers. If you make the change,
such applications will all of sudden start leaking them. There may be some
other code relying on the current wxConvertVariantToOle() behaviour too.
I believe that now the only possibility is to release all the passed
dispatches in Invoke() no matter if the individual wxConvertVaraintToOle()
calls succeed or not.
What am I missing?
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:3>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 0 | Blocking:
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
-----------------------------------------------------+----------------------
Comment(by PB):
Replying to [comment:2 vadz]:
> It looks like the correct thing to do would be to call `AddRef()` in
`wxConvertVariantToOle()` and then always clear the variants in `Invoke()`
implementation of wxConvertVariantToOle() so late in the game. I assume
there are other applications that learned to add an extra !AddRef() call
to the dispatches being passed as arguments to
wxAutomationObject::Invoke() and its wrappers. If you make the change,
such applications will all of sudden start leaking them. There may be some
other code relying on the current wxConvertVariantToOle() behaviour too.
I believe that now the only possibility is to release all the passed
dispatches in Invoke() no matter if the individual wxConvertVaraintToOle()
calls succeed or not.
What am I missing?
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:3>
wxTrac
May 15, 2012, 12:13:01 PM5/15/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:4>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
been clear from my previous posts. The current implementation of Invoke()
already calls VariantClear() on converted VARIANTs, but only if all the
conversions from wxVariant succeeded. See lines 194-195
[http://trac.wxwidgets.org/browser/wxWidgets/trunk/src/msw/ole/automtn.cpp
here].
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:4>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 0 | Blocking:
-----------------------------------------------------+----------------------
Comment(by PB):
If one is not familiar with wxAutomation::Invoke(), one thing may not had
-----------------------------------------------------+----------------------
Comment(by PB):
been clear from my previous posts. The current implementation of Invoke()
already calls VariantClear() on converted VARIANTs, but only if all the
conversions from wxVariant succeeded. See lines 194-195
[http://trac.wxwidgets.org/browser/wxWidgets/trunk/src/msw/ole/automtn.cpp
here].
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:4>
wxTrac
May 25, 2012, 5:30:46 AM5/25/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:5>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
-----------------------------------------------------+----------------------
Comment(by vadz):
I thought `wxConvertVariantToOle()` was an internal function but you're
right, it might be used outside of wx (I really miss Google code search,
it was great for checking things like this). Nothing prevents us from
adding a wrapper around it that would always `AddRef()` properly though,
right? So what about doing this?
The idea is, anyhow, that to avoid such leaks we need to make the variants
themselves responsible for destroying them. I.e. use RAII wrapper instead
of raw `VARIANTARG`s as we do now.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:5>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 0 | Blocking:
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
-----------------------------------------------------+----------------------
Comment(by vadz):
I thought `wxConvertVariantToOle()` was an internal function but you're
right, it might be used outside of wx (I really miss Google code search,
it was great for checking things like this). Nothing prevents us from
adding a wrapper around it that would always `AddRef()` properly though,
right? So what about doing this?
The idea is, anyhow, that to avoid such leaks we need to make the variants
themselves responsible for destroying them. I.e. use RAII wrapper instead
of raw `VARIANTARG`s as we do now.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:5>
wxTrac
May 25, 2012, 8:23:19 AM5/25/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:6>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
on the same page, so please let my try to reiterate the problem.
I reported the bug because of a possible memory leak: in case of an error
in converting the wxVariant argument to OLE VARIANT, the already
converted VARIANTs were not cleared (!VariantClear()), which would result
in leaks if any of the already converted VARIANTs contained a SAFEARRAY,
BSTR or IDispatch pointer.
The patch proposed in my first post deals with that for !SafeArrays and
BSTRs. But there is still an inconsistency when it comes to treating
wxVariants containing IDispatch pointers. The current implementation of
wxAutomationObject::Invoke() decreases the reference count of IDispatches
passed to it as arguments in wxVariants (if all the arguments were
successfully converted to OLE VARIANTs). The programmer learns very fast
about this (I certainly did when I my code started crashing because of
invalid pointers) and has to work around it, which often means adding an
extra IDispatch->!AddRef() call. If what you suggest is implemented, the
reference count of dispatches passed as arguments to
wxAutomationObject::Invoke() won't be changed which would mean the current
3rd party code could after recompiling with the updated wxWidgets start
leaking dispatches left and right.
With all that said, I believe the proper solution would be:
1. Do not alter behaviour of wxConvertVariantToOle(), it's too late to do
that now.
2. Make wxAutomation::Invoke() behaviour consistent: always decrease the
ref count of IDispatch pointers passed to it as arguments, regardless of
success of converting all its arguments to VARIANTs. I don't think anyone
would account for the current inconsistent behaviour (which of course
doesn't mean that the fix can't expose a bug in the user code which
happens to rely on the bug >.< ).
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:6>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 0 | Blocking:
-----------------------------------------------------+----------------------
Comment(by PB):
Vadim, it's probably because of my broken English but I think we are not
-----------------------------------------------------+----------------------
Comment(by PB):
on the same page, so please let my try to reiterate the problem.
I reported the bug because of a possible memory leak: in case of an error
in converting the wxVariant argument to OLE VARIANT, the already
converted VARIANTs were not cleared (!VariantClear()), which would result
in leaks if any of the already converted VARIANTs contained a SAFEARRAY,
BSTR or IDispatch pointer.
The patch proposed in my first post deals with that for !SafeArrays and
BSTRs. But there is still an inconsistency when it comes to treating
wxVariants containing IDispatch pointers. The current implementation of
wxAutomationObject::Invoke() decreases the reference count of IDispatches
passed to it as arguments in wxVariants (if all the arguments were
successfully converted to OLE VARIANTs). The programmer learns very fast
about this (I certainly did when I my code started crashing because of
invalid pointers) and has to work around it, which often means adding an
extra IDispatch->!AddRef() call. If what you suggest is implemented, the
reference count of dispatches passed as arguments to
wxAutomationObject::Invoke() won't be changed which would mean the current
3rd party code could after recompiling with the updated wxWidgets start
leaking dispatches left and right.
With all that said, I believe the proper solution would be:
1. Do not alter behaviour of wxConvertVariantToOle(), it's too late to do
that now.
2. Make wxAutomation::Invoke() behaviour consistent: always decrease the
ref count of IDispatch pointers passed to it as arguments, regardless of
success of converting all its arguments to VARIANTs. I don't think anyone
would account for the current inconsistent behaviour (which of course
doesn't mean that the fix can't expose a bug in the user code which
happens to rely on the bug >.< ).
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:6>
wxTrac
May 25, 2012, 1:26:28 PM5/25/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:7>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
And there are indeed 2 different things here. First one is the fix in your
initial patch: it's fine (except for that other problem...) but I'd prefer
to have a RAII wrapper for `VARIANT` to avoid having to write such code at
all. This is what I keep speaking about since the beginning.
Second one is more serious, of course. Personally I think it's a bug that
`Invoke()` changes its parameters and I really think it should never do it
instead of always doing it. But this would, of course, introduce leaks in
the existing code using it... I don't know what to do about it. The best
would probably be to deprecate this one and add an overload (with
different order of parameters or maybe different types, e.g. a
`wxVector<VARIANT>` instead of `noArgs, args`?) which handles the
reference counting properly.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:7>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 0 | Blocking:
-----------------------------------------------------+----------------------
Comment(by vadz):
I think your English is great, I just keep misunderstanding you.
-----------------------------------------------------+----------------------
Comment(by vadz):
And there are indeed 2 different things here. First one is the fix in your
initial patch: it's fine (except for that other problem...) but I'd prefer
to have a RAII wrapper for `VARIANT` to avoid having to write such code at
all. This is what I keep speaking about since the beginning.
Second one is more serious, of course. Personally I think it's a bug that
`Invoke()` changes its parameters and I really think it should never do it
instead of always doing it. But this would, of course, introduce leaks in
the existing code using it... I don't know what to do about it. The best
would probably be to deprecate this one and add an overload (with
different order of parameters or maybe different types, e.g. a
`wxVector<VARIANT>` instead of `noArgs, args`?) which handles the
reference counting properly.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:7>
wxTrac
May 27, 2012, 5:56:33 AM5/27/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:8>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
wxAutomationObject::Invoke(). If the function exits on line 150 or 172,
individual BSTRs in argNames are not freed. Those are allocated at lines
119-132 and freed only at lines 187-189. I hope my patch takes care about
all the leaks.[[BR]][[BR]]2. IDispatch issue[[BR]]It's not as simple as
deprecating wxAutomation::Invoke(), it would mean to deprecate and
redeclare all the methods that call Invoke = pretty much whole
wxAutomationObject interface (about dozen methods). My recommendation:
leave it be, just make sure it behaves consistently when it comes to
releasing dispatches in its arguments and document this surprising
behaviour.[[BR]][[BR]]3. Other[[BR]]I have added a wxConvertOleToVariant()
check for a value to be returned from Invoke(), if it fails I return false
instead of true. I think it makes sense because while the underlying get
property or method call succeeded, the user code won't be able to get the
resulting value.[[BR]]
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:8>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 0 | Blocking:
-----------------------------------------------------+----------------------
Comment(by PB):
1. Memory leaks[[BR]]I believe I found another potential memory leaks in
-----------------------------------------------------+----------------------
Comment(by PB):
wxAutomationObject::Invoke(). If the function exits on line 150 or 172,
individual BSTRs in argNames are not freed. Those are allocated at lines
119-132 and freed only at lines 187-189. I hope my patch takes care about
all the leaks.[[BR]][[BR]]2. IDispatch issue[[BR]]It's not as simple as
deprecating wxAutomation::Invoke(), it would mean to deprecate and
redeclare all the methods that call Invoke = pretty much whole
wxAutomationObject interface (about dozen methods). My recommendation:
leave it be, just make sure it behaves consistently when it comes to
releasing dispatches in its arguments and document this surprising
behaviour.[[BR]][[BR]]3. Other[[BR]]I have added a wxConvertOleToVariant()
check for a value to be returned from Invoke(), if it fails I return false
instead of true. I think it makes sense because while the underlying get
property or method call succeeded, the user code won't be able to get the
resulting value.[[BR]]
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:8>
wxTrac
May 27, 2012, 6:56:13 AM5/27/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:9>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
* patch: 0 => 1
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:9>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
Changes (by PB):
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
* patch: 0 => 1
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:9>
wxTrac
May 27, 2012, 7:21:17 AM5/27/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:10>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
I have to add that I have lingering doubts about the correctness of my
code in the patch. Neither wxBasicString nor wxOleVariantArg are PODs, yet
I treat them as if their in-memory representation was the same as that of
BSTR and VARIANTARG?!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:10>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
Comment(by PB):
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
I have to add that I have lingering doubts about the correctness of my
code in the patch. Neither wxBasicString nor wxOleVariantArg are PODs, yet
I treat them as if their in-memory representation was the same as that of
BSTR and VARIANTARG?!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:10>
wxTrac
May 27, 2012, 7:40:39 AM5/27/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:11>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
-----------------------------------------------------+----------------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Casting `wxBasicString*` to `BSTR*` is indeed not totally correct but in
practice this does work in all environments I know about and definitely
with all the Windows compilers so it's OK. The only thing that isn't is
that `begin()` isn't guaranteed to always return a pointer (even though
currently it does in `wxVector<>`), which is why `&vec[0]` should rather
be used. I'll apply the patch with this change soon, thanks!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:11>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
Comment(by vadz):
Keywords: Automation, OLE, VARIANT, resource leak | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------+----------------------
Casting `wxBasicString*` to `BSTR*` is indeed not totally correct but in
practice this does work in all environments I know about and definitely
with all the Windows compilers so it's OK. The only thing that isn't is
that `begin()` isn't guaranteed to always return a pointer (even though
currently it does in `wxVector<>`), which is why `&vec[0]` should rather
be used. I'll apply the patch with this change soon, thanks!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:11>
wxTrac
May 27, 2012, 9:00:06 AM5/27/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:12>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Changes (by VZ):
* status: confirmed => closed
* resolution: => fixed
Comment:
(In [71574]) Fix memory leaks in wxAutomationObject::Invoke().
Use wxVector<>, wxBasicString and wxOleVariantArg instead of raw arrays,
BSTR
and VARIANT to ensure that different objects allocated by this function
are
always freed when it exits.
Closes #14293.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:12>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
Reporter: PB | Owner:
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Changes (by VZ):
* status: confirmed => closed
* resolution: => fixed
Comment:
(In [71574]) Fix memory leaks in wxAutomationObject::Invoke().
Use wxVector<>, wxBasicString and wxOleVariantArg instead of raw arrays,
BSTR
and VARIANT to ensure that different objects allocated by this function
are
always freed when it exits.
Closes #14293.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:12>
wxTrac
May 27, 2012, 9:09:04 AM5/27/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:13>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Comment(by PB):
What about the idispatch issue? I noticed you removed my inconsistent
behaviour fix and also by "document Invoke() behaviour regarding
IDispatches in args" I meant to actually add it to the reference manual,
not just leave a comment in the CPP file. While it's not entirely related
to this ticket, I think it should be somewhere, but I am not sure if it
is worth a new ticket.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:13>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Comment(by PB):
What about the idispatch issue? I noticed you removed my inconsistent
behaviour fix and also by "document Invoke() behaviour regarding
IDispatches in args" I meant to actually add it to the reference manual,
not just leave a comment in the CPP file. While it's not entirely related
to this ticket, I think it should be somewhere, but I am not sure if it
is worth a new ticket.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:13>
wxTrac
May 28, 2012, 8:54:09 AM5/28/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:14>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Comment(by vadz):
Yes, it would be definitely better to document it somewhere but the
comment in the code was not very useful, this is why I removed it...
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:14>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Comment(by vadz):
Yes, it would be definitely better to document it somewhere but the
comment in the code was not very useful, this is why I removed it...
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:14>
wxTrac
May 28, 2012, 9:02:09 AM5/28/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:15>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Comment(by PB):
Actually, you didn't remove the comment, it's still there at the top of
the method, you removed the code which attempted to fix the inconsistency
when it comes to releases passed dispatches (situation when variant
conversion failed). Well, it's not like that it fixed the issue entirely,
as the method can still exit even before that ... I guess that's for
another ticket and time then.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:15>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Comment(by PB):
Actually, you didn't remove the comment, it's still there at the top of
the method, you removed the code which attempted to fix the inconsistency
when it comes to releases passed dispatches (situation when variant
conversion failed). Well, it's not like that it fixed the issue entirely,
as the method can still exit even before that ... I guess that's for
another ticket and time then.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:15>
wxTrac
May 28, 2012, 3:37:56 PM5/28/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:16>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Comment(by vadz):
Sorry, I didn't do it intentionally then.
Anyhow, returning to `Invoke()`, I still believe that it's completely
unexpected that it changes its parameters and that we should add another
overload of it not doing this and deprecate the current one.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:16>
#14293: Resource leak in wxAutomationObject::Invoke() if argument conversion fails
---------------------+------------------------------------------------------
Type: defect | Status: closed
Priority: low | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: Automation, OLE, VARIANT, resource leak
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Comment(by vadz):
Sorry, I didn't do it intentionally then.
Anyhow, returning to `Invoke()`, I still believe that it's completely
unexpected that it changes its parameters and that we should add another
overload of it not doing this and deprecate the current one.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14293#comment:16>
Reply all
Reply to author
Forward
0 new messages