Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > Julia Boes has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Merge branch 'master' into simpleserver > - Merge remote-tracking branch 'origin/simpleserver' into simpleserver > - Merge branch 'master' into simpleserver > - refactor isHidden,isReadable,isSymlink checks and cleanup tests > - Merge branch 'master' into simpleserver > - check isHidden, isSymlink, isReadable for all path segments > - add checks for all path segments > - Merge branch 'master' into componentcheck > - Merge branch 'master' into simpleserver > - improve output on startup > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131 Overall, this is a very nice addition. Good work. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 15:23:33 GMT, Michael McMahon wrote:
>> Julia Boes has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' into simpleserver
>> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>> - Merge branch 'master' into simpleserver
>> - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>> - Merge branch 'master' into simpleserver
>> - check isHidden, isSymlink, isReadable for all path segments
>> - add checks for all path segments
>> - Merge branch 'master' into componentcheck
>> - Merge branch 'master' into simpleserver
>> - improve output on startup
>> - ... and 6 more:
>> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
>
> test/jdk/com/sun/net/httpserver/FilterTest.java line 330:
>
>> 328: var response = client.send(request,
>> HttpResponse.BodyHandlers.ofString());
>> 329: assertEquals(response.statusCode(), 200);
>> 330: assertEquals(inspectedURI.get(), URI.create("/"));
>
> Could you make the request URI something like /foo/bar instead of just / here?
Yes, I'll do that.
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 16:04:21 GMT, Daniel Fuchs wrote:
>> Julia Boes has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' into simpleserver
>> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>> - Merge branch 'master' into simpleserver
>> - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>> - Merge branch 'master' into simpleserver
>> - check isHidden, isSymlink, isReadable for all path segments
>> - add checks for all path segments
>> - Merge branch 'master' into componentcheck
>> - Merge branch 'master' into simpleserver
>> - improve output on startup
>> - ... and 6 more:
>> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
>
> src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/FileServerHandler.java
> line 314:
>
>> 312: + "\n");
>> 313: try (var paths = Files.list(path)) {
>> 314: paths.filter(p -> !isHiddenOrSymLink(p))
>
> Shouldn't we filter paths that are not readable here too? There's no point in
> printing a link that will result in 404, is there?
Totally, thanks for noting!
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8238274: (sctp) JDK-7118373 is not fixed for SctpChannel [v4]
> Please review this change to the Unix implementations of > sun.nio.ch.sctp.Sctp*ChannelImpl#implCloseSelectableChannel() > to be same as SocketChannelImpl at JDK-7118373. (The preClose is missing a > check for the ST_KILLED state.) Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8238274: (sctp) JDK-7118373 is not fixed for SctpChannel - Changes: - all: https://git.openjdk.java.net/jdk/pull/5274/files - new: https://git.openjdk.java.net/jdk/pull/5274/files/c3bfdfc4..5ccdaed3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5274&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5274&range=02-03 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5274.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5274/head:pull/5274 PR: https://git.openjdk.java.net/jdk/pull/5274
Re: RFR: 8238274: (sctp) JDK-7118373 is not fixed for SctpChannel [v2]
On Thu, 16 Sep 2021 10:33:09 GMT, Daniel Fuchs wrote:
>> I pushed a fix to run the test in othervm mode. Please test again.
>
> Meanwhile I did some experiment and was puzzled by the fact that the new test
> still failed intermittently - while the corresponding
> SctpMultiChannel/CloseDescriptors did not. By comparing the two I noticed
> that the SctpMultiChannel test is introducing some delays at key places. I
> did the same to the new test - and behold! It stopped failing.
>
> Here are my changes (ignore the /othervm which you already did):
>
>
> diff --git a/test/jdk/com/sun/nio/sctp/SctpChannel/CloseDescriptors.java
> b/test/jdk/com/sun/nio/sctp/SctpChannel/CloseDescriptors.java
> index 99f9563e016..11e2e52f577 100644
> --- a/test/jdk/com/sun/nio/sctp/SctpChannel/CloseDescriptors.java
> +++ b/test/jdk/com/sun/nio/sctp/SctpChannel/CloseDescriptors.java
> @@ -26,7 +26,7 @@
> * @bug 8238274
> * @summary Potential leak file descriptor for SCTP
> * @requires (os.family == "linux")
> - * @run main CloseDescriptors
> + * @run main/othervm CloseDescriptors
> */
>
> import java.io.BufferedReader;
> @@ -79,9 +79,12 @@ public class CloseDescriptors {
> selThread = new SelectorThread();
> selThread.start();
>
> +// give time for the server and selector to start
> +Thread.sleep(100);
> for (int i = 0 ; i < 100 ; ++i) {
> System.out.println(i);
> doIt(port);
> +Thread.sleep(100);
> }
> System.out.println("end");
> if (!check()) {
> @@ -110,6 +113,7 @@ public class CloseDescriptors {
> catch (Exception ex) {
> ex.printStackTrace();
> }
> +Thread.sleep(200);
> }
> }
>
> @@ -199,4 +203,3 @@ public class CloseDescriptors {
> }
> }
> }
Thank you for your help. I applied your suggested fix and it works fine. But it
takes about 4 minutes to be finished. So I added a timeout parameter to 250.
-
PR: https://git.openjdk.java.net/jdk/pull/5274
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 17:16:08 GMT, Michael McMahon wrote: >> Julia Boes has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 16 commits: >> >> - Merge branch 'master' into simpleserver >> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver >> - Merge branch 'master' into simpleserver >> - refactor isHidden,isReadable,isSymlink checks and cleanup tests >> - Merge branch 'master' into simpleserver >> - check isHidden, isSymlink, isReadable for all path segments >> - add checks for all path segments >> - Merge branch 'master' into componentcheck >> - Merge branch 'master' into simpleserver >> - improve output on startup >> - ... and 6 more: >> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131 > > src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/SimpleFileServerImpl.java > line 135: > >> 133: var socketAddr = new InetSocketAddress(addr, port); >> 134: var server = SimpleFileServer.createFileServer(socketAddr, >> root, outputLevel); >> 135: server.setExecutor(Executors.newSingleThreadExecutor()); > > I think this code has the effect of creating one thread for the selector and > a second one for the execution of the handlers. If we want to keep thread > usage to an absolute minimum then it might be better to not set an executor. > Then, the selector thread executes the handlers as well. I did a quick stress test of both versions (with and without executor specified) and don’t see any difference in how many requests can be handled. Probably good to keep thread usage to a minimum, I'll update that. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v6]
> This change implements a simple web server that can be run on the > command-line with `java -m jdk.httpserver`. > > This is facilitated by adding an entry point for the `jdk.httpserver` module, > an implementation class whose main method is run when the above command is > executed. This is the first such module entry point in the JDK. > > The server is a minimal HTTP server that serves the static files of a given > directory, similar to existing alternatives on other platforms and convenient > for testing, development, and debugging. > > Additionally, a small API is introduced for programmatic creation and > customization. > > Testing: tier1-3. Julia Boes has updated the pull request incrementally with two additional commits since the last revision: - change default bind address from anylocal to loopback - address PR comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5505/files - new: https://git.openjdk.java.net/jdk/pull/5505/files/fe059131..639e018a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5505&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5505&range=04-05 Stats: 103 lines in 14 files changed: 22 ins; 38 del; 43 mod Patch: https://git.openjdk.java.net/jdk/pull/5505.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5505/head:pull/5505 PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v3]
On Fri, 17 Sep 2021 14:11:38 GMT, Julia Boes wrote: > Thanks for sharing your experience on this, it's appreciated. 0.0.0.0 is > common default for Apache httpd [1], Ngnix [2], the Python web server [3]. > This being said, I want to make sure we're taking the right decision here so > let me seek some further advice on this. > > [1] http://httpd.apache.org/docs/2.4/bind.html > [2] https://docs.nginx.com/nginx/admin-guide/web-server/web-server/ > [3] https://github.com/python/cpython/blob/3.4/Lib/http/server.py Further review concluded that a default binding to 0.0.0.0 creates too a high level of exposure, particularly for a low-threshold utility like this server. Binding to an unrestricted address is a known way for attackers to launch a Denial-of-Service attack, classified by MITRE as CWE-1327 [1]. We therefore update the default binding to the loopback address and amend the help output with information on how to bind to 0.0.0.0, e.g.: $ java -m jdk.httpserver -h Usage: java -m jdk.httpserver [-b bind address] [-p port] [-d directory] [-o none|info|verbose] [-h to show options] Options: -b, --bind-address- Address to bind to. Default: 127.0.0.1 (loopback). For 0.0.0.0 (all interfaces) use -b 0.0.0.0 or -b ::0. -d, --directory - Directory to serve. Default: current directory. -o, --output - Output format. none|info|verbose. Default: info. -p, --port- Port to listen on. Default: 8000. -h, -?, --help- Print this help message. To stop the server, press Ctrl + C. ``` Thanks again for flagging this, @jaikiran . [1] https://cwe.mitre.org/data/definitions/1327.html - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v6]
On Wed, 22 Sep 2021 15:26:33 GMT, Julia Boes wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > Julia Boes has updated the pull request incrementally with two additional > commits since the last revision: > > - change default bind address from anylocal to loopback > - address PR comments When the -b option is not explicitly specified on the command line it would be good to print a message that says that the server is bound to the loopback by default, and print the command line that would be needed to bind to all interfaces instead (or instruct the user to call `java -m jdk.httpserver --help` to learn how to bind to all interfaces). I don't think your latest changes include that. src/jdk.httpserver/share/classes/module-info.java line 55: > 53: * [-o none|info|verbose] [-h to show > options] > 54: *Options: > 55: *-b, --bind-address- Address to bind to. Default: 127.0.0.1 > (loopback). This assumes that the machine on which the server is run has IPv4 configured. It might not be the case. It can also depend on whether -Djdk.net.preferIPv6Addresses=true is specified on the java command line. Maybe this should be: `127.0.0.1 (or ::1), (loopback).` src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/SimpleFileServerImpl.java line 52: > 50: * > 51: * Unless specified as arguments, the default values are: > 52: * bind address: 127.0.0.1 (loopback) maybe this should say: `127.0.01 (or ::1), (loopback)` src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/resources/simpleserver.properties line 32: > 30: options=\ > 31: Options:\n\ > 32: -b, --bind-address- Address to bind to. Default: 127.0.0.1 > (loopback).\n\ This assumes that the machine on which the server is run has IPv4 configured. It might not be the case. It can also depend on whether -Djdk.net.preferIPv6Addresses=true is specified on the java command line. The actual address of the loopback (as returned by InetAddress.getLoopackeAddress()) should therefore preferably be passed as parameter to any message that talks about the loopback. It is not such an issue for the wildcard - because AFAIU there's no difference between 0.0.0.0 and ::0 at the OS level src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/resources/simpleserver.properties line 42: > 40: opt.bindaddress=\ > 41: \-b, --bind-address- Address to bind to. Default: 127.0.0.1 > (loopback).\n\ > 42: \ For 0.0.0.0 (all interfaces) use -b 0.0.0.0 or > -b ::0. is `opt.bindaddress` used somewhere? I couldn't find it. Same for the other `opt.*` properties below. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: JDK-8273142 : Remove dependancy of TestHttpServer, HttpTransaction, HttpCallback from open/test/jdk/sun/net/www/protocol/http/ tests [v2]
> Dependencies of TestHttpServre, HttpTransaction, HttpCallback are removed > from following tests: > open/test/jdk/sun/net/www/protocol/http/ResponseCacheStream.java > open/test/jdk/sun/net/www/protocol/http/SetChunkedStreamingMode.java > open/test/jdk/sun/net/www/protocol/http/RelativeRedirect.java > open/test/jdk/sun/net/www/protocol/http/B6296310.java Mahendra Chhipa has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Updated execution mode to othervm to resolve cleaning up thread issue. - Merge branch 'master' into JDK-8273142 - JDK-8273142 : Remove dependancy of TestHttpServer, HttpTransaction, HttpCallback from open/test/jdk/sun/net/www/protocol/http/ tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/5352/files - new: https://git.openjdk.java.net/jdk/pull/5352/files/f385c18c..b0bd0683 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5352&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5352&range=00-01 Stats: 30170 lines in 1267 files changed: 19486 ins; 5798 del; 4886 mod Patch: https://git.openjdk.java.net/jdk/pull/5352.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5352/head:pull/5352 PR: https://git.openjdk.java.net/jdk/pull/5352
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v3]
On Wed, 22 Sep 2021 15:20:21 GMT, Julia Boes wrote: > > Thanks for sharing your experience on this, it's appreciated. 0.0.0.0 is > > common default for Apache httpd [1], Ngnix [2], the Python web server [3]. > > This being said, I want to make sure we're taking the right decision here > > so let me seek some further advice on this. > > [1] http://httpd.apache.org/docs/2.4/bind.html > > [2] https://docs.nginx.com/nginx/admin-guide/web-server/web-server/ > > [3] https://github.com/python/cpython/blob/3.4/Lib/http/server.py > > Further review concluded that a default binding to 0.0.0.0 creates too a high > level of exposure, particularly for a low-threshold utility like this server. > Binding to an unrestricted address is a known way for attackers to launch a > Denial-of-Service attack, classified by MITRE as CWE-1327 [1]. We therefore > update the default binding to the loopback address and amend the help output > with information on how to bind to 0.0.0.0 Thank you Julia for considering this input and coordinating the change. - PR: https://git.openjdk.java.net/jdk/pull/5505
