Opened 6 years ago

Closed 6 months ago

#2184 closed bug (fixed)

if findExecutable finds a file that matchs the argument, check if it is an executable

Reported by: iago Owned by: leroux
Priority: high Milestone: 7.8.1
Component: libraries/directory Version: 6.8.2
Keywords: findExecutable check executable Cc:
Operating System: Linux Architecture: x86
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

Description

When search finds a file that matchs with the fileName argument of findExecutable, it doesn't check if the file is an executable file.

 do
  path <- getEnv "PATH"
  search (splitSearchPath path)
  where
    fileName = binary <.> exeExtension

    search :: [FilePath] -> IO (Maybe FilePath)
    search [] = return Nothing
    search (d:ds) = do
        let path = d </> fileName
        b <- doesFileExist path
        if b then return (Just path)
             else search ds

findExecutable mustn't returns a file that it isn't an executable file, for example

(~/bin is in PATH variable)

$ touch ~/bin/foofile
$ ghci
$ :m System.Directory
$ findExecutable "foofile"

shows that findExecutable returns IO (Just "/home/iago/bin/foofile").

Altought is not a bug, could be good add a new funcion (findExecutables for example) to return all the executables in all the paths of PATH variable that matchs with a given filename (like which -a).

Attachments (2)

0001-Fix-2184.-findExecutable-checks-if-the-file-actually.patch (1.2 KB) - added by leroux 8 months ago.
leroux's patch
0001-Fixes-2184-findExecutable-is-correctly-implemented-t.patch (3.5 KB) - added by leroux 7 months ago.
Final complete patch - findExecutable checks for the executable bit.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by igloo

  • Difficulty set to Unknown
  • Milestone set to 6.8.3
  • Summary changed from if findExecutable finds a file that matchs the argument don't check if it is an executable to if findExecutable finds a file that matchs the argument, check if it is an executable

Thanks for the report; we should be able to fix this for 6.8.3.

For your proposed findExecutables addition, please follow the library submissions procedure.

comment:2 Changed 6 years ago by igloo

  • Milestone changed from 6.8.3 to 6.10 branch

Hmm, the system function that we use on Windows returns the first path, regardless of whether or not it is executable. I think we should punt on this for now, and revisit it or 6.10.

comment:3 Changed 5 years ago by igloo

  • Milestone changed from 6.10 branch to 6.12 branch

comment:4 Changed 4 years ago by igloo

  • Milestone changed from 6.12 branch to 6.12.3

comment:5 Changed 4 years ago by igloo

  • Milestone changed from 6.12.3 to 6.14.1
  • Priority changed from normal to low

comment:6 Changed 3 years ago by igloo

  • Milestone changed from 7.0.1 to 7.0.2

comment:7 Changed 3 years ago by igloo

  • Milestone changed from 7.0.2 to 7.2.1

comment:8 Changed 3 years ago by igloo

  • Milestone changed from 7.2.1 to 7.4.1

comment:9 Changed 2 years ago by igloo

  • Milestone changed from 7.4.1 to 7.6.1
  • Priority changed from low to lowest

comment:10 Changed 20 months ago by igloo

  • Milestone changed from 7.6.1 to 7.6.2

comment:11 Changed 8 months ago by leroux

  • Owner set to leroux
  • Type of failure set to None/Unknown

comment:12 Changed 8 months ago by leroux

  • Status changed from new to patch

Changed 8 months ago by leroux

leroux's patch

comment:13 Changed 7 months ago by leroux

I've implemented findFiles, findFilesWith, and findExecutables as convenience functions.

findFilesWith checks for existence and applies another boolean functions for filtering purposes (such as checking permission bits).

findFiles and findExecutables are special cases of findFilesWith.

findFile and findExecutable are now simply listToMaybe of findFiles and findExecutables respectively.

I also took thoughtpolice's views on what a correct implementation is (fyi).

attachment:0001-Fixes-2184-findExecutable-is-correctly-implemented-t.patch

This ticket is ready for another review to be merged in.

comment:14 follow-up: Changed 7 months ago by joeyadams

Thanks for implementing this. I have an application that could use findFiles to locate its configuration files.

An issue I noticed: findExecutables only returns the first match on Windows. This could lead to compatibility problems if someone uses findExecutables and filters the resulting list. I don't think findExecutables should be exported. But if someone needs it, it should behave consistently between Windows and other systems.

comment:15 in reply to: ↑ 14 Changed 7 months ago by leroux

Replying to joeyadams:

Thanks for implementing this. I have an application that could use findFiles to locate its configuration files.

An issue I noticed: findExecutables only returns the first match on Windows. This could lead to compatibility problems if someone uses findExecutables and filters the resulting list. I don't think findExecutables should be exported. But if someone needs it, it should behave consistently between Windows and other systems.

Yes, that is what I was thinking as well. I guess I'll remove the export for findExecutables for now.

Changed 7 months ago by leroux

Final complete patch - findExecutable checks for the executable bit.

comment:16 Changed 7 months ago by leroux

  • Priority changed from lowest to normal

comment:17 Changed 7 months ago by simonpj

  • Milestone changed from 7.6.2 to 7.8.1
  • Priority changed from normal to high

I know nothing about this, but I'll milestone it for 7.8.1 so that Austin is sure to review it.

Austin: I'm not expressing an opinion, but could you look at this since we have a patch offered?

Simon

comment:18 Changed 6 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from patch to closed

Merged, thanks!

Note: See TracTickets for help on using tickets.