Skip to content
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

All statements and queries are closed in finally statements #132

Merged

Conversation

dryganets
Copy link
Contributor

SQLiteCipher crashes/hangs in cases when you don't close prepared statements correctly.

In this commit, I'm moving all cursors close statements inside of finally block.
Also missing close statements are added.

@dryganets dryganets mentioned this pull request Feb 22, 2017
} catch (IOException ignored) {
}
}
closeQuietly(in);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this!

@andpor
Copy link
Owner

andpor commented Feb 23, 2017

Excellent - all these make common sense. This cleanup was overdue. Great work Sergei! Do you have any more PR coming?

@andpor andpor merged commit 3530fe5 into andpor:master Feb 23, 2017
@dryganets dryganets deleted the sergeyd/statements-and-queries-close-fixed branch February 24, 2017 04:01
@dryganets
Copy link
Contributor Author

This is all I have for now.
I pushed sqlite cipher to my forks branch. It's a very small diff. Will do more cleanup later on. Thank you for quick merge of changes!

@andpor
Copy link
Owner

andpor commented Feb 24, 2017

@dryganets Sergey - if it is a small diff maybe it would make sense to incorporate it somehow to this module...I would have no objections actually..can you suggest something?

@dryganets
Copy link
Contributor Author

Well, the problem is not in the Dependency size, but in the library size. If you don't need encryption you don't want to have an extra mb to your apk.

We need to make a plugin model for Android with default implementation using system library.

@brodycj
Copy link
Contributor

brodycj commented Feb 24, 2017

@dryganets Sergey - if it is a small diff maybe it would make sense to incorporate it somehow to this module...I would have no objections actually..can you suggest something?

Also see #40 (comment):

I would recommend keeping encryption separate. When publishing apps with encryption people would have to follow the guide at https://discuss.zetetic.net/t/export-requirements-for-applications-using-sqlcipher/47 and it may be even worse in places like France.

@itinance
Copy link
Contributor

According to @brodybits arguments: can we leave this optional per #ifdef direction on objC-Side and something similar on android?
For those who need encryption they can activate it then easily and others don't have to ship one more MB with the app and aren't bothered by the export-requirements for apps using cipher

@andpor
Copy link
Owner

andpor commented Feb 25, 2017

I have to think about this. @brodybits touches on import/export compliance which is quite important...

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