Closed Bug 1464995 Opened 2 years ago Closed 2 years ago

[mozrunner] find_binary should check for file type and executable bit

Categories

(Testing :: Mozbase Rust, enhancement, P3)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: whimboo, Assigned: ato)

References

()

Details

Attachments

(3 files)

`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?
Flags: needinfo?(ato)
Priority: -- → P3
(In reply to Henrik Skupin (:whimboo) from comment #0)
> Andreas, mind mentoring this bug?

No please go ahead if you want to mentor it.
Flags: needinfo?(ato)
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
Assignee: nobody → ato
Status: NEW → ASSIGNED
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.
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?
Attachment #8981523 - Flags: review?(hskupin) → review+
Comment on attachment 8981524 [details]
Bug 1464995 - Minor readability lints.

https://reviewboard.mozilla.org/r/247638/#review253982
Attachment #8981524 - Flags: review?(hskupin) → 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?
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.
Attachment #8981525 - Flags: review?(hskupin) → 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?
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.
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.
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.
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.
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
```
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.
Blocks: 1441204
Comment on attachment 8981525 [details]
Bug 1464995 - Ensure found Firefox is an executable binary.

https://reviewboard.mozilla.org/r/247640/#review255438
Attachment #8981525 - Flags: review?(james) → review+
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
You need to log in before you can comment on or make changes to this bug.