1287219 - swap with raw pointer local in nsFtpProtocolHandler::RemoveConnection()

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Andrew McCreight [:mccr8]]

The end of the method does this:
    // swap connection ownership
    *_retval = ts->conn;
    ts->conn = nullptr;

The type of ts->conn is RefPtr<>, while _retval is nsFtpControlConnection**. This copies out the pointer, but without addrefing the connection. Then the RefPtr<> is nulled out, which will destroy the connection if this is the last reference, but we still have a pointer to it. (Maybe there's always something else holding the connection alive?)

I believe this is the result of a botched fix to a leak in bug 194402:
https://github.com/mozilla/gecko-dev/commit/bfd31e83de3c4f88305a43f5006980b90601e6f9#diff-7954d49dd5596a143a51909f88f19127R287

In fact, I believe the real cause of that leak is in nsFtpState::EstablishControlConnection():
http://hg.mozilla.org/mozilla-central/file/tip/netwerk/protocol/ftp/nsFtpConnectionThread.cpp#l265

The code basically does this:
    nsFtpControlConnection *connection = nullptr;
    ...
    gFtpHandler->RemoveConnection(mChannel->URI(), &connection);
    ...
    mControlConnection.swap(connection);

Because connection is not a RefPtr, the "swap" puts the new value from connection into mControlConnection, without addrefing it, which will eventually cause an extra Release() when it cleared. The old value of mControlConnection gets put into connection, and nothing happens to it, so we leak it.

Found by code inspection.

Comment 1

[Andrew McCreight [:mccr8]]

Doug, do you have any thoughts here? It would be nice to fix this, but I'm also wary of touching code that isn't well-tested.

Comment 2

[Patrick McManus [:mcmanus]]

my policy is that we only take security fixes here. this looks tractable enough that we can do it safely. I would trust honza to work it out if he's willing..

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Andrew McCreight [:mccr8] from comment #0)
> The end of the method does this:
>     // swap connection ownership
>     *_retval = ts->conn;
>     ts->conn = nullptr;
> 
> The type of ts->conn is RefPtr<>,

It's not (up to date m-c):

    // Stuff for the timer callback function
    struct timerStruct {
        nsCOMPtr<nsITimer>      timer;
>       nsFtpControlConnection *conn;
        char                   *key;
        
        timerStruct() : conn(nullptr), key(nullptr) {}
        
        ~timerStruct() {
            if (timer)
                timer->Cancel();
            if (key)
                free(key);
            if (conn) {
                conn->Disconnect(NS_ERROR_ABORT);
                NS_RELEASE(conn);
            }
        }
    };


> while _retval is nsFtpControlConnection**.
> This copies out the pointer, but without addrefing the connection. Then the
> RefPtr<> is nulled out, which will destroy the connection if this is the
> last reference, but we still have a pointer to it. (Maybe there's always
> something else holding the connection alive?)
> 
> I believe this is the result of a botched fix to a leak in bug 194402:
> https://github.com/mozilla/gecko-dev/commit/
> bfd31e83de3c4f88305a43f5006980b90601e6f9#diff-
> 7954d49dd5596a143a51909f88f19127R287
> 
> In fact, I believe the real cause of that leak is in
> nsFtpState::EstablishControlConnection():
> http://hg.mozilla.org/mozilla-central/file/tip/netwerk/protocol/ftp/
> nsFtpConnectionThread.cpp#l265
> 
> The code basically does this:
>     nsFtpControlConnection *connection = nullptr;
>     ...
>     gFtpHandler->RemoveConnection(mChannel->URI(), &connection);
>     ...
>     mControlConnection.swap(connection);
> 
> Because connection is not a RefPtr, the "swap" puts the new value from
> connection into mControlConnection, without addrefing it, which will
> eventually cause an extra Release() when it cleared. The old value of
> mControlConnection gets put into connection, and nothing happens to it, so
> we leak it.
> 
> Found by code inspection.

Has this all been reproduced or is this bug based only on your code inspection?

This all sounds like we want to make sure everything is on RefPtrs.  That's not a small task.  But I can do it.

Comment 4

[Patrick McManus [:mcmanus]]

I haven't really assessed this myself, but if this can be fixed up with a mutex that would probably be preferable to any kind of investment in the ftp code. Its definitely a low value thing. (unfortunately not low enough value to just toss it to an addon - though if we had a reasonable js addon to ship as a default system addon that would be attractive.. its just also a fairly sizable project that shouldn't be considered if this can be patched up)

Comment 5

[Andrew McCreight [:mccr8]]

> It's not (up to date m-c):
Oh, that's good. I'm not sure why I thought it was a RefPtr. Therefore this isn't a security issue. Sorry for my mistake.

> Has this all been reproduced or is this bug based only on your code inspection?
Code inspection.

The only remaining problem would be a leak from |connection|, but it looks like the invariant of this function is that mControlConnection is null, so if that holds, it would be okay.

It would still be nice if |connection| could be changed to a RefPtr<>. That would be a small local change. I'm trying to get rid of all uses of |RefPtr::swap(T*& aRhs)| because things can go very badly with it if you aren't careful.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1 (timerStruct::conn made a refptr, connection local made refptr)

this could be it.

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8773849 [details] [diff] [review]
v1 (timerStruct::conn made a refptr, connection local made refptr)

Review of attachment 8773849 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really tempted to WONTFIX any FTP changes at this point that aren't demonstrable problems simply on a risk/reward basis. But the total system benefit is probably worth it here..

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec891a342ef7
Fix leak of nsFtpControlConnection in nsFtpProtocolHandler::RemoveConnection, r=mcmanus

Comment 9

[Carsten Book [:Tomcat]]

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