do_QueryInterface abuse in nsAndroidHandlerApp.cpp

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: neil, Assigned: Paolo)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

The code

nsCOMPtr<nsAndroidHandlerApp> aApp = do_QueryInterface(aHandlerApp);

which appears in nsAndroidHandlerApp.cpp is an abuse of do_QueryInterface, since nsAndroidHandlerApp is not an interface, so what happens is that aHandlerApp gets reinterpreted as an nsAndroidHandlerApp*.

The accessor methods on the nsIHandlerApp parameter should probably be used instead.
The code

nsCOMPtr<nsMIMEInfoAndroid::SystemChooser> info = do_QueryInterface(aHandlerApp);

which appears in nsMIMEInfoAndroid.cpp is also an abuse of do_QueryInterface for similar reasons.
Blocks: 514280
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Blocks: 1355585, 1287658
Comment on attachment 8858369 [details]
Bug 986975 - do_QueryInterface abuse in nsAndroidHandlerApp.cpp.

https://reviewboard.mozilla.org/r/130316/#review134894

::: uriloader/exthandler/android/nsAndroidHandlerApp.cpp:66
(Diff revision 1)
> -  nsCOMPtr<nsAndroidHandlerApp> aApp = do_QueryInterface(aHandlerApp);
> -  *aRetval = aApp && aApp->mName.Equals(mName) &&
> -    aApp->mDescription.Equals(mDescription);
> +  *aRetval = false;
> +  if (!aHandlerApp) {
> +    return NS_OK;
> +  }
> +
> +  nsString name;

how long are these names usually? If on average it's small (less than 64 iirc), an AutoString could make sense

::: uriloader/exthandler/android/nsMIMEInfoAndroid.cpp:414
(Diff revision 1)
>    *aRetVal = false;
> +  if (!aHandlerApp) {
> +    return NS_OK;
> +  }
> +
> +  nsString name;

ditto

::: uriloader/exthandler/android/nsMIMEInfoAndroid.cpp:420
(Diff revision 1)
> +  nsString detailedDescription;
> +  aHandlerApp->GetName(name);
> +  aHandlerApp->GetDetailedDescription(detailedDescription);
> +
> +  *aRetVal = name.Equals(u"Android chooser") &&
> +             detailedDescription.Equals(u"Android's default handler application chooser");

these should probably use EqualsLiteral

Both strings should be moved to a #define or a static const char16_t*, so that there's no risk of modifying one instance and not the other one.
All the usage is in this same file.
Attachment #8858369 - Flags: review?(mak77) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd40784fb5b
do_QueryInterface abuse in nsAndroidHandlerApp.cpp. r=mak
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=93684714&repo=mozilla-inbound
Flags: needinfo?(paolo.mozmail)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9588216af214
Backed out changeset 8bd40784fb5b for bustage
(In reply to Marco Bonardo [::mak] from comment #3)
> > +  *aRetVal = name.Equals(u"Android chooser") &&
> > +             detailedDescription.Equals(u"Android's default handler application chooser");
> 
> these should probably use EqualsLiteral

Looks like Android doesn't like EqualsLiteral. I'll re-land the original version with Equals.
Flags: needinfo?(paolo.mozmail)
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d04ed8f11abe
do_QueryInterface abuse in nsAndroidHandlerApp.cpp. r=mak
https://hg.mozilla.org/mozilla-central/rev/d04ed8f11abe
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.