Do not attempt to find an executable if a script is referenced #216

adinapoli · 2022-03-28T14:12:10Z

@andreasabel This is a (potentially naive) attempt at fixing #189 (and perhaps #107?).

This was the result of a quick hacking session so I am sure the story is more complicated than this, and perhaps we should also have @gregwebs chime in on this one.

~~I am not sure why the switch from system-filepath to filepath triggered it~~, but the crux of the issue is that unless shelly is run with escaping False, the input FilePath fed into run & co will be "escaped", which in this context means calling the runCommand function, which will attempt to find an executable at PATH.

This works most of the time, but if we have something like ./foo/bar/baz.sh, this won't work, and Shelly will complain.

The easiest fix I could think of was to simply check if we had a FilePath commencing with . and, in this case, we don't attempt to find an executable, but we run the command as-if we used escaping False.

I have also added a regression test, to catch this in the future.

Thoughts?

EDIT: For visibility, here is the postmortem copied from an inline comment:

I have finally nailed down exactly why the regression happened. We have this code, which is responsible for locating the file, in the guts of whichEith:

    whichFull fp = do
      (trace . mappend "which " . toTextIgnore) fp >> whichUntraced
      where
        whichUntraced | isAbsolute fp          = checkFile
                      | dotSlash splitOnDirs   = checkFile
                      | length splitOnDirs > 0 = lookupPath  >>= leftPathError
                      | otherwise              = lookupCache >>= leftPathError

        splitOnDirs = splitDirectories fp
        dotSlash ("./":_) = True
        dotSlash _ = False

As you can see, we are already accounting for dotSlash, so why things fail? Well, it turns out that both system-filepath and filepath have a splitDirectory function, but hey behave differently:

> import qualified Filesystem.Path.CurrentOS as SFP
> import qualified Data.Text as T
> import System.FilePath as FP
> FP.splitDirectories "./test/data/hello.sh"
[".","test","data","hello.sh"]
> SFP.splitDirectories (SFP.fromText $ T.pack "./test/data/hello.sh")
[FilePath "./",FilePath "test/",FilePath "data/",FilePath "hello.sh"]
>

As you can see the former doesn't include the "./", so the pattern match on dotSlash fails, and here is our regression.

andreasabel · 2022-03-28T15:02:14Z

CI reports some broken tests under Linux:

Failures:

  test/src/FindSpec.hs:114:22: 
  1) find follow symlinks
       expected: ["dir", "dir/symlinked_dir", "dir/symlinked_dir/hoge_file", "hello.sh", "nonascii.txt", "symlinked_dir", "symlinked_dir/hoge_file", "zshrc"]
        but got: ["dir", "dir/symlinked_dir", "dir/symlinked_dir/hoge_file", "nonascii.txt", "symlinked_dir", "symlinked_dir/hoge_file", "zshrc"]

  To rerun use: --match "/find/follow symlinks/"

  test/src/FindSpec.hs:129:22: 
  2) find not follow symlinks
       expected: ["dir", "dir/symlinked_dir", "hello.sh", "nonascii.txt", "symlinked_dir", "symlinked_dir/hoge_file", "zshrc"]
        but got: ["dir", "dir/symlinked_dir", "nonascii.txt", "symlinked_dir", "symlinked_dir/hoge_file", "zshrc"]

  To rerun use: --match "/find/not follow symlinks/"

  test/src/RunSpec.hs:31:5: 
  3) run script at $PWD
       uncaught exception: ReThrownException
       
       Ran commands: 
       ./test/data/hello.sh
       
       Exception: ./test/data/hello.sh: createProcess: exec: invalid argument (Bad file descriptor)

  To rerun use: --match "/run/script at $PWD/"

Could you please have a look what causes these and how to update your PR accordingly, @adinapoli ?

adinapoli · 2022-03-28T17:45:49Z

In a lucky turn of events I will be working remotely from Rome and my digital nomad workstation is a Linux machine (as opposed to my main Mac mini), so I can definitely look into this one later this week.

Thanks for the heads up!

gregwebs · 2022-03-28T17:45:51Z

Thanks for adding a test case. With enough testing we can get make a change.

adinapoli · 2022-04-01T06:30:13Z

@andreasabel @gregwebs Ok, I think we should be in business now. Two failures were due to the fact I had forget to add hello.sh to the data files ( 🤦 ) whereas the run failure was due to the fact CI didn't have the +x permission set for hello.sh to execute it.

As far as windows testing goes, obviously running that bash script doesn't make sense, so I have guarded the test using isWindows. I am not sure what the best course of action is here.

src/Shelly.hs

andreasabel · 2022-04-01T08:35:36Z

src/Shelly.hs

-          -- it would be better to specifically detect that bug
+      = case fp of
+          -- If the 'FilePath' contains a more articulated path than just
+          -- an executable, don't try to find it.


Should this rather check whether there is a directory component in exe? What about bla/hello.sh instead of ./hello.sh? Currently you would accept e.g. .bla/hello.sh or .hello.sh.
I think checking for starting with dot isn't the correct criterion.

Yes, I take your point this is too simplistic. I will have to think about this a little bit.

@andreasabel Aha! After a more thorough look, I have finally nailed down exactly why the regression happened. We have this code, which is responsible for locating the file, in the guts of whichEith:

whichFull fp = do (trace . mappend "which " . toTextIgnore) fp >> whichUntraced where whichUntraced | isAbsolute fp = checkFile | dotSlash splitOnDirs = checkFile | length splitOnDirs > 0 = lookupPath >>= leftPathError | otherwise = lookupCache >>= leftPathError splitOnDirs = splitDirectories fp dotSlash ("./":_) = True dotSlash _ = False

As you can see, we are already accounting for dotSlash, so why things fail? Well, it turns out that both system-filepath and filepath have a splitDirectory function, but hey behave differently:

> import qualified Filesystem.Path.CurrentOS as SFP > import qualified Data.Text as T > import System.FilePath as FP > FP.splitDirectories "./test/data/hello.sh" [".","test","data","hello.sh"] > SFP.splitDirectories (SFP.fromText $ T.pack "./test/data/hello.sh") [FilePath "./",FilePath "test/",FilePath "data/",FilePath "hello.sh"] >

As you can see the former doesn't include the "./", so the pattern match on dotSlash fails, and here is our regression.

Excellent analysis! Thanks, @adinapoli!

test/data/hello.sh

andreasabel · 2022-04-03T22:08:29Z

src/Shelly.hs

-                      | otherwise              = lookupCache >>= leftPathError
+        whichUntraced | isAbsolute fp             = checkFile
+                      | startsWithDot splitOnDirs = checkFile
+                      | length splitOnDirs > 0    = lookupPath  >>= leftPathError


I don't understand when the guard length splitOnDirs > 0 can fail (be False)... It seems this happens only when fp is []. But this should not happen, should it? So is the last case dead?
Here are some experiments:

ghci> :t splitDirectories splitDirectories :: FilePath -> [FilePath] ghci> splitDirectories "." ["."] ghci> splitDirectories "" [] ghci> splitDirectories "/" ["/"] ghci> splitDirectories "//" ["//"] ghci> splitDirectories "///" ["///"] ghci> splitDirectories "/a" ["/","a"] ghci> splitDirectories "/a/" ["/","a"] ghci> splitDirectories "/a/b" ["/","a","b"] ghci> splitDirectories "c:/a" ["c:","a"] ghci> splitDirectories "c:a" ["c:a"] ghci> splitDirectories "c:foo.bat" ["c:foo.bat"] ghci> splitDirectories "c:\\foo.bat" ["c:\\foo.bat"] ghci> splitDirectories "c:\\\\foo.bat" ["c:\\\\foo.bat"] ghci> splitDirectories "c:/foo.bat" ["c:","foo.bat"]

Which output of splitDirectories am I looking at? the one from system-filepath or filepath? 😅 Hard to tell why that guard was added, as it's alas not documented -- possibly some weird corner case?

Oh sorry, this is the one of filepath which is used in latest shelly. But the system-filepath one is not much different.
@gregwebs : Can you explain the mystery of the length splitOnDirs > 0 guard? You introduced it in d441583#r70391227

Greg agrees that the guard length splitOnDirs > 0 is unnecessary (and the clause below can be dropped).

Excellent! I will amend this PR with this and the other changes between today and tomorrow. Thanks!

andreasabel · 2022-04-03T22:10:02Z

src/Shelly.hs

+        -- function, but it returns \"./\" as its first argument,
+        -- so we pattern match on both for backward-compatibility.
+        startsWithDot (".":_)  = True
+        startsWithDot ("./":_) = True


Does it actually make sense to keep the old test for backwards-compatibility? I mean, System.FilePath.splitDirectorieswill never return list that contains "./".

It does look harmless -- the only reason why I have kept it was mostly for documenting purposes and to defend ourselves from draconian cases where people would attempt frankestein builds that uses newer Shelly-s but system-filepath (assuming that is possible). I suppose it gives us a bit more flexibility at a very little cost, but I am also happy to get rid of the redundant test 😉

Don't get me wrong: I am very much for documenting the why and also the history.
However, an impossible case shouldn't be there, because it will create insecurity in the reader of the code. It will actually suggest that the case is possible and should be handled. If you want to keep it in, it should be

startsWithDot ("./":_) = undefined -- this case is impossible

but I think this isn't necessary.
I'd keep references to previous behavior in the comment.

test/data/hello.sh

adinapoli · 2022-04-04T06:48:02Z

Ok, we do not have to improve on Shelly in this PR, just restore its old behavior...

@andreasabel hehe yes, my temptation would be to eschew any temptation to improve Shelly as part of this PR and instead deliver the smallest fix that restores the old behaviour, and perhaps we flag all these low hanging fruits in a ticket, as those sounds very inviting for newcomers willing to contribute to the project 😉

adinapoli · 2022-04-06T06:44:58Z

@andreasabel Ok, I have addressed all the review remarks. Reshuffling that whichFull function also had the added bonus of removing some now-redundant functions.

Something I should point out though is that, at least on my Linux machine, splitDirectories can return a length of 0, which is when the empty string is passed:

> import System.FilePath as FP
> FP.splitDirectories ""
[]
>

This is very weird, because you also ran the same test here but got something different. Having said that, the tests seem to still all pass, so perhaps that's nothing to worry about.

Furthermore, I couldn't conceivably see why somebody would try to run a command by passing the empty string as the executable (and I agree with your analysis, garbage in, garbage out). A case could be made that Shelly could get rid of these unrepresentable states if it had its own (internal) definition of something like newtype FilePath = FilePath (NonEmpty Char) but again, this might be overkill (or too simplistic) and definitely doesn't belong to this PR 😉

adinapoli · 2022-05-24T07:31:10Z

@andreasabel I have lost track of this -- are you folks expecting anything else from me or is this essentially good to go? Thanks! 😉

This commit simplifies the local `whichFull` function (local to `whichEith`) by dropping a redundant guard as well as /always/ running `lookupPath >>= leftPathError`. Previously we had this: ``` whichFull fp = do (trace . mappend "which " . toTextIgnore) fp >> whichUntraced where whichUntraced | isAbsolute fp = checkFile | dotSlash splitOnDirs = checkFile | length splitOnDirs > 0 = lookupPath >>= leftPathError | otherwise = lookupCache >>= leftPathError splitOnDirs = splitDirectories fp ``` However `splitOnDirs` can never return the empty list, so that guard was redundant as that code path was always executed, and the /otherwise/ case never executed.

andreasabel

Thanks again @adinapoli and apologies for the long break.
I am planning to release this as 1.11.0 now, assuming you haven't experienced any problems with this code in the last 9 month.
I rebased this on latest master but otherwise left the commits untouched.

adinapoli · 2023-01-25T07:02:23Z

Thanks again @adinapoli and apologies for the long break. I am planning to release this as 1.11.0 now, assuming you haven't experienced any problems with this code in the last 9 month. I rebased this on latest master but otherwise left the commits untouched.

No worries, and thanks for picking this one up 😉 In interest of full transparency, I ended up not using (just yet) this branch/commit, as I try to avoid depending on unreleased/unapproved forks/branches/patches for production code, so I just ended up downgrading to a suitable Shelly, but given how much we spent thinking about this issue, I think it should be fine. We know precisely where and why the regression happened and we have a test to prevent that from happening again, so we should be good 😉

andreasabel · 2023-01-25T09:02:28Z

Thanks again, @adinapoli !
I pushed the button now, so you have opportunities to evaluate the fix in production now: https://hackage.haskell.org/package/shelly-1.11.0

adinapoli · 2023-01-25T09:03:26Z

Thanks again, @adinapoli ! I pushed the button now, so you have opportunities to evaluate the fix in production now: https://hackage.haskell.org/package/shelly-1.11.0

Beautiful, I'll keep you posted, thanks!

adinapoli force-pushed the adinapoli/filepath-regression branch 3 times, most recently from c1dae8b to 51b2fa1 Compare April 1, 2022 06:14

andreasabel requested changes Apr 1, 2022

View reviewed changes

adinapoli force-pushed the adinapoli/filepath-regression branch from 51b2fa1 to 51aff5e Compare April 1, 2022 10:18

andreasabel reviewed Apr 3, 2022

View reviewed changes

adinapoli requested a review from andreasabel April 6, 2022 06:45

gregwebs approved these changes Apr 6, 2022

View reviewed changes

adinapoli and others added 3 commits January 24, 2023 20:34

Add regression test

d322c12

Tentative fix for FilePath regression

650e2a7

andreasabel force-pushed the adinapoli/filepath-regression branch from 238b5f4 to c68825e Compare January 24, 2023 19:52

andreasabel approved these changes Jan 24, 2023

View reviewed changes

andreasabel linked an issue Jan 24, 2023 that may be closed by this pull request

Shelly can't run command with a relative path anymore #107

Closed

andreasabel added this to the 1.11.0 milestone Jan 24, 2023

andreasabel merged commit c68825e into gregwebs:master Jan 25, 2023

andreasabel self-assigned this Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not attempt to find an executable if a script is referenced #216

Do not attempt to find an executable if a script is referenced #216

adinapoli commented Mar 28, 2022 •

edited

Loading

andreasabel commented Mar 28, 2022

adinapoli commented Mar 28, 2022 •

edited

Loading

gregwebs commented Mar 28, 2022

adinapoli commented Apr 1, 2022

andreasabel Apr 1, 2022

adinapoli Apr 1, 2022

adinapoli Apr 1, 2022

andreasabel Apr 3, 2022

andreasabel Apr 3, 2022

adinapoli Apr 4, 2022 •

edited

Loading

andreasabel Apr 4, 2022

andreasabel Apr 5, 2022

adinapoli Apr 6, 2022

andreasabel Apr 3, 2022

adinapoli Apr 4, 2022

andreasabel Apr 4, 2022

adinapoli commented Apr 4, 2022

adinapoli commented Apr 6, 2022

adinapoli commented May 24, 2022

andreasabel left a comment

adinapoli commented Jan 25, 2023

andreasabel commented Jan 25, 2023

adinapoli commented Jan 25, 2023

Do not attempt to find an executable if a script is referenced #216

Do not attempt to find an executable if a script is referenced #216

Conversation

adinapoli commented Mar 28, 2022 • edited Loading

andreasabel commented Mar 28, 2022

adinapoli commented Mar 28, 2022 • edited Loading

gregwebs commented Mar 28, 2022

adinapoli commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adinapoli Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adinapoli commented Apr 4, 2022

adinapoli commented Apr 6, 2022

adinapoli commented May 24, 2022

andreasabel left a comment

Choose a reason for hiding this comment

adinapoli commented Jan 25, 2023

andreasabel commented Jan 25, 2023

adinapoli commented Jan 25, 2023

adinapoli commented Mar 28, 2022 •

edited

Loading

adinapoli commented Mar 28, 2022 •

edited

Loading

adinapoli Apr 4, 2022 •

edited

Loading