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

Writing to external storage using SAF leads to file corruption if the filename includes square brackets [] #337

Open
r3a1d3a1 opened this issue Mar 26, 2024 · 15 comments
Milestone

Comments

@r3a1d3a1
Copy link

It doesn't happen when writing to internal storage using PlainOldFS.
The client I'm using is ncftp. The server (code: 63) runs on an Nvidia Shield (OS 9.1). The external storage is formatted as exFat.
The corruption is silent, hence no errors on client or server side. It's in the form of missing the first 100KBs of a file of 100MBs size.
Removing the brackets from the filename solves the issue, but since the corruption happens silently, it's quite dangerous.

@wolpi
Copy link
Owner

wolpi commented Mar 29, 2024

What could the app do about it?
Automatically renaming files?
In all cases?
Are there other characters that cause such an issue?
Would escaping help?

People might whant to keep their brakcets in filenames in working cases ...

@r3a1d3a1
Copy link
Author

Silent corruption is the worst. At the very least, it can throw an error telling the user to rename their files appropriately.
Ideally, see where in the code brackets are mishandled and skip them with a backslash for example.

@wolpi
Copy link
Owner

wolpi commented Mar 31, 2024

Silent corruption is the worst.

yes, that is surly true.

At the very least, it can throw an error telling the user to rename their files appropriately.

Hhmm, not sure if a specific error message would make it's way through to be displayed on client. The server could log such cases or show them as toast. But both can easily been overseen.

I'll check error message handling.

see where in the code brackets are mishandled

Are you sure the root cause is not in android itself?

and skip them with a backslash for example.

That would be escaping. The question here is: what kind of escaping would help? Backslash would be a common escaping mechanism. It's not clear if it would help here. And it is also unclear if other characters would be affected, too.

@wolpi
Copy link
Owner

wolpi commented Mar 31, 2024

Ok, error messages make it through to client. Therfore checking for known bad characters and failing has been added.

@wolpi wolpi added this to the 7.1 milestone Mar 31, 2024
@r3a1d3a1
Copy link
Author

r3a1d3a1 commented Apr 1, 2024

After more testing:
* and ? also cause silent corruption, despite getting automatically changed to _.
\ erases the whole name up to and including itself.
Btw, is unicode supported?

@wolpi
Copy link
Owner

wolpi commented Apr 13, 2024

  • and ? also cause silent corruption,

good to know.

Btw, is unicode supported?

Depends on where you look at. As this app is written in java the code itself supprots unicode. But it is not clear to me how protocols (ftp and sftp) handle encodings (can be figured out, takes some time) and what SAF actually does.

@gogiii
Copy link

gogiii commented Aug 23, 2024

@wolpi I've been uploading files with square brackes using SAF for years with this server, never had any problem and now I just can't upload such files. (Version: 7.2 (code: 65), Android 14)
It was hard to find this ticket as for me it was just: "The connection with the server was reset" in Windows file explorer, while "501 Invalid character in command" in FAR manager for filenames I've been previously uploading fine (meaning that they are not invalid).
The reporter should try other ftp clients before reporting this and show an exact command he's using for the upload.
His explanation about other symbols (*, ?, ) also bothers me as there is probably some misundertanding of how he should escape these symbols in the command line of the ncftpput instead of suggesting additional handling to the server.
Also exFat potentially means he's using external storage that could be a cheap SD card that may be failing and corrupting files at random moment by itself.

@r3a1d3a1
Copy link
Author

@gogiii You idiot, the very first line of my issue is "It doesn't happen when writing to internal storage using PlainOldFS", and WTF exFat has to do with SD card?! It's actually a HDD.
I agree that the maintainer put shit effort into resolving this properly, but your assumptions are more like a 2 year old, just like your profile pic.

@gogiii
Copy link

gogiii commented Aug 24, 2024

@r3a1d3a1 rofl. Not happens both for plainoldfs and saf for me on internal storage. The brackets are legal symbols for exfat, so potentially the problem is your device, sdcard/hdd or the way you use the command which you still didn't present to us.

@r3a1d3a1
Copy link
Author

My device is perfect :P You're lying.

@lmagyar
Copy link
Contributor

lmagyar commented Aug 29, 2024

I've tried to find any spec that says [] are illegal characters in file names (SFTP, FTP, Android DocumentFile), but can't.

Even tested on Android 9, SD card, FAT32: Total Commander renames files to/from with [] chars in their name.

So i think this filename checking should be reverted, the root cause seems to be somewhere else.

@r3a1d3a1
Copy link
Author

Nowhere in this issue did wolpi or I mention these chars to be illegal.
But while the underlying culprit is not found, it's DANGEROUS to allow these since they cause silent failures.

image

@lmagyar
Copy link
Contributor

lmagyar commented Aug 30, 2024

Then we need your logs that shows what is the exact root cause of the issue, because we can't reproduce this. This is dangerous on your system, it should be handled by the server only if this is a general problem. If it fails only there, why should we disable this everywhere?

So instead of dumb pictures, plese share some logs.

@r3a1d3a1
Copy link
Author

r3a1d3a1 commented Aug 31, 2024

It's unlikely to find anything in the logs since it finishes "successfully". Best way to triage is to get hands-on with an Android TV or preferably an Nvidia Shield. In the meantime, since it can cause irreversible damage, it's best to keep it contained the way it is. How would you like your files getting screwed w/o you even knowing?!

@wolpi
Copy link
Owner

wolpi commented Sep 1, 2024

I'm going to allow those 2 characters again. As next release is still months away it will stay for some time as is.

@wolpi wolpi modified the milestones: 7.1, 7.3 Sep 1, 2024
@wolpi wolpi reopened this Sep 1, 2024
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

No branches or pull requests

4 participants