From 3f4737e49a0c9542b46b521114f98588fd0a9514 Mon Sep 17 00:00:00 2001 From: Ehsan Akhgari Date: Sun, 19 Apr 2015 13:22:35 -0400 Subject: [PATCH] Bug 1156084 - Disallow AddRef() and Release() calls on the return value of methods returning XPCOM objects; r=jrmuizel When a method returns type D derived from RefCounted type B, there is an ImplicitCastExpr (or an ExplicitCastExpr, if there is an explicit cast to the base type in the code) in the AST between the CallExpr and MemberExpr, which we didn't take into account before. This caused the analysis to not work on common patterns such as nsCOMPtr. --- build/clang-plugin/clang-plugin.cpp | 8 ++++ .../tests/TestNoAddRefReleaseOnReturn.cpp | 45 +++++++++++++++++++ dom/ipc/TabParent.cpp | 7 +-- dom/media/gmp/GMPService.cpp | 4 +- gfx/2d/DrawTargetCG.cpp | 2 +- netwerk/protocol/ftp/FTPChannelParent.cpp | 4 +- netwerk/protocol/http/HttpChannelParent.cpp | 4 +- .../websocket/WebSocketChannelParent.cpp | 4 +- .../protocol/wyciwyg/WyciwygChannelParent.cpp | 4 +- 9 files changed, 68 insertions(+), 14 deletions(-) diff --git a/build/clang-plugin/clang-plugin.cpp b/build/clang-plugin/clang-plugin.cpp index 0999789d44ec..218f8dcbaee1 100644 --- a/build/clang-plugin/clang-plugin.cpp +++ b/build/clang-plugin/clang-plugin.cpp @@ -639,11 +639,19 @@ DiagnosticsMatcher::DiagnosticsMatcher() )).bind("node"), &nanExprChecker); + // First, look for direct parents of the MemberExpr. astMatcher.addMatcher(callExpr(callee(functionDecl(hasNoAddRefReleaseOnReturnAttr()).bind("func")), hasParent(memberExpr(isAddRefOrRelease(), hasParent(callExpr())).bind("member") )).bind("node"), &noAddRefReleaseOnReturnChecker); + // Then, look for MemberExpr that need to be casted to the right type using + // an intermediary CastExpr before we get to the CallExpr. + astMatcher.addMatcher(callExpr(callee(functionDecl(hasNoAddRefReleaseOnReturnAttr()).bind("func")), + hasParent(castExpr(hasParent(memberExpr(isAddRefOrRelease(), + hasParent(callExpr())).bind("member")))) + ).bind("node"), + &noAddRefReleaseOnReturnChecker); astMatcher.addMatcher(lambdaExpr( hasDescendant(declRefExpr(hasType(pointerType(pointee(isRefCounted())))).bind("node")) diff --git a/build/clang-plugin/tests/TestNoAddRefReleaseOnReturn.cpp b/build/clang-plugin/tests/TestNoAddRefReleaseOnReturn.cpp index 8283a72583ee..2e1f83377e50 100644 --- a/build/clang-plugin/tests/TestNoAddRefReleaseOnReturn.cpp +++ b/build/clang-plugin/tests/TestNoAddRefReleaseOnReturn.cpp @@ -6,12 +6,20 @@ struct Test { void foo(); }; +struct TestD : Test {}; + struct S { Test* f() MOZ_NO_ADDREF_RELEASE_ON_RETURN; Test& g() MOZ_NO_ADDREF_RELEASE_ON_RETURN; Test h() MOZ_NO_ADDREF_RELEASE_ON_RETURN; }; +struct SD { + TestD* f() MOZ_NO_ADDREF_RELEASE_ON_RETURN; + TestD& g() MOZ_NO_ADDREF_RELEASE_ON_RETURN; + TestD h() MOZ_NO_ADDREF_RELEASE_ON_RETURN; +}; + template struct X { T* f() MOZ_NO_ADDREF_RELEASE_ON_RETURN; @@ -28,6 +36,10 @@ Test* f() MOZ_NO_ADDREF_RELEASE_ON_RETURN; Test& g() MOZ_NO_ADDREF_RELEASE_ON_RETURN; Test h() MOZ_NO_ADDREF_RELEASE_ON_RETURN; +TestD* fd() MOZ_NO_ADDREF_RELEASE_ON_RETURN; +TestD& gd() MOZ_NO_ADDREF_RELEASE_ON_RETURN; +TestD hd() MOZ_NO_ADDREF_RELEASE_ON_RETURN; + void test() { S s; s.f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}} @@ -39,6 +51,16 @@ void test() { s.h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}} s.h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}} s.h().foo(); + SD sd; + sd.f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}} + sd.f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}} + sd.f()->foo(); + sd.g().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'g'}} + sd.g().Release(); // expected-error{{'Release' cannot be called on the return value of 'g'}} + sd.g().foo(); + sd.h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}} + sd.h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}} + sd.h().foo(); X x; x.f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}} x.f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}} @@ -49,10 +71,24 @@ void test() { x.h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}} x.h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}} x.h().foo(); + X xd; + xd.f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}} + xd.f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}} + xd.f()->foo(); + xd.g().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'g'}} + xd.g().Release(); // expected-error{{'Release' cannot be called on the return value of 'g'}} + xd.g().foo(); + xd.h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}} + xd.h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}} + xd.h().foo(); SP sp; sp->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'operator->'}} sp->Release(); // expected-error{{'Release' cannot be called on the return value of 'operator->'}} sp->foo(); + SP spd; + spd->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'operator->'}} + spd->Release(); // expected-error{{'Release' cannot be called on the return value of 'operator->'}} + spd->foo(); f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}} f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}} f()->foo(); @@ -62,4 +98,13 @@ void test() { h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}} h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}} h().foo(); + fd()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'fd'}} + fd()->Release(); // expected-error{{'Release' cannot be called on the return value of 'fd'}} + fd()->foo(); + gd().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'gd'}} + gd().Release(); // expected-error{{'Release' cannot be called on the return value of 'gd'}} + gd().foo(); + hd().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'hd'}} + hd().Release(); // expected-error{{'Release' cannot be called on the return value of 'hd'}} + hd().foo(); } diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp index d927bb2a7462..466baa8a44da 100644 --- a/dom/ipc/TabParent.cpp +++ b/dom/ipc/TabParent.cpp @@ -3062,15 +3062,16 @@ public: NS_IMETHOD SetOriginalURI(nsIURI*) NO_IMPL NS_IMETHOD GetURI(nsIURI** aUri) override { - NS_IF_ADDREF(mUri); - *aUri = mUri; + nsCOMPtr copy = mUri; + copy.forget(aUri); return NS_OK; } NS_IMETHOD GetOwner(nsISupports**) NO_IMPL NS_IMETHOD SetOwner(nsISupports*) NO_IMPL NS_IMETHOD GetLoadInfo(nsILoadInfo** aLoadInfo) override { - NS_IF_ADDREF(*aLoadInfo = mLoadInfo); + nsCOMPtr copy = mLoadInfo; + copy.forget(aLoadInfo); return NS_OK; } NS_IMETHOD SetLoadInfo(nsILoadInfo* aLoadInfo) override diff --git a/dom/media/gmp/GMPService.cpp b/dom/media/gmp/GMPService.cpp index 74b1c768caf1..4d3af96b6bbb 100644 --- a/dom/media/gmp/GMPService.cpp +++ b/dom/media/gmp/GMPService.cpp @@ -283,8 +283,8 @@ GeckoMediaPluginService::GetThread(nsIThread** aThread) InitializePlugins(); } - NS_ADDREF(mGMPThread); - *aThread = mGMPThread; + nsCOMPtr copy = mGMPThread; + copy.forget(aThread); return NS_OK; } diff --git a/gfx/2d/DrawTargetCG.cpp b/gfx/2d/DrawTargetCG.cpp index 246ee4982f58..70c941594968 100644 --- a/gfx/2d/DrawTargetCG.cpp +++ b/gfx/2d/DrawTargetCG.cpp @@ -251,7 +251,7 @@ GetRetainedImageFromSourceSurface(SourceSurface *aSurface) if (!data) { MOZ_CRASH("unsupported source surface"); } - data->AddRef(); + data.get()->AddRef(); return CreateCGImage(releaseDataSurface, data.get(), data->GetData(), data->GetSize(), data->Stride(), data->GetFormat()); diff --git a/netwerk/protocol/ftp/FTPChannelParent.cpp b/netwerk/protocol/ftp/FTPChannelParent.cpp index d7c4c00e4f43..2a2bea67bf3f 100644 --- a/netwerk/protocol/ftp/FTPChannelParent.cpp +++ b/netwerk/protocol/ftp/FTPChannelParent.cpp @@ -465,8 +465,8 @@ FTPChannelParent::GetInterface(const nsIID& uuid, void** result) { // Only support nsILoadContext if child channel's callbacks did too if (uuid.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) { - NS_ADDREF(mLoadContext); - *result = static_cast(mLoadContext); + nsCOMPtr copy = mLoadContext; + copy.forget(result); return NS_OK; } diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp index 7e4dcbbbee45..8ce483145bb9 100644 --- a/netwerk/protocol/http/HttpChannelParent.cpp +++ b/netwerk/protocol/http/HttpChannelParent.cpp @@ -223,8 +223,8 @@ HttpChannelParent::GetInterface(const nsIID& aIID, void **result) // Only support nsILoadContext if child channel's callbacks did too if (aIID.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) { - NS_ADDREF(mLoadContext); - *result = static_cast(mLoadContext); + nsCOMPtr copy = mLoadContext; + copy.forget(result); return NS_OK; } diff --git a/netwerk/protocol/websocket/WebSocketChannelParent.cpp b/netwerk/protocol/websocket/WebSocketChannelParent.cpp index 2c92b8221478..af1520934589 100644 --- a/netwerk/protocol/websocket/WebSocketChannelParent.cpp +++ b/netwerk/protocol/websocket/WebSocketChannelParent.cpp @@ -306,8 +306,8 @@ WebSocketChannelParent::GetInterface(const nsIID & iid, void **result) // Only support nsILoadContext if child channel's callbacks did too if (iid.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) { - NS_ADDREF(mLoadContext); - *result = static_cast(mLoadContext); + nsCOMPtr copy = mLoadContext; + copy.forget(result); return NS_OK; } diff --git a/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp b/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp index 9f0b4f6bc2eb..6096acc6d616 100644 --- a/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp +++ b/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp @@ -343,8 +343,8 @@ WyciwygChannelParent::GetInterface(const nsIID& uuid, void** result) { // Only support nsILoadContext if child channel's callbacks did too if (uuid.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) { - NS_ADDREF(mLoadContext); - *result = static_cast(mLoadContext); + nsCOMPtr copy = mLoadContext; + copy.forget(result); return NS_OK; }