On LayoutTestApacheHttpd._run(), subprocess.Popen is used with `shell=True`. It is usually bad idea to pass shell=True, and there's no strong reason we should use shell=True in the code. On the other hand, if we remove this, we get benefits like: - no intermediate process which may ended up for security risk - arguments are array, not a string, which is easy to manage. - no need to quoting a lot.
Created attachment 334716 [details] WIP first trial
shell=True was added in this commit <https://trac.webkit.org/changeset/54091/webkit>. Let's make sure we aren't breaking the reason it was used in the first place.
(In reply to Jonathan Bedard from comment #2) > shell=True was added in this commit > <https://trac.webkit.org/changeset/54091/webkit>. Let's make sure we aren't > breaking the reason it was used in the first place. Yes, we have to check the validity of shell=True first. For native Windows, with shell=True, it can't launch process correctly. I don't understand why the original patch did this. In general, shell=True causes platform dependent issue. For the sake of Windows, shell=False should be used. Did python for Windows have such issue at that moment? Nobody knows. Anyway, I confirmed Mac is okay. Native Windows requires this to be False. I am asking GTK guys, but with quick test on linux, this shouldn't be the issue becaused passed arguments don't use shell expansions or environment variable resolution.
On the IRC, Carlos Garcia Campos said at 9:21 am 2/28/2018: > just land it, yes, if it eventually breaks gtk or wpe tests we can try to fix it, or simply roll out the patch and find a solution
(In reply to Basuke Suzuki from comment #3) > (In reply to Jonathan Bedard from comment #2) > ... > > Yes, we have to check the validity of shell=True first. > > For native Windows, with shell=True, it can't launch process correctly. I > don't understand why the original patch did this. In general, shell=True > causes platform dependent issue. For the sake of Windows, shell=False should > be used. Did python for Windows have such issue at that moment? Nobody knows. According to the original commit, it looks like shell=True was for Cygwin. I just talked to Per
...and we're running Windows layout tests in EWS now, so the fact that this patch passed EWS shows that Cygwin is working.
(In reply to Jonathan Bedard from comment #6) > ...and we're running Windows layout tests in EWS now, so the fact that this > patch passed EWS shows that Cygwin is working. So would this be good to land then?
(In reply to Don Olmstead from comment #7) > (In reply to Jonathan Bedard from comment #6) > > ...and we're running Windows layout tests in EWS now, so the fact that this > > patch passed EWS shows that Cygwin is working. > > So would this be good to land then? I don't see any issue with it landing.
Great. I'll prepare a patch for landing. Thanks.
Just for future reference, these are passed arguments at this moment: C:\xampp\apache\bin\httpd.exe -f "C:\Users\basuke\webkit-trunk\WebKitBuild\Release\bin64\layout-test-results\httpd.conf" -C 'DocumentRoot "C:\Users\basuke\webkit-trunk\LayoutTests\http\tests"' -c 'TypesConfig "C:\Users\basuke\webkit-trunk\LayoutTests\http\conf\mime.types"' -c 'PHPINIDir "C:\Users\basuke\webkit-trunk\LayoutTests\http\conf"' -c 'CustomLog "C:\Users\basuke\webkit-trunk\WebKitBuild\Release\bin64\layout-test-results\access_log.txt" common' -c 'ErrorLog "C:\Users\basuke\webkit-trunk\WebKitBuild\Release\bin64\layout-test-results\error_log.txt"' -c 'PidFile c:\users\basuke\appdata\local\temp\WebKit\httpd.pid' -k start -c 'Alias /js-test-resources "C:\Users\basuke\webkit-trunk\LayoutTests\resources"' -c 'Alias /media-resources "C:\Users\basuke\webkit-trunk\LayoutTests\media"' -c 'Alias /modern-media-controls "C:\Users\basuke\webkit-trunk\LayoutTests\../Source/WebCore/Modules/modern-media-controls"' -c 'Alias /resources/testharness.css "C:\Users\basuke\webkit-trunk\LayoutTests\resources/testharness.css"' -c 'Alias /resources/testharness.js "C:\Users\basuke\webkit-trunk\LayoutTests\resources/testharness.js"' -c 'Alias /resources/testharnessreport.js "C:\Users\basuke\webkit-trunk\LayoutTests\resources/testharnessreport.js"' -C 'Listen 127.0.0.1:8000' -C 'Listen [::1]:8000' -C 'Listen 127.0.0.1:8080' -C 'Listen [::1]:8080' -C 'Listen 127.0.0.1:8443' -C 'Listen [::1]:8443' -c 'SSLCertificateFile C:\Users\basuke\webkit-trunk\LayoutTests\http\conf\webkit-httpd.pem'
Created attachment 334752 [details] PATCH
Comment on attachment 334752 [details] PATCH Clearing flags on attachment: 334752 Committed r229141: <https://trac.webkit.org/changeset/229141>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38038263>