Bug 202192

Summary: [PATCH] x11-fm/thunar 1.6.11 change file permissions on sshfs mounted files/dirs
Product: Ports & Packages Reporter: rozhuk.im
Component: Individual Port(s)Assignee: freebsd-xfce mailing list <xfce>
Status: Open ---    
Severity: Affects Many People CC: madpilot, olivierd, rozhuk.im
Priority: --- Flags: madpilot: maintainer-feedback+
Version: Latest   
Hardware: Any   
OS: Any   
URL: https://bugzilla.xfce.org/show_bug.cgi?id=12863
Attachments:
Description Flags
permission fix and respect HAVE_GIO_UNIX
none
patch using statfs(2) none

Description rozhuk.im 2015-08-09 02:40:31 UTC
I have mount point:
sshfs root@172.16.0.254:/ '/home/rim/mnt/172.16.0.254' -o reconnect -o workaround=all -o transform_symlinks -o default_permissions -o hard_remove -o noauto_cache

chmod 0777 /home/rim/mnt/172.16.0.254/test.txt
change file permission, but in Thunar all controls on Permissions tab in file Properties - disabled.
Comment 1 rozhuk.im 2017-05-09 02:34:58 UTC
Created attachment 182427 [details]
permission fix and respect HAVE_GIO_UNIX

This fix my issue with unable to set permissions.
Also some build issue then GIO not set.
Comment 2 rozhuk.im 2017-08-20 21:31:06 UTC
ping
Comment 3 Guido Falsi freebsd_committer 2017-11-13 17:21:43 UTC
Hi,

The GIO part of your patch seems to be already present in the newer upstream code, even if the implementation is a little different there:

https://git.xfce.org/xfce/thunar/commit/thunar/thunar-sendto-model.c?id=1f5eed407c762bf7ca118e377f91c6cb1e63eb31

regarding the rest of your patch, you are removing some upstream logic and replacing it with a very simplistic condition.

While I believe it works for your use case, I don't feel comfortable changing it for all users in such a way without thorough testing, or at least a good understanding of why the upstream had those more complex conditions.

I'll leave this open and try to study the source, time allowing, but it will take some time to figure out. Also, I don't know sshfs, and maybe it's a very specific thing.

BTW have you seen the wrong behavior without this patch appear in any other filesystem besides sshfs?
Comment 4 rozhuk.im 2017-11-28 15:15:08 UTC
IMHO they dont try build without HAVE_GIO_UNIX: GDesktopAppInfo is part of gio and hey will get undefined struct/type compiler error.
They patch does not work (without mine) - 100%.

Upstream do many insane and crappy things, I dont want understand why :)

I replace
effective_user_id == g_file_info_get_attribute_uint32 (file->info, G_FILE_ATTRIBUTE_UNIX_UID)
to
thunar_file_is_writable(file)

All other logic not changed, just refactored to human understanding.

You can use both checks via ||, like:
return (effective_user_id == g_file_info_get_attribute_uint32 (file->info, G_FILE_ATTRIBUTE_UNIX_UID) || thunar_file_is_writable(file))

May be in some cases there different access levels for file data and metadata and original code will return TRUE is user is file owner or root.
I dont know what use cases this cover, probably none.

My check is simple: if file writeable than we can write metadata.
This is good for most cases.
I dont know how to allow user write data and restrict write metadata in unix with chmod().

In my case sshfs mounted to me (simple user), by authorized on remote side as root.
Comment 5 Guido Falsi freebsd_committer 2017-11-28 16:09:37 UTC
(In reply to rozhuk.im from comment #4)
> IMHO they dont try build without HAVE_GIO_UNIX: GDesktopAppInfo is part of
> gio and hey will get undefined struct/type compiler error.
> They patch does not work (without mine) - 100%.
> 
> Upstream do many insane and crappy things, I dont want understand why :)

First of all a general clarification:

I can understand but keep in mind that ports are just that, portings.

The development should happen upstream and bugfixes should go there where there are developers who know how the existing code works and also usually have better ability to test changes.

This kind of changes which diverge from upstream tend to make maintain a port difficult and could also cause regressions which end up being unjustly reported upstream casting shadows on FreeBSD porting methods.

I think you understand divergence with upstream should be kept to a minimum.

So as a general rule I'm accepting such changes only if they are for very important bugs and have good chance of being merged upstream, or are very FreeBSD specific (which sometimes hinders upstream inclusion).


For the GIO part, how do I test in the ports tree with GIO disabled? The port does not have such an option, so it looks like we don't cover this condition either.

> 
> I replace
> effective_user_id == g_file_info_get_attribute_uint32 (file->info,
> G_FILE_ATTRIBUTE_UNIX_UID)
> to
> thunar_file_is_writable(file)
> 
> All other logic not changed, just refactored to human understanding.
> 
> You can use both checks via ||, like:
> return (effective_user_id == g_file_info_get_attribute_uint32 (file->info,
> G_FILE_ATTRIBUTE_UNIX_UID) || thunar_file_is_writable(file))
> 
> May be in some cases there different access levels for file data and
> metadata and original code will return TRUE is user is file owner or root.
> I dont know what use cases this cover, probably none.
> 
> My check is simple: if file writeable than we can write metadata.
> This is good for most cases.
> I dont know how to allow user write data and restrict write metadata in unix
> with chmod().

As I said while I hear and understand your explanation I have no explanation about the existing code.

I'm not refusing your code, and have no specific objection about it.
I just need time to understand it properly.

In the while if you can get your code included upstream or at least reviewed by an upstream developer that would greatly help this request.

> 
> In my case sshfs mounted to me (simple user), by authorized on remote side
> as root.

Not sure I understand, you mean that you are a plain user on the local machine but authenticated as root on the ssh mount on the remote one?
Comment 6 rozhuk.im 2017-11-28 18:18:07 UTC
(In reply to Guido Falsi from comment #5)

https://bugzilla.xfce.org/show_bug.cgi?id=12863
year+ - no activity in upstream, is there are life?

Yes, our port does not have GIO option, I do tests by add to makefile GIO option and uncheck it.


I dont know what I need to do with upstream, they do not react on bug tacker.


Yes, I mean that Im plain user that have sshfs root@remote mount.


PS: miss null check
return (thunar_file_is_writable(file) ||
(NULL != file->info && effective_user_id == g_file_info_get_attribute_uint32 (file->info, G_FILE_ATTRIBUTE_UNIX_UID)))
Comment 7 Guido Falsi freebsd_committer 2017-11-28 19:19:52 UTC
(In reply to rozhuk.im from comment #6)
> (In reply to Guido Falsi from comment #5)
> 
> https://bugzilla.xfce.org/show_bug.cgi?id=12863
> year+ - no activity in upstream, is there are life?

This is irrelevant and in fact against your interest to point this out.

If a patch is not accepted upstream maybe there are reasons for that.

Also the fact the patch is not accepted upstream, even if accepting without question it to be good, is no reason to push it in FreeBSD. It could add unexpected effects.

It will also make updating the port more difficult. The patched code could be modified upstream and the patch need reconciliation, or changes upstream can cause the patch to break.

> 
> Yes, our port does not have GIO option, I do tests by add to makefile GIO
> option and uncheck it.

So the GIO case is irrelevant to our ports tree. No need to diverge to fix a problem that cannot happen.

> 
> 
> I dont know what I need to do with upstream, they do not react on bug tacker.
> 

While I understand your frustration I have no connection with the upstream.

If you get your code accepted by upstream that is proof of a minimum quality of the change and also will mean thee code will anyway get in the ports tree, so there is no reason to hold the change.

If you cannot some FreeBSD committer needs to test check and make a decision, this takes time. This is a volunteer project and no one is paid for it or working full time on port bugs. Things may require time.

Please give me time to evaluate your code properly.

This is a "port" of the upstream code not original code. Any divergence needs a good motivation.
Comment 8 Guido Falsi freebsd_committer 2017-11-28 20:59:18 UTC
(In reply to rozhuk.im from comment #4)

> My check is simple: if file writeable than we can write metadata.
> This is good for most cases.
> I dont know how to allow user write data and restrict write metadata in unix
> with chmod().

Looking at the upstream code and checking how UFS works I can now say this is definitely a wrong assumption.

you can chmod any file you own, even if it is not writable, otherwise there would be no way to modify permissions on a file with 0444 permissions. obviously root can change permissions on any file.

The upstream code, while a little convoluted, checks just for this, which is correct for local file systems.

> 
> In my case sshfs mounted to me (simple user), by authorized on remote side
> as root.

Considering the previous correction this use case is quite peculiar and difficult to accommodate.

Thunar clearly checks file permissions based on the local user. But the remote user is different, so it gets wrong conclusions. But I cannot see a way to fix this except give thunar detailed knowledge of the specific remote file system and the actual remote user.

Anyway your patch as is cannot be accepted since it is actually wrong for the UFS semantics. It will work most of the time but fail in important situations (files with 0444 permissions).

Looks like for some specific sshfs semantics thunar_file_is_writable(file) returns true and lets you go ahead. But it looks like a coincidence. I need to find some information on why that checks gives a different result.

The only acceptable option would be to add an || thunar_file_is_writable(file) right before the "&& !thunar_file_is_trashed (file));" in the last return in the upstream code. But I still have to evaluate the consequences.

I need to study sshfs a little.

Your suggeestion about adding an ||
Comment 9 Guido Falsi freebsd_committer 2017-11-28 21:11:27 UTC
(In reply to Guido Falsi from comment #8)
> (In reply to rozhuk.im from comment #4)
> 
> > My check is simple: if file writeable than we can write metadata.
> > This is good for most cases.
> > I dont know how to allow user write data and restrict write metadata in unix
> > with chmod().
> 
> Looking at the upstream code and checking how UFS works I can now say this
> is definitely a wrong assumption.
> 
> you can chmod any file you own, even if it is not writable, otherwise there
> would be no way to modify permissions on a file with 0444 permissions.
> obviously root can change permissions on any file.
> 
> The upstream code, while a little convoluted, checks just for this, which is
> correct for local file systems.
> 
> > 
> > In my case sshfs mounted to me (simple user), by authorized on remote side
> > as root.
> 
> Considering the previous correction this use case is quite peculiar and
> difficult to accommodate.
> 
> Thunar clearly checks file permissions based on the local user. But the
> remote user is different, so it gets wrong conclusions. But I cannot see a
> way to fix this except give thunar detailed knowledge of the specific remote
> file system and the actual remote user.
> 
> Anyway your patch as is cannot be accepted since it is actually wrong for
> the UFS semantics. It will work most of the time but fail in important
> situations (files with 0444 permissions).
> 
> Looks like for some specific sshfs semantics thunar_file_is_writable(file)
> returns true and lets you go ahead. But it looks like a coincidence. I need
> to find some information on why that checks gives a different result.
> 
> The only acceptable option would be to add an ||
> thunar_file_is_writable(file) right before the "&& !thunar_file_is_trashed
> (file));" in the last return in the upstream code. But I still have to
> evaluate the consequences.

This is wrong too.

thunar_file_is_writable(file) could return true for files you don't own (think write bit on a group you are part of), so on which you can't change attributes. Checking for write permissions to change attributes is unrelated.

I don't know a proper way to cater for a remote file system were you access as a different user...especially if that user is root and you are not root locally.

If you find a solution please file a new bug report.
Comment 10 rozhuk.im 2017-11-29 09:40:15 UTC
False positive - not problem, at least for me.
I dont care even if it always return true without any checks.

On access deny user will get error on apply, it much better than do not try chmod() because of false-negative.
Comment 11 rozhuk.im 2017-11-29 10:31:23 UTC
Unable to change file permissions on sshfs mounted files/dirs - cant be "work as intended".
Comment 12 Guido Falsi freebsd_committer 2017-11-29 11:28:01 UTC
(In reply to rozhuk.im from comment #10)
> False positive - not problem, at least for me.
> I dont care even if it always return true without any checks.
> 
> On access deny user will get error on apply, it much better than do not try
> chmod() because of false-negative.

I agree that having that check return true would work for you, with your simplistic definition of "works".

First of all consider that the ports tree is not yours, and all use cases must be kept in consideration.

You also must consider that the ports tree contains that, ported software. Original development must be done upstream. If the upstream does a stupid thing the port will also do a stupid thing. The only logical exception is when the stupid thing is caused by a specific FreeBSD difference from other OSes. In that case too at least trying to get a correct patch accepted upstream is a requirement.

The upstream code shows that the xfce guys actually don't want to hit system call errors and that's the very reason why they apply such a check.

Your patch is anyway definitely wrong and would give false positives and false negatives on UFS and ZFS, since write permissions are completely unrelated to chmod usage. Returning true would be a clear breakage of upstream code.

This patch is not acceptable and it looks like your use case is not catered by upstream. So the software is "working as intended", you happen to disagree with the intended way the upstream software should work.

regarding comment 11, since you disagree on the "works as intended" resolution I'll change it to a more arbitrary "rejected", so you will have one excuse less to open this up again.

I encourage you to push a correct solution with the upstream thunar developers.
Comment 13 Guido Falsi freebsd_committer 2017-11-29 11:38:00 UTC
(In reply to Guido Falsi from comment #12)
> I encourage you to push a correct solution with the upstream thunar
> developers.

BTW I can review proposed patches but they have to be correct for the common case, which is local file systems, usually UFS and/or ZFS. The thunar guys will care a lot for ext[2-4], btrfs and other common linux file systems.

Forcibly allowing a corner case in sshfs by breaking the software for all others by deliberately removing checks the upstream put in the code is definitely not acceptable.

(hitting errors in chmod(2) is clearly accounted as breakage by upstream, and I tend to agree with that interpretation)
Comment 14 Guido Falsi freebsd_committer 2017-11-29 12:36:20 UTC
I'd like to suggest a solution idea:

If you can find a way, from inside the thunar_file_get_deletion_date() function to discover with good confidence that you are in a sshfs filesystem and just in that special case return an unconditional true or another check that fits your use case, that could be an acceptable strategy.
Comment 15 Guido Falsi freebsd_committer 2017-12-04 19:55:49 UTC
Created attachment 188531 [details]
patch using statfs(2)

Hi again.

I thought of a possible solution to this issue.

I prepared a patch to the thunar port, adding an option which adds an extra patch.

This patches the thunar code to check if the file is on sshfs through the statfs(2) system call.

I have only tested building the patch. I will not be able to test this on a running system before the week end.

Anyway you could give it a spin and see if it fixes your problem.

Thanks.
Comment 16 rozhuk.im 2017-12-06 21:06:48 UTC
1. Original code is ureadable die to strange logic.
It can be rewriten to:

gboolean
thunar_file_is_chmodable (const ThunarFile *file)
{
  _thunar_return_val_if_fail (THUNAR_IS_FILE (file), FALSE);

  /* we can only change the mode if we the euid is
   *   a) equal to the file owner id
   * or
   *   b) the super-user id
   * and the file is not in the trash.
   */
  if (thunar_file_is_trashed (file))
    return (FALSE);
  if (effective_user_id == 0)
    return (TRUE);
  if (file->info != NULL &&
      effective_user_id == g_file_info_get_attribute_uint32 (file->info,
                                                             G_FILE_ATTRIBUTE_UNIX_UID))
    return (TRUE);
  return (FALSE);
}


2. Only one check really needed: thunar_file_is_trashed (file), all other is waste of time.
I dont understand what problem return always true (except thunar_file_is_trashed()), and see error (or nothing?) later?


3. If you want to play around chmod() failsafe then there is more things from statfs can be added, see
https://bugs.freebsd.org/bugzilla/attachment.cgi?id=188261&action=diff
is_fs_local()

+

/* Exclude from file changes monitoring, watch only for dirs. */
static const char *non_local_fs[] = {
	"fusefs.sshfs",
	NULL
};

There is possible also curl and some others fusefs plugins.
Comment 17 Guido Falsi freebsd_committer 2017-12-06 21:51:50 UTC
(In reply to rozhuk.im from comment #16)
> 1. Original code is ureadable die to strange logic.
> It can be rewriten to:
> 
> gboolean
> thunar_file_is_chmodable (const ThunarFile *file)
> {
>   _thunar_return_val_if_fail (THUNAR_IS_FILE (file), FALSE);
> 
>   /* we can only change the mode if we the euid is
>    *   a) equal to the file owner id
>    * or
>    *   b) the super-user id
>    * and the file is not in the trash.
>    */
>   if (thunar_file_is_trashed (file))
>     return (FALSE);
>   if (effective_user_id == 0)
>     return (TRUE);
>   if (file->info != NULL &&
>       effective_user_id == g_file_info_get_attribute_uint32 (file->info,
>                                                             
> G_FILE_ATTRIBUTE_UNIX_UID))
>     return (TRUE);
>   return (FALSE);
> }

I agree the original code is ugly, but, as I have already explained, this is just a port, so, as long the original code works, I'm not willing to modify it.

The upstream code works for the common case of the local file systems. My proposed patch avoids that code for the sshfs special case in exactly the way you suggest.

> 
> 
> 2. Only one check really needed: thunar_file_is_trashed (file), all other is
> waste of time.
> I dont understand what problem return always true (except
> thunar_file_is_trashed()), and see error (or nothing?) later?
> 

The problem is that thunar is clearly programmed by the author to not behave in such a way, and does not behave in such a way on other OSes. There is no reason why a port (not original development) should behave differently.

Thunar is using this check to choose if the file permissions interface should be enabled. Thunar users expect it to be disabled when they can't change file permissions. It would at least break POLA.

I understand you'd like thunar to act in such a way but why should you impose this preference of yours on all FreeBSD ports users?

Finally since this is just a porting, there's no reason to intentionally make the port software act differently, in porting we just adapt the software and keep it's strategic choices.

You noticed a problem which I agree is a problem and tried to suggest a solution which keeps the standard behaviour and skips it in the way you ask for others. isn't that an acceptable compromise?

> 
> 3. If you want to play around chmod() failsafe then there is more things
> from statfs can be added, see
> https://bugs.freebsd.org/bugzilla/attachment.cgi?id=188261&action=diff
> is_fs_local()
> 
> +
> 
> /* Exclude from file changes monitoring, watch only for dirs. */
> static const char *non_local_fs[] = {
> 	"fusefs.sshfs",
> 	NULL
> };
> 
> There is possible also curl and some others fusefs plugins.

Good point.

I'll rework my hack so that it uses a list that can be updated if needed.
Comment 18 Guido Falsi freebsd_committer 2017-12-06 21:55:38 UTC
(In reply to rozhuk.im from comment #16)
> 1. Original code is ureadable die to strange logic.
> It can be rewriten to:
[...]
> 
> There is possible also curl and some others fusefs plugins.

BTW you did not tell me if you tested my patch and it did allow you to work on sshfs.

I'd like that information, which would confirm or not that the general approach is viable.
Comment 19 rozhuk.im 2017-12-08 23:03:23 UTC
(In reply to Guido Falsi from comment #17)

1. I dont like patch bad/ugly code.

2. I do not insist than my patch must be committed to ports.
I insist that my patch is best for me. :)
I understand and accept that if some one with commit bit here or in upstream not commit my patch then I have no other options except continue use my patch as private patch. No problem.

No, I do not test your patch.
I think I will update mine to:
gboolean
thunar_file_is_chmodable (const ThunarFile *file)
{
  _thunar_return_val_if_fail (THUNAR_IS_FILE (file), FALSE);

  if (thunar_file_is_trashed (file))
    return (FALSE);
  return (TRUE);
}
and continue use it as private patch.
Comment 20 Guido Falsi freebsd_committer 2017-12-14 09:35:57 UTC
Being unable to properly test patches for this specific issue alone and the original submitter not willing to test further patches I'm sending this bug report back to the heap.