Open Bug 1524584 Opened 8 months ago Updated Last month

Add check that binary is executable

Categories

(Testing :: geckodriver, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: ato, Unassigned)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

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.

Blocks: 1489130
Priority: -- → P3
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: P3 → P1

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

Blocks: 1520585

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.

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":""}}

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.

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
.

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.

Depends on: 1410838
Assignee: ato → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Blocks: 1573798
No longer blocks: 1520585
You need to log in before you can comment on or make changes to this bug.