565392 - Potential leak in nsTransferable::GetTransferData

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Mats Palmgren (inactive)]

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)

Comment 1

[Mats Palmgren (inactive)]

Attachment: wip2

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Mark Banner (:standard8)]

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.

Comment 5

[Mark Banner (:standard8)]

Can we disable the test or back the patch out for now?

Comment 6

[Mark Banner (:standard8)]

(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)

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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!

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Taking to investigate.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Fix for the regression

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.)

Comment 11

[Mats Palmgren (inactive)]

(I'll attempt to write a JS unit test for this)

Comment 12

[Mats Palmgren (inactive)]

Attachment: mochitest

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 449608 [details] [diff] [review]
Fix for the regression

Why can't we fix the bad callers?

Comment 14

[Mats Palmgren (inactive)]

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.

Comment 15

[Mark Banner (:standard8)]

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

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The regression fix should block alpha 5, it's causing the second highest or highest OS X topcrash.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?)

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get this locally applied and the STR in bug 570774 retried to verify that it fixes the issue? And, uh, checked in? :)

Comment 19

[Mats Palmgren (inactive)]

Filed bug 571074 to fix nsTransferable::GetTransferData callers.

I verified it fixed bug 570774 locally.

http://hg.mozilla.org/mozilla-central/rev/b3404e64afad

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you reenable the test?

Comment 21

[Mats Palmgren (inactive)]

I'm about to, just need to star some orange first...

Comment 22

[Johnny Stenback (:jst)]

It's been decided that we will not hold alpha 5 for this, moving blocker flag to beta 1.

Comment 23

[Mats Palmgren (inactive)]

Re-enabled the TestWinDND.cpp test:
http://hg.mozilla.org/mozilla-central/rev/2392ce8a0d3b

Comment 24

[Steven Michaud [:smichaud] (Retired)]

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.