third_party/WebKit/Tools/Scripts/webkitpy/common/system/path.py uses subprocess in unsafe manner |
|||||
Issue description
The code in third_party/WebKit/Tools/Scripts/webkitpy/common/system/path.py uses subprocess in the "class _CygPath" to """Manages a long-running 'cygpath' process for file conversion."""
The approach currently in use is unsafe and can potentially block forever;
---------------------------
def convert(self, path):
if not self.is_running():
self.start()
self._child_process.stdin.write("%s\r\n" % path)
self._child_process.stdin.flush()
windows_path = self._child_process.stdout.readline().rstrip()
# Some versions of cygpath use lowercase drive letters while others
# use uppercase. We always convert to uppercase for consistency.
windows_path = '%s%s' % (windows_path[0].upper(), windows_path[1:])
return windows_path
---------------------------
If the path is ever near the buffer size, this can block forever as the stdin call can't complete until stdout is read from. The subprocess.Popen doesn't set the buffering size/mode and hence depends on the operating system set up, python version, etc.
Possible fixes might be;
* Set an explicit buffering size the subprocess.Popen call and explicitly manage the buffering.
* Change to subprocess42?
,
May 11 2016
webkitpy does a bare minimum of supporting cygwin; it wouldn't actually surprise me too much if run-webkit-tests didn't work at all under cygwin at this point. So, we could remove support for it completely, or get rid of the long-running process, or just do a couple of simple lookups (since nearly all of our lookups will be from one or two subtrees) and do the rest w/o talking to the subprocess.
,
May 18 2016
,
May 18 2016
Deprecating component:Blink>LayoutTests, to use label Test=Layout instead. Merging these to component:Blink for the Blink rotation to pick up and re-triage as appropriate.
,
May 23 2016
,
Apr 5 2017
Quick CL to remove support for cygwin entirely: https://codereview.chromium.org/2799713002
,
Apr 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d04df31fccaf2a270f6cd30300cf99b6131625e5 commit d04df31fccaf2a270f6cd30300cf99b6131625e5 Author: qyearsley <qyearsley@chromium.org> Date: Thu Apr 06 18:57:22 2017 Remove all support for cygwin in run-webkit-tests. BUG= 610614 Review-Url: https://codereview.chromium.org/2799713002 Cr-Commit-Position: refs/heads/master@{#462566} [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_unittest.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/common/system/path.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/common/system/path_unittest.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/common/system/platform_info.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/common/system/platform_info_mock.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/common/system/platform_info_unittest.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/server_process.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/win.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_unittest.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/crash_service_unittest.py [modify] https://crrev.com/d04df31fccaf2a270f6cd30300cf99b6131625e5/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/pretty_diff.py
,
Apr 6 2017
,
Jul 12 2017
#2 > it wouldn't actually surprise me too much if run-webkit-tests didn't work at all under cygwin at this point. Actually, it was working fine. I was rebasing tests on Win with it, expecting it to work per usual, and found this bug ... so could we bring it back please?
,
Jul 13 2017
I would rather not. very few people use this config and supporting cygwin historically has had all sorts of weirdness, though I don't know if this is better or worse on win10. I recommend you use either the cmd shell or msys git bash. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tansell@chromium.org
, May 10 2016