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

Use truncate mode to replace the contents of existing files #5663

Merged
merged 2 commits into from Mar 31, 2022

Conversation

cketti
Copy link
Contributor

@cketti cketti commented Mar 29, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Instances where ContentResolver.openOutputStream(Uri) is used to potentially open existing files for writing have been fixed.

Motivation and context

ContentResolver.openOutputStream(Uri) does not truncate existing files. If the amount of data written is smaller than the file size, you end up with new data at the beginning of the file followed by old data at the end of the file.

Some places where ContentResolver.openOutputStream(Uri) is used have not been changed because they only deal with newly created files and are thus not a problem.

Tests

  1. Create a large text file with arbitrary content on your Android device (1 MB should do)
  2. Go to Settings → Security & Privacy
  3. Select Export E2E room keys
  4. In the system file picker, select the file created in step 1, confirm that you want to overwrite the file
  5. Complete the export flow (enter passwords, etc)
  6. Notice that the file size hasn't changed. Only the beginning of the file has been overwritten with the exported data.

The other export functionality can be tested in a similar way.

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 12

Checklist

Signed-off-by: cketti <ck@cketti.de>

@cketti cketti marked this pull request as ready for review March 29, 2022 15:41
@ouchadam ouchadam added the Z-Community-PR Issue is solved by a community member's PR label Mar 29, 2022
`ContentResolver.openOutputStream(Uri)` does not truncate existing files. If the amount of data written is smaller than the file size, you end up with new data at the beginning of the file followed by old data at the end of the file.
@ouchadam ouchadam added the X-Needs-Product Issue needs input from Product team label Mar 30, 2022
@ouchadam
Copy link
Contributor

looping in product, we might want to warn the user if we're overwriting files

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

I understand the pb and I think you fixed it properly.

The file replacement warning is handled by the system, so this is OK to me.

image

Thanks!

@@ -34,7 +34,7 @@ class KeysExporter @Inject constructor(
suspend fun export(password: String, uri: Uri) {
withContext(dispatchers.io) {
val data = session.cryptoService().exportRoomKeys(password)
context.contentResolver.openOutputStream(uri)
context.contentResolver.openOutputStream(uri, "wt")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe creating a quick extension like this could help to avoid code duplication:

    fun Context.safeOpenOutputStream(uri: Uri): OutputStream? {
        return contentResolver.openOutputStream(uri, "wt")
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍
Done

@ouchadam ouchadam removed the X-Needs-Product Issue needs input from Product team label Mar 30, 2022
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the fast update!

@bmarty bmarty merged commit 0fe3cc3 into element-hq:develop Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants