Bug 183191 – [webkitpy] Use shell=False to launch apache http server.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 334716 [details]
WIP first trial

[Jonathan Bedard]

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.

[Basuke Suzuki]

(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.

[Basuke Suzuki]

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

[Jonathan Bedard]

(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

[Jonathan Bedard]

...and we're running Windows layout tests in EWS now, so the fact that this patch passed EWS shows that Cygwin is working.

[Don Olmstead]

(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?

[Jonathan Bedard]

(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.

[Basuke Suzuki]

Great. I'll prepare a patch for landing. Thanks.

[Basuke Suzuki]

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'

[Basuke Suzuki]

Created attachment 334752 [details]
PATCH

[WebKit Commit Bot]

Comment on attachment 334752 [details]
PATCH

Clearing flags on attachment: 334752

Committed r229141: <https://trac.webkit.org/changeset/229141>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38038263>