Closed Bug 565392 Opened 10 years ago Closed 10 years ago

Potential leak in nsTransferable::GetTransferData

Categories

(Core :: DOM: Serializers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: mats, Assigned: mats)

References

Details

(Keywords: memory-leak)

Attachments

(3 files)

It looks like we have a potential leak in nsTransferable::GetTransferData:
http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsTransferable.cpp?mark=318-318#306
DataStruct::GetData addrefs aData which may be overwritten without release.

(FYI, kFlavorHasDataProvider == 0,  )

Two cases: 
1. If *aData QI:s to nsIFlavorDataProvider and *aDataLen = 0
   then "dataProvider->GetFlavorData" assigns *aData (leak), or
   if it fails, falls through to the "format converter", see 2.
2. If *aData does not QI to nsIFlavorDataProvider, *aDataLen = 0
   then we fall through to the "format converter" block which may assign
   *aData (leak)
Attached patch wip2Splinter Review
The nsHTMLFormatConverter::Convert change isn't related to the leak
but apparently some callers (notably nsTransferable::GetTransferData)
uses the out params also if it returns an error.

The added null-checks of 'dataBytes' (in two places):
-        if (len == kFlavorHasDataProvider) {
+        if (len == kFlavorHasDataProvider && dataBytes) {
isn't strictly necessary since the block doesn't do anything if
'dataBytes' doesn't QI to nsIFlavorDataProvider, but the null-check
makes the code more readable IMO, since it's the condition for when
there can be a provider.
Assignee: nobody → matspal
Attachment #448455 - Flags: review?(dbaron)
Comment on attachment 448455 [details] [diff] [review]
wip2

>     if ( data.GetFlavor().Equals(aFlavor) ) {
>-      data.GetData(aData, aDataLen);
>-      if (*aDataLen == kFlavorHasDataProvider) {
>+      nsCOMPtr<nsISupports> dataBytes;
>+      data.GetData(getter_AddRefs(dataBytes), aDataLen);
>+      if (*aDataLen == kFlavorHasDataProvider && dataBytes) {
>         // do we have a data provider?
>-        nsCOMPtr<nsIFlavorDataProvider> dataProvider = do_QueryInterface(*aData);
>+        nsCOMPtr<nsIFlavorDataProvider> dataProvider = do_QueryInterface(dataBytes);
>         if (dataProvider) {
>-          rv = dataProvider->GetFlavorData(this, aFlavor, aData, aDataLen);
>+          rv = dataProvider->GetFlavorData(this, aFlavor,
>+                                           getter_AddRefs(dataBytes), aDataLen);
>           if (NS_FAILED(rv))
>             break;    // the provider failed. fall into the converter code below.
>         }
>       }
>-      if (*aData && *aDataLen > 0)
>+      if (dataBytes && *aDataLen > 0) { // XXXmats why is zero length not ok?
>+        dataBytes.forget(aData);
>         return NS_OK;
>+      }

I think it would be clearer if you also used a separate dataLen variable, and only assigned *aDataLen = dataLen in this success case.  It's odd to use one of the out params and not the other.

r=dbaron with that
Attachment #448455 - Flags: review?(dbaron) → review+
Yeah, good point.  I did so but I named it 'len' to match the latter part
of the method.
http://hg.mozilla.org/mozilla-central/rev/b6126b690acd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
This has caused the "make check" test TestWinDND.cpp fail on Thunderbird builds:

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1275848328.1275853372.32138.gz

Running Test Windows Drag and Drop tests...
NEXT ERROR TEST-UNEXPECTED-FAIL | File data object did not provide data on request
TEST-UNEXPECTED-FAIL | Drag object tests failed on a single file
TEST-PASS | Successfully created a working string drag object!
TEST-PASS | Successfully set arbitrary data on a drag object
NEXT ERROR TEST-UNEXPECTED-FAIL | File data object did not provide data on request
TEST-UNEXPECTED-FAIL | Drag object tests failed on multiple files
TEST-PASS | Successfully created a working multiple string drag object!
TEST-PASS | Successfully set arbitrary data on a drag object
Finished running Test Windows Drag and Drop tests.

Note that in the makefile this test is ifndef MOZ_ENABLE_LIBXUL so it is quite conceivable that this would fail on Firefox if build in the --disable-libxul state.
Can we disable the test or back the patch out for now?
(In reply to comment #5)
> Can we disable the test or back the patch out for now?

(Sorry, that's assuming there's no obvious fix that can be applied)
I wrote the test in question.  I haven't investigated it much, but that's likely to be an actual regression.  I would suggest backing out, but the tree is closed.  I'll spin up a non-libxul build and check it out.  I'm glad somebody still runs non-libxul tests!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Taking to investigate.
Assignee: matspal → me
This works in Firefox.  I'll investigate why it fails in Thunderbird, but for now I'd suggest disabling the test (since it's not built on any tested configuration in Firefox) when m-c reopens.
The problem is that some consumers ignores the return value
and uses the nsISupports out param anyway. E.g.
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.cpp#1327
So this seems to be a supported but undocumented part of the API.

This patch restores that behavior.
(I'm returning NS_ERROR_FAILURE for this case as it did
before, even though NS_OK would seem more logical.)
Assignee: me → matspal
Attachment #449608 - Flags: review?(dbaron)
(I'll attempt to write a JS unit test for this)
Attached patch mochitestSplinter Review
Comment on attachment 449608 [details] [diff] [review]
Fix for the regression

Why can't we fix the bad callers?
I'd like to fix the regression here first, please.  I agree we should fix
the callers, but I'd like to do that in a separate bug since it requires
more investigation of this case.
After discussion with mats on irc, I've temporarily disabled TestWinDND.cpp in mozilla-central whilst this is resolved (especially as it doesn't run for Firefox anyway):

http://hg.mozilla.org/mozilla-central/rev/545c6d13e8e8
Depends on: 570774
The regression fix should block alpha 5, it's causing the second highest or highest OS X topcrash.
blocking2.0: --- → ?
Comment on attachment 449608 [details] [diff] [review]
Fix for the regression

ok, r=dbaron

(Are we sure this will fix the topcrash, or should we temporarily back out the original?)
Attachment #449608 - Flags: review?(dbaron) → review+
blocking2.0: ? → alpha5+
Can we get this locally applied and the STR in bug 570774 retried to verify that it fixes the issue? And, uh, checked in? :)
Filed bug 571074 to fix nsTransferable::GetTransferData callers.

I verified it fixed bug 570774 locally.

http://hg.mozilla.org/mozilla-central/rev/b3404e64afad
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Did you reenable the test?
I'm about to, just need to star some orange first...
It's been decided that we will not hold alpha 5 for this, moving blocker flag to beta 1.
Status: RESOLVED → REOPENED
blocking2.0: alpha5+ → beta1+
Resolution: FIXED → ---
Re-enabled the TestWinDND.cpp test:
http://hg.mozilla.org/mozilla-central/rev/2392ce8a0d3b
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
For the record, the fix for this bug *did* get into alpha 5 on the Mac, which was based on the 2010-06-10 nightly.
You need to log in before you can comment on or make changes to this bug.