Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Description • 6 years ago

`find_binary` currently can fail if eg Firefox is extracted into a `firefox` subdirectory, but the path points to the folder a level higher. 

See: https://github.com/mozilla/geckodriver/issues/1288

Instead the following code should make sure that the path found is a file and executable:

https://dxr.mozilla.org/mozilla-central/rev/a466172aed4bc2afc21169b749b8068a4b98c93f/testing/mozbase/rust/mozrunner/src/runner.rs#346

Andreas, mind mentoring this bug?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 1 • 6 years ago

(In reply to Henrik Skupin (:whimboo) from comment #0)
> Andreas, mind mentoring this bug?

No please go ahead if you want to mentor it.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 2 • 6 years ago

Actually, I’m already doing some Rust work today.  Fixing this seems
trivial.

So determining if a file is executable in Rust is trickier than I
thought.  We can certainly do it through std::os::unix::fs::PermissionsExt,
but I notice there are already a micro-library that provide this
functionality:
https://github.com/fitzgen/is_executable/blob/master/src/lib.rs

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 6 years ago

Instead of having to vendor a new crate of just a couple of tens
of lines of code, I’ll create a new "path" module in mozrunner which
just has the base necessities of what we need.  This will provide
only one public function for searching the system path.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8981523 [details]
Bug 1464995 - Document mozrunner::firefox_default_path().

https://reviewboard.mozilla.org/r/247636/#review253980

::: testing/mozbase/rust/mozrunner/src/runner.rs:416
(Diff revision 2)
>      use std::io::Error;
>      use std::path::PathBuf;
>      use winreg::RegKey;
>      use winreg::enums::*;
>  
> +    /// Searches the Windows registry, then the system path for `firefox.exe`.

Please note that we still have https://github.com/mozilla/geckodriver/issues/1045#issuecomment-341960393. I wonder if we should get this added until it's fixed. Btw, I cannot find a bug about it yet?

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8981524 [details]
Bug 1464995 - Minor readability lints.

https://reviewboard.mozilla.org/r/247638/#review253982

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 12 • 6 years ago
mozreview-review

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review253988

Can you please trigger a try build first?

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 13 • 6 years ago
mozreview-review

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review254616

I think that looks fine but would like to have another pair of eyes on it from James.

	James Graham [:jgraham]
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review254630

::: testing/mozbase/rust/mozrunner/src/path.rs:19
(Diff revision 2)
> +            .ok()
> +            .map_or(false, |meta| meta.permissions().is_executable())
> +    }
> +}
> +
> +impl IsExecutable for fs::Permissions {

Having a private trait here seems a little like needless indirection compared to just defining an is_excutable function.

::: testing/mozbase/rust/mozrunner/src/path.rs:23
(Diff revision 2)
> +
> +impl IsExecutable for fs::Permissions {
> +    #[cfg(unix)]
> +    fn is_executable(&self) -> bool {
> +        use std::os::unix::fs::PermissionsExt;
> +        // 111 binary == 7 octal

I don't understand what this comment is trying to tell me?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 15 • 6 years ago
mozreview-review-reply

Comment on attachment 8981523 [details]
Bug 1464995 - Document mozrunner::firefox_default_path().

https://reviewboard.mozilla.org/r/247636/#review253980

> Please note that we still have https://github.com/mozilla/geckodriver/issues/1045#issuecomment-341960393. I wonder if we should get this added until it's fixed. Btw, I cannot find a bug about it yet?

I will point out in the docs that it does not search the
HKEY_CURRENT_USER.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 16 • 6 years ago
mozreview-review-reply

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review254630

> I don't understand what this comment is trying to tell me?

111 doesn’t tell me anything because I don’t read binary, but the
octal code 7 is what you use as input for chmod(1) for changing the
executable bit.  I find that more instructive.

	James Graham [:jgraham]
	Comment 20 • 6 years ago
mozreview-review-reply

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review254630

> 111 doesn’t tell me anything because I don’t read binary, but the
> octal code 7 is what you use as input for chmod(1) for changing the
> executable bit.  I find that more instructive.

But the constant is written in octal, not binary? I assumed that was the point since afaict the permissions flags are two bytes, interpreted as 4 4 bit numbers, of which only the last 3 are significant here. Then, for each 4 bit number (which is a single octal digit) you're interested in whether the lowest bit is set, which gives the executable flag. So in octal your mask is `0o0111`, but in binary it would be `0b0000000100010001`.

Is the confusion from `chmod 777` to make something executable? I think that's misleading because that's just setting all the flages i.e. all of read, write, and executable to true.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 21 • 6 years ago
mozreview-review-reply

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review254630

> But the constant is written in octal, not binary? I assumed that was the point since afaict the permissions flags are two bytes, interpreted as 4 4 bit numbers, of which only the last 3 are significant here. Then, for each 4 bit number (which is a single octal digit) you're interested in whether the lowest bit is set, which gives the executable flag. So in octal your mask is `0o0111`, but in binary it would be `0b0000000100010001`.
> 
> Is the confusion from `chmod 777` to make something executable? I think that's misleading because that's just setting all the flages i.e. all of read, write, and executable to true.

I will trust that what you say is correct.

What concrete proposal do you have to make this readable?  We could
put the value in a variable with a descriptive name.

	James Graham [:jgraham]
	Comment 22 • 6 years ago
mozreview-review-reply

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review254630

> I will trust that what you say is correct.
> 
> What concrete proposal do you have to make this readable?  We could
> put the value in a variable with a descriptive name.

I think a comment along the lines of

```
Permissions are a set of four 4-bit bitflags, represented by a single octal digit. The lowest bit of each of the last three values represents the executable permission for all, group and user, repsectively. We assume the file is executable if any of these are set
```

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 26 • 6 years ago
mozreview-review-reply

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review254630

> I think a comment along the lines of
> 
> ```
> Permissions are a set of four 4-bit bitflags, represented by a single octal digit. The lowest bit of each of the last three values represents the executable permission for all, group and user, repsectively. We assume the file is executable if any of these are set
> ```

Thanks, I’ve updated the patch.

	James Graham [:jgraham]
	Comment 28 • 6 years ago
mozreview-review

Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review255438

	Pulsebot
	Comment 32 • 6 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a4e0a2002ef
Document mozrunner::firefox_default_path(). r=whimboo
https://hg.mozilla.org/integration/autoland/rev/576357318f40
Minor readability lints. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/595995b9a63e
Ensure found Firefox is an executable binary. r=jgraham,whimboo

	Arthur Iakab [arthur_iakab]
	Comment 33 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0a4e0a2002ef
https://hg.mozilla.org/mozilla-central/rev/576357318f40
https://hg.mozilla.org/mozilla-central/rev/595995b9a63e