911109 - Statement::internalFinalize can cause crashes if the connection is already closed

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

In bug 910226, in his great wisdom, asuth said
> So, it seems like the death problem is definitely that
> 
> Statement::internalFinalize called by Statement::~Statement does:
>
>  int srv = ::sqlite3_finalize(mDBStatement);
>  mDBStatement = nullptr;
>
> regardless of whether the connection is alive or not.  It needs to check the
> connection's closed state.  (The connection must still be alive.)
>
> It seems like we can add a unit test to verify this becomes a non-crasher:
> - Create sync connection
> - Create sync statement, keep it alive
> - Run asyncClose on connection, wait for the close to notify as complete.
> - Finalize sync statement.  Don't crash.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Note that we'll need to be careful to ensure that we have no race condition between asyncClose and internalFinalize.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Making sure that we don't finalize a statement after the connection is closed

Attaching a first version.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=182f890696d3

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 798525 [details] [diff] [review]
Making sure that we don't finalize a statement after the connection is closed

Looks, good, but I think we need a comment/readability fix.  Specifically, this fixes the following case too that a naive reading of the code/patch suggests it should not:

1) Create sync statement.  (Do not create any async statements.)
2) Close connection.  Now succeeds where previously it failed, leaving an orphaned statement.
3) Finalize sync statement.

I just skimmed the unit tests, and I don't think we have a test for the former, so we should probably add it too.

The above synchronous case should work out fine because we set 'mAsyncExecutionThreadShuttingDown' to true, with proper mutex holding, in Connection::setClosedState which is called by Connection::Close.

I suggest that we rename isAsyncClosing to just isClosing to avoid the confusion and add some comments at mAsyncExecutionThreadShuttingDown's definition site to indicate that it is also set by the synchronous close process as well.  (Which makes sense, since we don't want async things to start happening to a closed connection.)

Clearing the review request of :mak since I'm assuming this was a case where you just wanted whomever would review it first, but please re-add mak if you did want his review too.  (And :mak, feel free to chime in either way! :)

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Making sure that we don't finalize a statement after the connection is closed, v2

Applied feedback, added more doc.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 798781 [details] [diff] [review]
Making sure that we don't finalize a statement after the connection is closed, v2

r=asuth.  I love the comments!

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=4b7d01048f70

Thanks :)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/617107cdc0f9

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/617107cdc0f9

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 798781 [details] [diff] [review]
Making sure that we don't finalize a statement after the connection is closed, v2

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

::: storage/src/mozStorageStatement.cpp
@@ +358,5 @@
>      return NS_OK;
>  
> +  int srv = SQLITE_OK;
> +
> +  if (!mDBConnection->isClosing(true)) {

How did we determine that the connection is closing at this point (reason for which we pass a true argument)?
internalFinalize can be invoked at any time, even with a well alive connection, just by invoking finalize().
Maybe it's just the argument naming that is strange?

Basically here the async thread is not shutting down yet and connectionReady is still returning true (I think this is somehow related to bug 726990), you added an argument that basically should be named aIgnoreConnectionReady.  I'm pretty sure to be missing something, but I don't easily understand the approach. I wonder if we should rather make connectionReady a bit more strict (like, once asyncClose has been invoked, it returns false).

That said, it's not a big deal to keep it as is, I just find that the new isClosing argument looks strange. If I'm somehow right, it would be fine to clenaup the thing in a new separate bug.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(In reply to Marco Bonardo [:mak] from comment #11)
> Comment on attachment 798781 [details] [diff] [review]
> Making sure that we don't finalize a statement after the connection is
> closed, v2
> 
> Review of attachment 798781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: storage/src/mozStorageStatement.cpp
> @@ +358,5 @@
> >      return NS_OK;
> >  
> > +  int srv = SQLITE_OK;
> > +
> > +  if (!mDBConnection->isClosing(true)) {
> 
> How did we determine that the connection is closing at this point (reason
> for which we pass a true argument)?
> internalFinalize can be invoked at any time, even with a well alive
> connection, just by invoking finalize().
> Maybe it's just the argument naming that is strange?

The |true| only asks |isClosing| to return |true| if the connection is either closing or closed, which is what we are interested in.
Passing |false| or nothing keeps the existing default behavior, i.e. return |true| if the connection is closing, |false| if it is already closed.

> Basically here the async thread is not shutting down yet and connectionReady
> is still returning true (I think this is somehow related to bug 726990), you
> added an argument that basically should be named aIgnoreConnectionReady.

Well, yes I did.

> I'm pretty sure to be missing something, but I don't easily understand the
> approach. I wonder if we should rather make connectionReady a bit more
> strict (like, once asyncClose has been invoked, it returns false).
> 
> That said, it's not a big deal to keep it as is, I just find that the new
> isClosing argument looks strange. If I'm somehow right, it would be fine to
> clenaup the thing in a new separate bug.

Fine with me.

Comment 13

[Marco Bonardo [:mak]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> The |true| only asks |isClosing| to return |true| if the connection is
> either closing or closed, which is what we are interested in.
> Passing |false| or nothing keeps the existing default behavior, i.e. return
> |true| if the connection is closing, |false| if it is already closed.

passing |true| means "ignore che connection ready status, just return true unless the async thread is gone" afaict, there is no check that the connection is closing or closed.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

The async thread naming there is somewhat of a misnomer for mAsyncExecutionThreadShuttingDown since a fully sync connection's call to close will set the same flags thanks to internalClose.  I talked about this a little in comment 5, I believe.