1524584 - Add check that binary is executable

Status: NEW

(Testing :: geckodriver, defect, P3)

Description

[Andreas Tolfsen ❲:ato❳]

As noted in https://github.com/mozilla/geckodriver/issues/1488, it
would be helpful if geckodriver returned an error immediately if
the passed binary capability value is not an executable binary.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks like you want to fix it. So we can have it as part of the 0.25.0 release.

Comment 2

[Andreas Tolfsen ❲:ato❳]

Command::spawn() does seem to be returning an error (PermissionDenied)
when attempting to run a non-executable:

Command::new("/etc/hosts").spawn() = Err(
    Os {
        code: 13,
        kind: PermissionDenied,
        message: "Permission denied"
    }
)

So somewhere along the way this error is discarded.

Comment 3

[Andreas Tolfsen ❲:ato❳]

The same goes if you set --binary /etc/hosts as an argument:

% MOZ_HEADLESS=1 ./mach geckodriver -v --binary /etc/hosts
 0:00.40 /home/ato/src/gecko/obj-x86_64-pc-linux-gnu/dist/bin/geckodriver -v --binary /etc/hosts
1549388773543	webdriver::httpapi	DEBUG	Creating routes
1549388773569	geckodriver	DEBUG	Listening on 127.0.0.1:4444
1549388776028	webdriver::server	DEBUG	-> POST /session {}
1549388776029	webdriver::command	WARN	You are using deprecated legacy session negotiation patterns (desiredCapabilities/requiredCapabilities), see https://developer.mozilla.org/en-US/docs/Web/WebDriver/Capabilities#Legacy
1549388776033	mozrunner::runner	INFO	Running command: "/etc/hosts" "-marionette" "-foreground" "-no-remote" "-profile" "/tmp/rust_mozprofile.MUHPZzMHkwVO"
1549388776039	webdriver::server	DEBUG	<- 500 Internal Server Error {"value":{"error":"session not created","message":"Failed to start browser /etc/hosts: permission denied","stacktrace":""}}

And when configuring it via firefoxOptions:

% curl -d '{"capabilities": {"alwaysMatch": {"moz:firefoxOptions": {"binary": "/etc/hosts"}}}}' http://localhost:4444/session
{"value":{"error":"session not created","message":"Failed to start browser /etc/hosts: permission denied","stacktrace":""}}

Same goes for using a directory as binary:

% curl -d '{"capabilities": {"alwaysMatch": {"moz:firefoxOptions": {"binary": "/home/ato"}}}}' http://localhost:4444/session
{"value":{"error":"session not created","message":"Failed to start browser /home/ato: permission denied","stacktrace":""}}

Comment 4

[Andreas Tolfsen ❲:ato❳]

I’m able to reproduce the same problem as afriestad in
https://github.com/mozilla/geckodriver/issues/1488:

% cat binary.py 
from selenium import webdriver
from selenium.webdriver.firefox.options import Options

opts = Options()
opts.log.level = "trace"
driver = webdriver.Firefox(options=opts, firefox_binary="/etc")
% MOZ_HEADLESS=1 PATH=~/src/gecko/target/debug:$PATH python binary.py 
Traceback (most recent call last):
  File "binary.py", line 6, in <module>
    driver = webdriver.Firefox(options=opts, firefox_binary="/etc")
  File "/home/ato/_binary/local/lib/python2.7/site-packages/selenium/webdriver/firefox/webdriver.py", line 174, in __init__
    keep_alive=True)
  File "/home/ato/_binary/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 157, in __init__
    self.start_session(capabilities, browser_profile)
  File "/home/ato/_binary/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 252, in start_session
    response = self.execute(Command.NEW_SESSION, parameters)
  File "/home/ato/_binary/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 321, in execute
    self.error_handler.check_response(response)
  File "/home/ato/_binary/local/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", line 242, in check_response
    raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.SessionNotCreatedException: Message: Unable to find a matching set of capabilities

And unfortunately the geckodriver.log file is empty, which leads
me to suspect that this is a bug—possibly two!—in the Selenium
Python client:

1. We would not expect an “[u]nable to find a matching set of
   capabilities” if the binary capability is wrong, so Selenium
   must be construing the capabilities JSON Object in some bad way.

2. geckodriver.log should not be empty when the log level is set
   to trace,.

The second point could be explained by the logging level not being
set internally in geckodriver until after the capabilities have
been passed. If the Selenium client had started geckodriver with
-v or -vv this problem would presumably go away.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Using the webdriver.Firefox class’ service_args kwarg I’m able
to finally get some output written to geckodriver.log.

It shows that the above Python script makes the Selenium Python
client construct the following JSON Object for creating a new
session:

{
  "capabilities": {
    "alwaysMatch": {
      "acceptInsecureCerts": true,
      "browserName": "firefox",
      "moz:firefoxOptions": {
        "binary": "/etc",
        "log": {
          "level": "trace"
        }
      }
    },
    "firstMatch": [
      {}
    ]
  },
  "desiredCapabilities": {
    "acceptInsecureCerts": true,
    "browserName": "firefox",
    "moz:firefoxOptions": {
      "binary": "/etc",
      "log": {
        "level": "trace"
      }
    },
    "marionette": true
  }
}

Besides needlessly duplicating the capabilities object, we know
that geckodriver always picks up the top-level capabilities field
first and then goes into W3C-conforming capabilities parsing mode.

From the geckodriver log we learn that it trips up on reading version
information:

1549391206227	webdriver::server	DEBUG	-> POST /session {"capabilities": {"alwaysMatch": {"acceptInsecureCerts": true, "browserName": "firefox", "moz:firefoxOptions": {"binary": "/etc", "log": {"level": "trace"}}}, "firstMatch": [{}]}}
1549391206228	geckodriver::capabilities	DEBUG	Trying to read firefox version from ini files
1549391206228	geckodriver::capabilities	DEBUG	Trying to read firefox version from binary
1549391206230	geckodriver::capabilities	DEBUG	Failed to get binary version

I thought this was only meant to happen when you pass in a
browserVersion restriction to the capabilities, but I found that
we also call BrowserCapabilities::version() (in turn calling
::version_from_binary()) when acceptInsecureCerts is being
used.

Comment 6

[Andreas Tolfsen ❲:ato❳]

I think the fix here would be to have a stricter binary check
earlier during capabilities parsing that would tell us there’s
something wrong with the binary. At the moment we don’t do any
checks, apart from type checks.

In order to facilitate this in a meaningful way, we would need to
return better error messages from capabilities parsing. If add
only a better binary check, we would end up with the same cryptic
error as afriestad complained about initially.

That problem was originally described in
https://github.com/mozilla/geckodriver/issues/832 and I’ve decided
to repurpose https://bugzilla.mozilla.org/show_bug.cgi?id=1410838
to follow up on it.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #5)

I thought this was only meant to happen when you pass in a
browserVersion restriction to the capabilities, but I found that
we also call BrowserCapabilities::version() (in turn calling
::version_from_binary()) when acceptInsecureCerts is being
used.

Note that with the fix on bug 1658870 we no longer do the binary version check for acceptInsecureCerts. Also with additional fixes for version parsing the failure looks like this with geckodriver 0.27.0:

1598262147085 webdriver::server DEBUG <- 400 Bad Request {"value":{"error":"invalid argument","message":"binary is not a Firefox executable","stacktrace":""}}

I wonder if there is anything else we should do here. It's very unlikely that a Firefox binary is passed that is not executable. I would downgrade the priority and clearly not block on any upcoming geckodriver release. James, what do you think?

Comment 8

[James Graham [:jgraham]]

I agree that there's something that could be improved here. I also agree it's not a high priority at this time (but P3 seems fine).