After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 340228 - gnome_vfs_open() accept to open a directory for non-file: URI
gnome_vfs_open() accept to open a directory for non-file: URI
Status: RESOLVED WONTFIX
Product: gnome-vfs
Classification: Deprecated
Component: Module: ssh/sftp
2.15.x
Other All
: Normal normal
: ---
Assigned To: gnome-vfs maintainers
gnome-vfs maintainers
gnome[unmaintained]
Depends on:
Blocks:
 
 
Reported: 2006-04-30 21:43 UTC by Sebastien Bacher
Modified: 2018-08-17 13:49 UTC
See Also:
GNOME target: ---
GNOME version: 2.15/2.16


Attachments
patch made from head (1.07 KB, patch)
2006-06-18 17:05 UTC, Matthieu Baechler
needs-work Details | Review
second try (1.75 KB, patch)
2006-07-19 20:28 UTC, Matthieu Baechler
none Details | Review

Description Sebastien Bacher 2006-04-30 21:43:30 UTC
That bug has been described on https://launchpad.net/distros/ubuntu/+source/totem/+bug/41437

"Totem doesn't handle sftp directory properly
...
I use gnome-vfs to access some remote files on a ssh server.
I can drag and drop files into totem playlist, but droping a directory doesn't work.

I get the following message :

"
Totem could not play 'sftp://XXX@XXX:22/mnt/musique/'.

could not read from resource
""

totem-pl-parser does that:

"	/* Open the file. */
	result = gnome_vfs_open (&handle, uri, GNOME_VFS_OPEN_READ);
	if (result != GNOME_VFS_OK) {
	  g_print("result: %d\n", result);
		if (result == GNOME_VFS_ERROR_IS_DIRECTORY)
			return DIR_MIME_TYPE;
		return NULL;
	}"

the gnome_vfs_open accepts to open a vfs directory but not on a file: URI
Comment 1 Matthieu Baechler 2006-06-18 17:03:55 UTC
I took a look at this bug.

In OpenSSH, the sftp command SSH2_FXP_OPEN has no special case for treating directories.
It just makes an "open" call, and return a handle.

So, the only way for GnomeVFSMethodOpenFunc method to handle that case is to make a call at GnomeVFSMethodGetFileInfoFromHandleFunc, and check the "type" field.

It's not a good thing from a latency point of view, as it makes one more round-trip, but it's the only way I found for correct behaviour.
Comment 2 Matthieu Baechler 2006-06-18 17:05:25 UTC
Created attachment 67586 [details] [review]
patch made from head

Implement my previous comment.
Comment 3 Christian Neumair 2006-07-13 16:20:13 UTC
Thanks for your efforts!

Unfortunately, you forgot to close the valid handle when returning with an error. The handler is not expected to close it when we return with an error.

Otherwise, I think the patch is correct.
Comment 4 Christian Neumair 2006-07-13 16:20:51 UTC
Are you willing to rework the patch according to my suggestion? :)
Comment 5 Matthieu Baechler 2006-07-15 17:41:30 UTC
Of course I will rework my patch. Thanks for the revue.

I'm not sure to understand your comment :

>Unfortunately, you forgot to close the valid handle when returning with an
> error. The handler is not expected to close it when we return with an error.

Which handle are you talking about ? do you mean method_handle should be set to NULL ?

And another question : do I need to unref the connection as it's done a few line below ?
Comment 6 Christian Neumair 2006-07-16 06:56:53 UTC
> Which handle are you talking about ?

do_open() is an equivalent to the UNIX fopen().

When iobuf_read_handle succeeds, you have a handle pointing to an open file (similar to fopen()). Locally, you fclose() it again. Because you flag failure to the caller, he doesn't expect to receive a file handle at all. Thus, you should do the same as in the iobuf failure case, but also close the handle:



* call do_close (method, method_handle, NULL);

note that cancellation is NULL because cancellation isn't
really desirable here (it might leak the handle), and
we don't unref the connection in do_open because do_close
does that already.

* set the method handle to NULL

* return GNOME_VFS_ERROR_IS_DIRECTORY;

I also think that you shouldn't return res, but rather put the whole checks before

return GNOME_VFS_OK;

so that failure when getting the file info aren't passed to the caller (where he already has a valid file handle!).
Comment 7 Matthieu Baechler 2006-07-16 12:30:11 UTC
(In reply to comment #6)
> * call do_close (method, method_handle, NULL);

I think I understand your advice, but what should I do when do_close fail ?
I'm a bit suprised that do_close doesn't always succeed.

> I also think that you shouldn't return res, but rather put the whole checks
> before
> 
> return GNOME_VFS_OK;
> 
> so that failure when getting the file info aren't passed to the caller (where
> he already has a valid file handle!).

Do you mean the lack of result from do_get_info_from_handle shouldn't raise an error from do_open ?
Comment 8 Christian Neumair 2006-07-16 19:29:00 UTC
> I think I understand your advice, but what should I do when do_close fail ?

Ignore it and return GNOME_VFS_ERROR_IS_DIRECTORY. The worst thing that could happen is that we leak a handle on the server until the end of the session, so no data loss is involved.

> Do you mean the lack of result from do_get_info_from_handle shouldn't raise an
error from do_open ?

Exactly.

Oh, and I forgot to mention that

 if (res == GNOME_VFS_OK && file_info.type == GNOME_VFS_FILE_TYPE_DIRECTORY)

should rather be

 if (res == GNOME_VFS_OK &&
     (file_info.valid_fields & GNOME_VFS_FILE_INFO_FIELDS_TYPE != 0) &&
     file_info.type == GNOME_VFS_FILE_TYPE_DIRECTORY)

Thanks in advance for your efforts :).
Comment 9 Christian Kellner 2006-07-16 19:41:09 UTC
Actually this:
---- SNIP -----
GnomeVFSFileInfo file_info;
GnomeVFSResult res;
res = do_get_file_info_from_handle (method, *method_handle, &file_info,
                                    GNOME_VFS_FILE_INFO_DEFAULT, context);
if (res == GNOME_VFS_OK && file_info.type == GNOME_VFS_FILE_TYPE_DIRECTORY) {
---- SNAP -----
is wrong also. 
GnomeVfsFileInfo has internal refcounting and stuff and needs to be initialized. That is the reason why you should rather do a:

GnomeVFSFileInfo *file_info;
file_info = gnome_vfs_file_info_new ();
[...]
gnome_vfs_file_info_unref (file_info);

;-D
Comment 10 Christophe Fergeau 2006-07-17 08:46:31 UTC
(In reply to comment #9)

> GnomeVfsFileInfo has internal refcounting and stuff and needs to be
> initialized. That is the reason why you should rather do a:
> 
> GnomeVFSFileInfo *file_info;
> file_info = gnome_vfs_file_info_new ();
> [...]
> gnome_vfs_file_info_unref (file_info);


If I'm not mistaken (ie I haven't checked with the source), GnomeVFSFileInfo file_info = {0, }; might do the trick as well.

Comment 11 Christian Kellner 2006-07-17 11:40:26 UTC
Teuf, yep, you are right, but I wanna discourage people from doing so. GnomeVFSFileInfo is a pseudo object and should be treated as such. ;-D
Comment 12 Matthieu Baechler 2006-07-19 20:28:54 UTC
Created attachment 69195 [details] [review]
second try

Thanks for all the comments and advices, I hope it makes my contribution better.
Comment 13 Matthieu Baechler 2006-07-20 21:36:52 UTC
can someone take a look at my second try patch ?
Comment 14 Christian Kellner 2006-07-20 23:16:43 UTC
If we really wanna do this expensive stat commmand on each open we should do it before we open the file. I don't see any reason to do it the other way around. Is there?
Comment 15 Christian Neumair 2006-07-21 08:12:11 UTC
Newer SFTP protocol versions [1] force the server to return

SSH_FX_FILE_IS_A_DIRECTORY           24

when

SSH_FXP_OPEN                3

is applied to a directory.

We may want to wrap this status code to

GNOME_VFS_ERROR_IS_DIRECTORY

and figure out what version the server uses, so that we don't have to apply this hack when it supports this stuff.


[1] since version 6, i.e. http://tools.ietf.org/wg/secsh/draft-ietf-secsh-filexfer/draft-ietf-secsh-filexfer-07.txt
Comment 16 André Klapper 2018-08-17 13:49:58 UTC
gnome-vfs got deprecated in 2008.

gnome-vfs is not under active development anymore and had its last code changes
in 2011. Its codebase has been archived:
https://gitlab.gnome.org/Archive/gnome-vfs/commits/master

gio (in glib) and gvfs are its successors. See https://developer.gnome.org/gio/stable/ch33.html and https://people.gnome.org/~gicmo/gio-migration-guide/ for porting info.

Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect
reality. Feel free to open a task in GNOME Gitlab if the issue described in this task still applies to a recent + supported version of glib/gio/gvfs. Thanks!