[WIP] Fix --exec

[jakwings]

Problem solved but the tests are in a tangle.

ping #139 #141

[jakwings]

After decoupling the tests about shell_escape from the tests of src/exec/input.rs, it is still not easy to write thorough tests for --exec, unless we fix/solidate the behavior of shell_escape::escape by specifying a full version string.

ping #134

[sharkdp]

This looks very good to me!

I'd like to hear @mmstick's feedback though.

@iology Do you have any idea what's causing the failure on i686-unknown-linux-musl?

[jakwings]

Testing on Windows is a little hard. Always need more tests. Windows is not my daily OS, I just do anything to keep the features complete.

On Windows, A single drive name without slashes (e.g. C:, not C:\) behaves differently on different drives. So fd.exe still has bugs. ;-) Direct manipulation on path string, instead of using std::path::Path::components, is not DRY and makes this problem unnoticed further.

I don't use musl.

[mmstick]

Looks fine to me. The tests pass with musl on my end with your PR, so I'm not sure why Travis errored there.

[sharkdp]

Interestingly, the tests with musl pass on x86_64, but not i686.

[sharkdp]

I am able to reproduce this locally. Here is what is happening:

GNU gdb (GDB) 8.0.1
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /home/shark/fd-jakwings/target/i686-unknown-linux-musl/debug/fd...done.
warning: Missing auto-load script at offset 0 in section .debug_gdb_scripts
of file /home/shark/fd-jakwings/target/i686-unknown-linux-musl/debug/fd.
Use `info auto-load python-scripts [REGEXP]' to list them.
(gdb) run foo --exec 'echo {}'
Starting program: /home/shark/fd-jakwings/target/i686-unknown-linux-musl/debug/fd foo --exec 'echo {}'
[New LWP 21012]
[New LWP 21013]

Thread 2 "fd" received signal SIGILL, Illegal instruction.
[Switching to LWP 21012]
alloc::arc::{{impl}}::clone<fd::exec::TokenizedCommand> (self=0xf79ff510)
    at /checkout/src/liballoc/arc.rs:573
573	/checkout/src/liballoc/arc.rs: No such file or directory.
(gdb) backtrace 
#0  alloc::arc::{{impl}}::clone<fd::exec::TokenizedCommand> (self=0xf79ff510)
    at /checkout/src/liballoc/arc.rs:573
#1  0x565de108 in fd::walk::scan::{{closure}} () at src/walk.rs:72
#2  0x5657205b in std::sys_common::backtrace::__rust_begin_short_backtrace<closure,()> (f=...)
    at /checkout/src/libstd/sys_common/backtrace.rs:136
#3  0x5657ab8a in std::thread::{{impl}}::spawn::{{closure}}::{{closure}}<closure,()> ()
    at /checkout/src/libstd/thread/mod.rs:364
#4  0x565d18eb in std::panic::{{impl}}::call_once<(),closure> (self=..., _args=())
    at /checkout/src/libstd/panic.rs:296
#5  0x5657b49a in std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> (
    data=0xf79ffa50 "@\362\247\367\000") at /checkout/src/libstd/panicking.rs:454
#6  0x56afc973 in panic_unwind::__rust_maybe_catch_panic () at /checkout/src/libpanic_unwind/lib.rs:98
#7  0x5657b14b in std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> (f=...)
    at /checkout/src/libstd/panicking.rs:433
#8  0x565781d8 in std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> (f=...)
    at /checkout/src/libstd/panic.rs:361
#9  0x5657a640 in std::thread::{{impl}}::spawn::{{closure}}<closure,()> ()
    at /checkout/src/libstd/thread/mod.rs:363
#10 0x565acbf0 in alloc::boxed::{{impl}}::call_box<(),closure> (self=0xf7a14340, args=())
    at /checkout/src/liballoc/boxed.rs:648
#11 0x56af3e4b in alloc::boxed::{{impl}}::call_once<(),()> () at /checkout/src/liballoc/boxed.rs:658
#12 std::sys_common::thread::start_thread () at /checkout/src/libstd/sys_common/thread.rs:21
#13 std::sys::imp::thread::{{impl}}::new::thread_start () at /checkout/src/libstd/sys/unix/thread.rs:84
#14 0x56b2e834 in start ()

@mmstick Could this be related to the unsafe block?

fd/src/walk.rs

Line 74 in e9cf8af

let cmd = unsafe { Arc::from_raw(cmd as *const TokenizedCommand) };

[sharkdp]

The actual SIGILL seems to come from the abort() call here:
https://github.com/rust-lang/rust/blob/0ade339411587887bf01bcfa2e9ae4414c8900d4/src/liballoc/arc.rs#L562-L575

        // However we need to guard against massive refcounts in case someone
        // is `mem::forget`ing Arcs. If we don't do this the count can overflow
        // and users will use-after free. We racily saturate to `isize::MAX` on
        // the assumption that there aren't ~2 billion threads incrementing
        // the reference count at once. This branch will never be taken in
        // any realistic program.
        //
        // We abort because such a program is incredibly degenerate, and we
        // don't care to support it.
        if old_size > MAX_REFCOUNT {
            unsafe {
                abort();
            }
        }

[sharkdp]

So it seems like this is indeed somehow related to that unsafe block. The error can be fixed by replacing:

let cmd = unsafe { Arc::from_raw(cmd as *const TokenizedCommand) };

with

let cmd = Arc::new(cmd.clone());

[sharkdp]

@iology Could you please try to rebase this on top of master? This should hopefully fix the tests.

I'd like to include this before releasing fd v5.0.

[jakwings]

Sorry, I'm going to close this if nobody else wants to take good care of the Windows build. In my unpublished fork the Windows support has been completely removed. (a very backward-incompatible fork ;-)

This patch is not completed for the Windows build even after rebasing. So I can't say it fixes everything. Anyway the code is licensed for merging.

[sharkdp]

Sorry, I'm going to close this if nobody else wants to take good care of the Windows build.

I would really like to integrate the tests you wrote (for Linux). Does this PR make things worse on Windows? Otherwise, someone else can still continue working on the Windows part later.

In my unpublished fork the Windows support has been completely removed. (a very backward-incompatible fork ;-)

I'm also not a Windows-developer and I think it's definitely not our main target platform, but there has definitely been interest in a Windows version. However, I think it would be perfectly fine to support only a limited subset of features on Windows (the others might still be supported at a later point in time).

This patch is not completed for the Windows build even after rebasing. So I can't say it fixes everything. Anyway the code is licensed for merging.

If it's not too much trouble, it would be great if you could still rebase it on master. I currently cannot merge or rebase (unless I would manually cherry-pick your commits on the command line or something similar).

Once you choose to publish your private fork, let me know! It would be a shame to lose you as a contributor. Anyways, thank you very much for your great support in the last few weeks!

[jakwings]

Ok, I'll rebase and add some FIXME notes to the code.

Once you choose to publish your private fork, let me know!

No problem. I've customized it for my taste. --exec will not depend on the system shells and resemble --exec of find. ./ is not omitted for relative paths. <pattern> comes after the <root directory>. --type executable is supported for regular files. And changes to flag names, error handling and other trivial things. (Also this is my way to learn Rust without bothering you too much. :P

[jakwings]

Rebased, please check.

[sharkdp]

Thank you very much! fd v5.0 coming in 3...2...1...

[jakwings]

@sharkdp FYI: https://github.com/jakwings/ff-find (still a lot of todos)

[sharkdp]

@iology Very cool! I will take a closer look soon. Maybe we can 'backport' a few ideas to fd 😄