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

BugFix - NPE internalFolderSyncTimestamp & File Existence Check #13612

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Sep 24, 2024

  • Tests written, or not not needed

Changes

  1. Fix the NullPointerException (NPE) for internalFolderSyncTimestamp.
  2. Check for file existence in the InternalTwoWaySyncWork worker; otherwise, it will throw an exception.

During my tests, I wasn’t able to reproduce the issue; however, some users with version 3.30.0 Final have already experienced the crash. It means some OCFile already saved into the internal storage via null value.

Potential fix could be in this PR:

Changing following set function, but I’m not sure.

ocFile.setInternalFolderSyncTimestamp(fileEntity.getInternalTwoWaySync()); ->

ocFile.setInternalFolderSyncTimestamp(nullToZero(fileEntity.getInternalTwoWaySync()));

To prevent further crashes, I checked nullability of the Long object and return non-null object. Alternative solution can be we can convert the Long object to long primitive type.

@superzanti
Copy link

Thank you!

This fixed it for me.

@superzanti superzanti mentioned this pull request Sep 24, 2024
1 task
@alperozturk96 alperozturk96 marked this pull request as draft September 25, 2024 07:04
@alperozturk96 alperozturk96 changed the title BugFix - NPE internalFolderSyncTimestamp BugFix - NPE internalFolderSyncTimestamp & File Existence Check Sep 25, 2024
@alperozturk96 alperozturk96 marked this pull request as ready for review September 25, 2024 10:22
@alperozturk96 alperozturk96 marked this pull request as draft September 27, 2024 07:04
@tobiasKaminsky
Copy link
Member

Draft or ready to review? 🙈

@alperozturk96
Copy link
Collaborator Author

@tobiasKaminsky We have to test two-way sync properly before merge this PR, so it's a draft.

@alperozturk96 alperozturk96 marked this pull request as ready for review September 30, 2024 12:14
@alperozturk96 alperozturk96 marked this pull request as draft September 30, 2024 12:14
@alperozturk96 alperozturk96 marked this pull request as ready for review September 30, 2024 12:55
@nextcloud nextcloud deleted a comment from github-actions bot Sep 30, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Sep 30, 2024
@@ -89,6 +81,25 @@ class InternalTwoWaySyncWork(
}
}

private fun checkFreeSpace(folder:OCFile): Result? {
folder.storagePath?.let { storagePath ->
Copy link
Member

Choose a reason for hiding this comment

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

If folder does not yet exist, we can still compute it:

  • localFolderSize is 0, as nothing was downloaded yet

Rest of computation would then still apply.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@alperozturk96 alperozturk96 Sep 30, 2024

Choose a reason for hiding this comment

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

If the file does not exist, freeSpaceLeft will be 0 because file.getFreeSpace() will return 0.

Therefore, the condition if (freeSpaceLeft < (remoteFolderSize - localFolderSize)) will evaluate to true, as 0 < some_value - 0 is always true, causing the worker to be canceled. I assume remoteFolderSize going to be bigger than 0.

…ullable value

Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/13612.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

Copy link

Codacy

Lint

TypemasterPR
Warnings5959
Errors33

SpotBugs

CategoryBaseNew
Bad practice6464
Correctness6363
Dodgy code296296
Experimental11
Internationalization77
Malicious code vulnerability11
Multithreaded correctness66
Performance5353
Security1818
Total509509

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