Rex icon indicating copy to clipboard operation
Rex copied to clipboard

Rex hangs when trying to modify file without proper permission (macos)

Open sdondley opened this issue 5 years ago • 7 comments
trafficstars

On macos, this task

task edit_file => sub {
  file "/Users/me/rex/test2/file",
    content => 'change me';
};

will hang if the local user does not have permission to edit the file.

sdondley avatar Apr 16 '20 03:04 sdondley

debug output:

[2020-04-15 23:50:04] DEBUG - This is Rex version: 1.8.2
[2020-04-15 23:50:04] DEBUG - Command Line Parameters
[2020-04-15 23:50:04] DEBUG - 	d = 1
[2020-04-15 23:50:04] DEBUG - Creating lock-file (Rexfile.lock)
[2020-04-15 23:50:04] DEBUG - Loading Rexfile
[2020-04-15 23:50:04] DEBUG - Enabling task_chaining_cmdline_args feature
[2020-04-15 23:50:04] DEBUG - Activating new template engine.
[2020-04-15 23:50:04] DEBUG - Disabling usage of a tty
[2020-04-15 23:50:04] DEBUG - Activating autodie.
[2020-04-15 23:50:04] DEBUG - Using Net::OpenSSH if present.
[2020-04-15 23:50:04] DEBUG - Add service check.
[2020-04-15 23:50:04] DEBUG - Setting set() to not append data.
[2020-04-15 23:50:04] DEBUG - Registering CMDB as template variables.
[2020-04-15 23:50:04] DEBUG - activating featureset >= 0.51
[2020-04-15 23:50:04] DEBUG - activating featureset >= 0.40
[2020-04-15 23:50:04] DEBUG - activating featureset >= 0.35
[2020-04-15 23:50:04] DEBUG - activating featureset >= 0.31
[2020-04-15 23:50:04] DEBUG - Creating new distribution class of type: Base
[2020-04-15 23:50:04] DEBUG - new distribution class of type Rex::TaskList::Base created.
[2020-04-15 23:50:04] DEBUG - Returning existing distribution class of type: Rex::TaskList::Base
[2020-04-15 23:50:04] DEBUG - Creating task: Blah:Boo:example
[2020-04-15 23:50:04] DEBUG - Found Net::OpenSSH and Net::SFTP::Foreign - using it as default
[2020-04-15 23:50:04] DEBUG - Registering task: Blah:Boo:example
[2020-04-15 23:50:04] DEBUG - Returning existing distribution class of type: Rex::TaskList::Base
[2020-04-15 23:50:04] DEBUG - Creating task: gooby_goo
[2020-04-15 23:50:04] DEBUG - Registering task: gooby_goo
[2020-04-15 23:50:04] DEBUG - Returning existing distribution class of type: Rex::TaskList::Base
[2020-04-15 23:50:04] DEBUG - Creating task: edit_file
[2020-04-15 23:50:04] DEBUG - Registering task: edit_file
[2020-04-15 23:50:04] DEBUG - Initializing Logger from parameters found in Rexfile
[2020-04-15 23:50:04] DEBUG - Returning existing distribution class of type: Rex::TaskList::Base
[2020-04-15 23:50:04] DEBUG - Returning existing distribution class of type: Rex::TaskList::Base
[2020-04-15 23:50:04] INFO - Running task edit_file on <local>
[2020-04-15 23:50:04] DEBUG - Rex::Group::Entry::Server (private_key): returning
[2020-04-15 23:50:04] DEBUG - Rex::Group::Entry::Server (public_key): returning
[2020-04-15 23:50:04] DEBUG - $VAR1 = '';

[2020-04-15 23:50:04] DEBUG - Auth-Information inside Task:
[2020-04-15 23:50:04] DEBUG - password => [[%s]]
[2020-04-15 23:50:04] DEBUG - user => [[user]]
[2020-04-15 23:50:04] DEBUG - public_key => [[]]
[2020-04-15 23:50:04] DEBUG - sudo_password => [[**********]]
[2020-04-15 23:50:04] DEBUG - sudo => [[]]
[2020-04-15 23:50:04] DEBUG - port => [[]]
[2020-04-15 23:50:04] DEBUG - auth_type => [[try]]
[2020-04-15 23:50:04] DEBUG - private_key => [[]]
[2020-04-15 23:50:04] DEBUG - Executing: export PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin:/usr/pkg/bin:/usr/pkg/sbin ; LC_ALL=C which perl
[2020-04-15 23:50:04] DEBUG - /usr/bin/perl

[2020-04-15 23:50:04] DEBUG - Executing edit_file
[2020-04-15 23:50:04] DEBUG - Executing: LC_ALL=C /bin/sh -c '[ -L "/Users/user/rex/test2/file" ]'
[2020-04-15 23:50:04] DEBUG - Calculating checksum (MD5) of /Users/user/rex/test2/file
[2020-04-15 23:50:04] DEBUG - Executing: export PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin:/usr/pkg/bin:/usr/pkg/sbin ; LC_ALL=C perl -MDigest::MD5 -e 'open my $fh, "<", $ARGV[0] or die "Cannot open " . $ARGV[0]; binmode $fh; print Digest::MD5->new->addfile($fh)->hexdigest;' '/Users/user/rex/test2/file'
[2020-04-15 23:50:04] DEBUG - d41d8cd98f00b204e9800998ecf8427e
[2020-04-15 23:50:04] DEBUG - MD5 checksum of /Users/user/rex/test2/file: d41d8cd98f00b204e9800998ecf8427e
[2020-04-15 23:50:04] DEBUG - Opening file: /Users/user/rex/test2/.rex.tmp.file for writing.
[2020-04-15 23:50:04] DEBUG - Opening /Users/user/rex/test2/.rex.tmp.file with mode: >
[2020-04-15 23:50:04] DEBUG - Calculating checksum (MD5) of /Users/user/rex/test2/.rex.tmp.file
[2020-04-15 23:50:04] DEBUG - Executing: export PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin:/usr/pkg/bin:/usr/pkg/sbin ; LC_ALL=C perl -MDigest::MD5 -e 'open my $fh, "<", $ARGV[0] or die "Cannot open " . $ARGV[0]; binmode $fh; print Digest::MD5->new->addfile($fh)->hexdigest;' '/Users/user/rex/test2/.rex.tmp.file'
[2020-04-15 23:50:04] DEBUG - bc838b7c7e59458afd78c94db41203f5
[2020-04-15 23:50:04] DEBUG - MD5 checksum of /Users/user/rex/test2/.rex.tmp.file: bc838b7c7e59458afd78c94db41203f5
[2020-04-15 23:50:04] DEBUG - Need to use the new file. md5 sums are different. <<d41d8cd98f00b204e9800998ecf8427e>> = <<bc838b7c7e59458afd78c94db41203f5>>
[2020-04-15 23:50:04] DEBUG - Executing: LC_ALL=C /bin/mv /Users/user/rex/test2/.rex.tmp.file /Users/user/rex/test2/file
[2020-04-15 23:53:12] DEBUG - Removing lockfile
[2020-04-15 23:53:12] DEBUG - Removing lockfile
[2020-04-15 23:53:12] DEBUG - Executing: LC_ALL=C /bin/sh -c '[ -L "Rexfile.lock" ]'
[2020-04-15 23:53:12] DEBUG - Executing: LC_ALL=C /bin/sh -c '[ -L "Rexfile.lock" ]'

sdondley avatar Apr 16 '20 04:04 sdondley

When I try to run Executing: LC_ALL=C /bin/mv /Users/user/rex/test2/.rex.tmp.file /Users/user/rex/test2/file manually, I get:

override rw-r--r-- root/staff for /Users/user/rex/test2/file? (y/n [n])

Perhaps this where it is getting stuck?

sdondley avatar Apr 16 '20 04:04 sdondley

OK, adding a -f option to the mv command fixed it.

sdondley avatar Apr 16 '20 04:04 sdondley

Thanks for the report, I can reproduce it on Linux too:

rex 'nonwritable_file', sub {
  file '/tmp/a',
    content => gmtime(),
    mode => 444;
};

At this point, I have a few questions already:

  • I wonder if forced overwriting is the solution in all cases?
  • Is -f supported on all platforms and for all connection methods?
  • Only rename operations are affected or other file system operations would have the same behavior as well (e.g. remove)?

Hanging is definitely a bug, but I find both behaviors to be valid use cases:

  • forcing it: "I said I want that file to be managed, do it"
  • being defensive: "I thought I could write that file, let's investigate"

For the rename, I'm thinking about the following fix:

  • write a test that reproduces the scenario and fails when the user does not have write permissions on a file, both for the imperative rename and declarative file commands
  • add a force => {TRUE|FALSE} option to Rex::Interface::Fs::*::rename
  • based on force setting implement forcible overwriting for all platforms (Linux, BSDs, Windows) and for all interfaces (SSH, OpenSSH, Local, etc.) for rename operations
  • fail rename in the non-writable cases when force => FALSE (check for writability and permissions)
  • make the file command use force => TRUE when calling rename
  • tests pass

If other commands are affected, I think a similar pattern can be followed. Travis CI would test for at least Linux, Mac OS and Windows, and a development release would help getting feedback via CPAN Testers (all the BSDs, Solaris, etc.).

ferki avatar Apr 16 '20 04:04 ferki

Yes, my patch is basically a conversation starter.

My big question is: Is there a way to anticipate the problem and error out before the code executes and hangs?

sdondley avatar Apr 16 '20 10:04 sdondley

There is a -w option: https://www.cyberciti.biz/faq/unix-linux-shell-scripting-test-if-filewritable/

sdondley avatar Apr 16 '20 10:04 sdondley

As discussed on IRC, the minimum approach for this would be to:

  • test for file writability in all the Rex::Interface:Fs::* modules before having a chance to hang on an interactive prompt, and raise an exception: the built-in is_writable function might help here
  • fix all potentially affected methods: at least unlink would probably be affected as well, and there might be more, like rmdir

Also let's find a way to test at least a subset of those cases. Perhaps Test::Timer might help to detect hangs? Not sure how to kill the hung call though yet.

To give more control to the users, a force option would make sense to be added for the same methods, which would control addition of the -f flag to e.g. mv. In that case, for example the rename call in the file command should also pass force => TRUE (since file is declarative).

For a full implementation, we might consider a combination of a few factors when trying to predict if e.g. an mv call would hang or not, or whether it makes sense to try forcing the operation or not:

  • is the file writable?
  • is the current executing user also the owner of the file?
  • is the current executing user also a member of the group owner of the file?

The underlying tools seem to decide whether they show a prompt or not based on these factors according to our initial trial-and-error research.

ferki avatar Apr 19 '20 15:04 ferki