1423961 - Improve NS_MutateURI.Finalize by passing reference to nsCOMPtr instead nsIURI**

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1423961 - Improve NS_MutateURI.Finalize by passing reference to nsCOMPtr instead nsIURI**

Review commit: https://reviewboard.mozilla.org/r/206310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206310/

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8935417 [details]
Bug 1423961 - Improve NS_MutateURI.Finalize by passing reference to nsCOMPtr instead nsIURI**

https://reviewboard.mozilla.org/r/206310/#review211950

::: netwerk/base/nsIURIMutator.idl:295
(Diff revision 1)
>    {
>      NS_ENSURE_SUCCESS(mStatus, mStatus);
> -    mStatus = mMutator->Finalize(aURI);
> +    nsCOMPtr<nsIURI> uri;
> +    mStatus = mMutator->Finalize(getter_AddRefs(uri));
> +    NS_ENSURE_SUCCESS(mStatus, mStatus);
> +    aURI = do_QueryInterface(uri, &mStatus);

if the comptr passed here is |this| and there is just a single reference then this line will release that only reference and delete |this|!

nice footgun

I think this is why we have getter_AddRefs at the first place? ;)

and I think this renders this whole bug a wontfix?

::: netwerk/test/gtest/TestStandardURL.cpp:256
(Diff revision 1)
> -  nsCOMPtr<nsIURI> uri2;
>    rv = NS_MutateURI(uri)
>           .SetScheme(NS_LITERAL_CSTRING("ftp"))
>           .SetHost(NS_LITERAL_CSTRING("mozilla.org"))
>           .SetPathQueryRef(NS_LITERAL_CSTRING("/path?query#ref"))
> -         .Finalize(getter_AddRefs(uri2));
> +         .Finalize(uri);

this should crash in debug builds (but maybe only in tsan builds, sometimes use after free are not caught)

Comment 3

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> > +    aURI = do_QueryInterface(uri, &mStatus);
> 
> if the comptr passed here is |this| and there is just a single reference
> then this line will release that only reference and delete |this|!

I really wasn't intending for it to be used inside a nsIURI implementation, so `this` really shouldn't be passed to finalize.
But also, I don't think you can pass this to Finalize, as this would not be an nsCOMPtr, right?

> I think this is why we have getter_AddRefs at the first place? ;)

That actually caused an even weirder bug when it got passed the same URI that we we're trying to mutate.
I think it's because the compiler would notice (nsIURI*) being passed to the constructor and (nsIURI**) passed to Finalize, and decided that the first ptr must be null.

> > +         .Finalize(uri);
> 
> this should crash in debug builds (but maybe only in tsan builds, sometimes
> use after free are not caught)

Why would it crash?
What freed memory is it using?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #2)
> > > +    aURI = do_QueryInterface(uri, &mStatus);
> > 
> > if the comptr passed here is |this| and there is just a single reference
> > then this line will release that only reference and delete |this|!
> 
> I really wasn't intending for it to be used inside a nsIURI implementation,
> so `this` really shouldn't be passed to finalize.
> But also, I don't think you can pass this to Finalize, as this would not be
> an nsCOMPtr, right?
> 
> > I think this is why we have getter_AddRefs at the first place? ;)
> 
> That actually caused an even weirder bug when it got passed the same URI
> that we we're trying to mutate.
> I think it's because the compiler would notice (nsIURI*) being passed to the
> constructor and (nsIURI**) passed to Finalize, and decided that the first
> ptr must be null.


getter_AddRefs() releases the passed in comptr/refptr before entering the method/function.  so it can easily release the url before its methods are called...  actually, quite the same thing... so, yeah, getter_AddRefs() can screw things as well..


> 
> > > +         .Finalize(uri);
> > 
> > this should crash in debug builds (but maybe only in tsan builds, sometimes
> > use after free are not caught)
> 
> Why would it crash?
> What freed memory is it using?



To explain:

nsCOMPtr<nsIURI> uri = ..create a new uri..
// uri.refcnd == 1

rv = NS_MutateURI(uri).someChane().finalize(uri);

what exactly happens:

  nsresult Finalize(nsCOMPtr<C>& aURI)
  {
    NS_ENSURE_SUCCESS(mStatus, mStatus);
    nsCOMPtr<nsIURI> uri;
    mStatus = mMutator->Finalize(getter_AddRefs(uri));
// uri is a new instance of MozURI (or what is the class name)
    NS_ENSURE_SUCCESS(mStatus, mStatus);
    aURI = do_QueryInterface(uri, &mStatus);
// aURI.mRawPtr == this!  and this line does:
// aURI.release() -- last reference, hence it effective |delete this;|
    NS_ENSURE_SUCCESS(mStatus, mStatus);
// access to released memory
    return mStatus;
// another one
  }

maybe if you just swap the pointers (uri with aURI) instead of directly assinging would save you.


I may need to understand what is the exact motivation for this bug.  what are you trying to solve actually?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> To explain:
> 
> nsCOMPtr<nsIURI> uri = ..create a new uri..
> // uri.refcnd == 1
> 
> rv = NS_MutateURI(uri).someChane().finalize(uri);
> 
> what exactly happens:
> 
>   nsresult Finalize(nsCOMPtr<C>& aURI)
>   {
>     NS_ENSURE_SUCCESS(mStatus, mStatus);
>     nsCOMPtr<nsIURI> uri;
>     mStatus = mMutator->Finalize(getter_AddRefs(uri));
> // uri is a new instance of MozURI (or what is the class name)
>     NS_ENSURE_SUCCESS(mStatus, mStatus);
>     aURI = do_QueryInterface(uri, &mStatus);
> // aURI.mRawPtr == this!  and this line does:
> // aURI.release() -- last reference, hence it effective |delete this;|
>     NS_ENSURE_SUCCESS(mStatus, mStatus);

mStatus is a member of NS_MutateURI, so even if the initial URI gets released, that's OK I think.

> // access to released memory
>     return mStatus;
> // another one
>   }
> 
> maybe if you just swap the pointers (uri with aURI) instead of directly
> assinging would save you.

I could do that, but I don't think that is really necessary. What's happening here is:

rv = NS_MutateURI(uri).someChane().finalize(uri);

NS_MutateURI is created, which keeps a ref to an nsIURIMutator which keeps a clone of the initial URI
We use nsIURIMutator methods to change the clone of the initial URI
We call finalize. The initial URI gets released, and `uri` now points to the changed clone.
Since mStatus is in NS_MutateURI, it's OK to access it after releasing the initial URI.

> I may need to understand what is the exact motivation for this bug.  what
> are you trying to solve actually?

Exactly this case, so you wouldn't have to keep  a ref to both the previous and the changed URI, when all you need is the changed one.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #5)
> mStatus is a member of NS_MutateURI, so even if the initial URI gets
> released, that's OK I think.

Stupid me!  yes!

Thanks.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8935417 [details]
Bug 1423961 - Improve NS_MutateURI.Finalize by passing reference to nsCOMPtr instead nsIURI**

https://reviewboard.mozilla.org/r/206310/#review212364

::: netwerk/base/nsIURIMutator.idl:291
(Diff revision 1)
>    }
> -  nsresult Finalize(nsIURI** aURI)
> +
> +  template <class C>
> +  nsresult Finalize(nsCOMPtr<C>& aURI)
>    {
>      NS_ENSURE_SUCCESS(mStatus, mStatus);

just please add blank lines after each NS_ENSURE to read the code better

::: netwerk/base/nsIURIMutator.idl:297
(Diff revision 1)
> -    mStatus = mMutator->Finalize(aURI);
> +    nsCOMPtr<nsIURI> uri;
> +    mStatus = mMutator->Finalize(getter_AddRefs(uri));
> +    NS_ENSURE_SUCCESS(mStatus, mStatus);
> +    aURI = do_QueryInterface(uri, &mStatus);
> +    NS_ENSURE_SUCCESS(mStatus, mStatus);
>      return mStatus;

return NS_OK?

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8935417 [details]
Bug 1423961 - Improve NS_MutateURI.Finalize by passing reference to nsCOMPtr instead nsIURI**

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206310/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6cd64922e36
Improve NS_MutateURI.Finalize by passing reference to nsCOMPtr instead nsIURI** r=mayhemer

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e6cd64922e36