Saving to content:// is still broken

[Alexander--]

I have downloaded the latest apk and looked at the current code. It seems, that saving to content:// was not fixed and the broken behavior was made even more broken in recent changes by @martincz

1. The text viewer still assumes that uri.getPath == path to local file. This is obviously not the case: if, say, opencloud ContentProvider exposes uri where uri.getPath == /mnt/storage/example.txt, that does not mean that you should mess with /mnt/storage/example.txt on local drive... What if the uri path accidentally happens to match something on local system? Say, /dev/block/vdc? Trying to blindly open that file with root shell (before even asking ContentProvider for file descriptor!) is not a good idea...

2. The same goes for filename. The documentation of OpenableColumns#FILE_NAME clearly states:

   The human-friendly name of file. If this is not provided then the name should default to the the last segment of the file's URI.

   E.g. you should query the ContentProvider for OpenableColumns#FILE_NAME before assuming that getLastPathSegment() can be used as file name.

3. No check for null from openFile: if ContentProvider does not return a file, your app crashes.

4. If something happen to process (see p. 3), the file manager starts to crash upon reopening:

    java.lang.NullPointerException: Attempt to invoke virtual method 'boolean eu.chainfire.libsuperuser.Shell$Interactive.isRunning()' on a null object reference

[VishalNehra]

1. If I try to open the file descriptor before doing anything else gives a problem, i.e. if it does succeed to open a read-only descriptor, then other rw opening methods would not work (like opening file from root). I could remove the read-only descriptor all together, but then would obviously be problem for content providers exposing only readable uri. If you could point to a solution, I'll be happy to implement.
2. I removed querying from else condition and is now the preferred method, after saving the file name from getLastPathSegment()
3. Checks are there, see line # 685 in TextReader.java
4. Shell commands are run only when root is available and shell is running. I'll put on null check just to be safe.

[Alexander--]

@vishal0071 I am not talking about opening. I am talking about this line. You should not assume that uri.getPath() contains path to file on disk. "The path is a an absolute path to file and last segment is filename" rule applies to file:// uris only. Content uris may look like this:

content://com.blabla.provider/1223423523#rw

Some of them may look like file:// uris e.g.

content://com.blabla.provider//mnt/sdcard/1223423523.txt#rw

But you can not make any assumptions about their structure.

Checks are there, see line # 685 in TextReader.java

Not sure why, but during my test the apk failed with following exception:

java.lang.NullPointerException: Attempt to invoke virtual method 'int android.os.ParcelFileDescriptor.getFd()' on a null object reference
                                                                       at com.amaze.filemanager.activities.TextReader.getInputStream(TextReader.java:704)
                                                                       at com.amaze.filemanager.activities.TextReader.access$400(TextReader.java:95)
                                                                       at com.amaze.filemanager.activities.TextReader$4.run(TextReader.java:487)

[VishalNehra]

We're not assuming uri.getpath is path to disk. If you check further, that's what checks are for, both while getting InputStream and OutputStream. If the uri indeed is from content provider, then the path returned by ````uri.getPath``` would fail to get stream using basic filesystem methods, and when it does that, then only we look for the FileDescriptors as rw, and finally as r as last resort.

So basically, when reading, we check for
if(can't write && root mode) // create a cache file by reading from root
else (can read) // open FileInputStream directly

if(failed above) try // opening rw ParcelFileDescriptor
catch // opening ro ParcelFileDescriptor
if(failed in getting FD (maybe couldn't read /proc/self/fd)) // ContentResolver#openInputStream

So, you see, basic file system will automatically fail when we have a Uri from ContentProviders, and we would fall back to FDs. Similarly for getting OutputStream.

PS. the only pitfall that seems to me is, when you have rootmode and you get a Uri from ContentProvider. Then first check would obviously be triggered and we would try to copy the path from uri into cache. But since that is not a real path, an empty file would be created? Hence a stream pointing to empty file, maybe that's where you case lies?
Pps. Will something like Uri#isRelative work to find out such condition? (like when file path is in file system, isRelative will return true otherwise isAbsolute will return true)?

[Alexander--]

Hence a stream pointing to empty file, maybe that's where you case lies?

More or less, you got it.

Will something like Uri#isRelative work to find out such condition?

No, this won't work. content:// uri is a random sequence of symbols. It is opaque. It is not guaranteed to encode any information about itself or it's relation to filesystem files. Any time it accidentally matches something on disk, a bug in your program may get triggered.

You seem to assume, that ContentProviders are some dumb mechanism to read regular files. This is not true. ContentProvider is Android answer to Unix shell command pipes. They can convert/transform input on fly. When Unix admin wants to view a pdf in console, they may run something like this is Bash:

wget "http://mysite.net/docs/s.pdf" | pdf2text

Android allows ContentProviders to do the same: one ContentProvider may download content from web on fly (e.g. Google Drive content provider) while another one converts it to text on fly. And the resulting uri (referring to pipe) may be passed to your editor.

If you get a uri, referring to pdf file on disk, you current code will try to circumvent the converting ContentProvider and directly load the pdf file file from local disk… This is a bug. This means that your content:// support is basically useless: it works for a couple of simple ContentProviders with well-known mechanics (which are already supported by your root subsystem), but won't work for most powerful/useful third party content providers, and will generate bugs when users try to use it with them.

[Alexander--]

@vishal0071 Regarding /proc/self/fd/ — you don't actually have to use it if all you need is a FileInputStream. There is a constructor in FileInputStream/FileOutputStream, that accepts a FileDescriptor (can be obtained from ParcelFileDescriptor#getFileDescriptor). The resulting stream will wrap the descriptor without owning it (e.g. it won't close the descriptor when closed). /proc/self/fd is mainly there to integrate with native code and classes like RandomAccessFile, that understand only file paths.

[VishalNehra]

Alright, I didn't know FileInputStream/FileOutputStream support this kind of constructor. So we can use RandomAccessFile successfully using Fd at this path? I'm curious to try opening channels for OTG and SD Card. Currently we're using Buffered Streams for all I/O operations (using contentresolver#openInputStream / contentresolver#openOutputStream)

Ps. I didn't get these statements by you quite much

If you get a uri, referring to pdf file on disk, you current code will try to circumvent the converting   
ContentProvider and directly load the pdf file file from local disk… This is a bug. This means that your 
content:// support is basically useless: it works for a couple of simple ContentProviders with well-known 
mechanics (which are already supported by your root subsystem), but won't work for most     
powerful/useful third party content providers, and will generate bugs when users try to use it with them.

I hope you're not referring here to the new code, and are actually referring to the old code which didn't handle uri from content:// at all. As I assume content provider exposes uri only when it has processed the file (with whatever it was doing, whether downloading it or writing it). So I assume consuming the ParcelFileDescriptor is safe way to handle this.

[Alexander--]

Alright, I didn't know FileInputStream/FileOutputStream support this kind of constructor. So we can use RandomAccessFile successfully using Fd at this path?

Sorry, it seems I have misled you: it is possible to use RandomAccessFile with /proc/self/fd/ paths, but only under the correct circumstances:

1. The file descriptors in question correspond to ordinary seekable files, not pipe or socket (File#isFile returns true).
2. The descriptor was open with appropriate access flags (e.g. "rw")
3. You have the requires access (read/write) to the corresponding files on disk (File#canRead/File#canWrite returns true).

I assume content provider exposes uri only when it has processed the file

It seems, our previous discussion has somehow gotten over your head… Do you know, what a "socket" is? It is a special thing, used to get data stuff from network. The entire file does not get transferred instantly: the server is writing it at the same time as client is reading. Piece by piece. This is how video streaming works. Pipes are the same as sockets except between local processes. When you read from pipe, another side is writing to it. If the other side does not have enough data to fill the pipe/socket buffer, your reading thread may be blocked until the other side writes some more. Pipes and sockets are file descriptors.

ContentProvider may be streaming it's data to you. This is the point of using InputStream/OutputStream: no need to store entire thing somewhere, it can be generated little by little. When you open a uri, the ContentProvider may send you either normal file descriptor, suitable for use with RandomAccessFile, or ParcelFileDescriptor, referring to a side of pipe. (ParcelFileDescriptor#createPipe()).

[VishalNehra]

You're right! I totally overlooked sockets and pipe and assumed everything to be file, my bad.
But since OTG and SD Cards don't deal with these, and do have seekable streams, I assume it's worth trying to get a channel to them?
Besides, I'm working on creating a content provider myself. Hope this discussion with you comes in handy. Hope you won't mind if I consult with you privately in case I get stuck somewhere? Can't thank you enough for your guidance.

[VishalNehra]

BTW test whether the issue is been fixed with v3.1.2 beta 9

[Alexander--]

Besides, I'm working on creating a content provider myself. Hope this discussion with you comes in handy.

@vishal0071 What a coincidence — I am also writing one right now (actually there is both provider and a library for creating such providers). When finished, is might be able to resolve #405

BTW test whether the issue is been fixed with v3.1.2 beta 9

It still crashes when null ParcelFileDescriptor is returned:

Process: com.amaze.filemanager, PID: 22896
             java.lang.NullPointerException: Attempt to invoke virtual method 'int android.os.ParcelFileDescriptor.getFd()' on a null object reference
                                         at com.amaze.filemanager.activities.TextReader.getInputStream(TextReader.java:693)
                                         at com.amaze.filemanager.activities.TextReader.access$400(TextReader.java:90)
                                         at com.amaze.filemanager.activities.TextReader$4.run(TextReader.java:466)
                                         at java.lang.Thread.run(Thread.java:818)

[Alexander--]

Have there been any progress on this? Is there a newer version I can test?

By the way, the above-mentioned ContentProvider is finished.

[VishalNehra]

I'm sorry. Got caught up on content provider. Will work on this.
BTW what's your provider about?

[Alexander--]

@vishal0071 It allows opening files via su. That is: a caller invokes ContentResolver#openFileDescriptor or ContentResolver#open*Stream and receives a descriptor/stream for desirable system file, such as /system/etc/hosts. The file does not have to be copied/transferred by pipe, instead the returned descriptor is a genuined "rw" descriptor, referring to original. This works with all kinds of files, including special files in/sys/, /proc/ and /dev/.

[VishalNehra]

That's great! Looking forward to using it with Amaze (: