Skip to content

Address a crash when close is called from finalize. #311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 6, 2017

Conversation

wongk
Copy link
Contributor

@wongk wongk commented Mar 6, 2017

On Android, finalize has 10 seconds to complete. SQLiteProgram.close is being called from finalize, and it does not consider whether it has already been closed before it tries to acquire the database lock. If the database is in use elsewhere, this can easily cause finalize to timeout, causing a fatal exception. This fixes the issue by making a second call to close a no-op.

On Android, finalize has 10 seconds to complete. SQLiteProgram.close is being called from finalize, and it does not consider whether it has already been closed before it tries to acquire the database lock. If the database is in use elsewhere, this can easily cause finalize to timeout, causing a fatal exception. This fixes the issue by making a second call to close a no-op.
@developernotes
Copy link
Member

Hello @wongk,

Thank you for submitting this pull request. We ask that you submit a contributor agreement in order for us to accept this request. More information about this agreement can be found here. Please reach out to us at [email protected] for more information. Thanks!

@developernotes
Copy link
Member

Hello @wongk

Also, is this something that you can prepare a test for which can be included in the SQLCipher for Android test suite?

@wongk
Copy link
Contributor Author

wongk commented Mar 9, 2017

@developernotes I am a VMware employee, and my request to have the CLA completed is currently working its way through "the machine."

@wongk
Copy link
Contributor Author

wongk commented Mar 9, 2017

@developernotes I will investigate the test suite. Thank you for the suggestion.

@wongk
Copy link
Contributor Author

wongk commented Mar 13, 2017

@developernotes I am having a good bit of trouble trying to build the test project. Since it appears to be a maven project, I am using the command 'mvn compile', and receiving the following error:

[ERROR] Failed to execute goal com.jayway.maven.plugins.android.generation2:maven-android-plugin:2.8.4:generate-sources (default-generate-sources) on project net.zetetic.sqlcipher.test: Execution default-generate-sources of goal com.jayway.maven.plugins.android.generation2:maven-android-plugin:2.8.4:generate-sources failed: Could not find tool 'aapt'. Please provide a proper Android SDK directory path as configuration parameter ... in the plugin . As an alternative, you may add the parameter to commandline: -Dandroid.sdk.path=... or set environment variable ANDROID_HOME. -> [Help 1]

I have set the ANDROID_HOME env variable to reflect my "command line tools" version of ADT, and also updated my PATH to pick up those tools first.

@developernotes
Copy link
Member

Hello @wongk

Have you, or can you attempt to import the pom.xml as a project within IntelliJ to see if you are able to build the project through the IDE?

@wongk
Copy link
Contributor Author

wongk commented Mar 13, 2017

@developernotes I just tried that and instead received a number of 'cannot find symbol' errors related to generated resource classes (R).

@wongk
Copy link
Contributor Author

wongk commented Mar 13, 2017

Adding a local.properties and specifying 'sdk.dir' didn't seem to make any difference either.

@brody4hire
Copy link

I am afraid it would not be possible to reproduce the reported issue within the test suite since finalize is a protected method which is generally triggered by the Android system. I would personally favor including the proposed close guard along with a test that covers the changes. Some hints on adding to the test suite:

I think the proposed changes along with test coverage would lead to more deterministic and tested behavior in this kind of scenario.

@wongk
Copy link
Contributor Author

wongk commented Mar 14, 2017

@brodybits Thanks for the direction and I am on the same page about test coverage. Unfortunately, I cannot build the tests, so I am unable to add new ones.

@wongk
Copy link
Contributor Author

wongk commented Mar 27, 2017

An update on this, we have the necessary approvals from within the organization, but we are having trouble getting the necessary executive to sign the CLA.

@developernotes developernotes merged commit fda559f into sqlcipher:master Apr 6, 2017
@g123k
Copy link

g123k commented May 2, 2017

Hello,

I am using the latest version available (3.5.7) and I still have this issue in production.
Here is the stacktrace :

Exception: java.util.concurrent.TimeoutException: net.sqlcipher.database.SQLiteCompiledSql.finalize() timed out after 10 seconds
at java.lang.Object.wait(Object.java)
at java.lang.Thread.parkFor(Thread.java:1205)
at sun.misc.Unsafe.park(Unsafe.java:325)
at java.util.concurrent.locks.LockSupport.park(LockSupport.java:
at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:813)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:846)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1175)
at java.util.concurrent.locks.ReentrantLock$FairSync.lock(ReentrantLock.java:195)
at java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:256)
at net.sqlcipher.database.SQLiteDatabase.lock(SQLiteDatabase.java:553)
at net.sqlcipher.database.SQLiteCompiledSql.releaseSqlStatement(SQLiteCompiledSql.java:106)
at net.sqlcipher.database.SQLiteCompiledSql.finalize(SQLiteCompiledSql.java:152)
at java.lang.Daemons$FinalizerDaemon.doFinalize(Daemons.java:187)
at java.lang.Daemons$FinalizerDaemon.run(Daemons.java:170)
at java.lang.Thread.run(Thread.java:841)

@wongk
Copy link
Contributor Author

wongk commented May 2, 2017

Yes, my change specifically addressed SQLiteProgram, not SQLiteCompiledSql.

@g123k
Copy link

g123k commented May 2, 2017

Ok, I will open a new issue for this one then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants