Arno,
I found an issue on Windows when proxies are specified
per-protocol, i.e. they are returned with their optional
scheme set. I believe that the scheme should be checked,
at least without this change FTP proxies were being
returned for HTTP URL's, on my machine.
$ hg -R jdk diff
diff -r 07e07fecf383
src/java.base/windows/native/libnet/DefaultProxySelector.c
--- a/src/java.base/windows/native/libnet/DefaultProxySelector.c
Mon Jan 30 14:09:14 2017 +0000
+++ b/src/java.base/windows/native/libnet/DefaultProxySelector.c
Mon Jan 30 16:09:23 2017 +0000
@@ -99,6 +99,7 @@
* Returns the size of the array as int.
*/
static int createProxyList(LPWSTR win_proxy, const WCHAR *pproto,
list_item **head) {
+ static const wchar_t separators[] = L"\t\r\n ;";
list_item *current = NULL;
int nr_elems = 0;
wchar_t *context = NULL;
@@ -109,13 +110,26 @@
* The proxy server list contains one or more of the following
strings separated by semicolons or whitespace.
* ([<scheme>=][<scheme>"://"]<server>[":"<port>])
*/
- current_proxy = wcstok_s(win_proxy, L"; ", &context);
- while (current_proxy != NULL) {
+ current_proxy = wcstok_s(win_proxy, separators, &context);
+ while (current_proxy != NULL) {
LPWSTR pport;
LPWSTR phost;
int portVal = 0;
wchar_t *next_proxy = NULL;
list_item *proxy = NULL;
+ wchar_t* pos = NULL;
+
+ /* Filter based on the scheme, if there is one */
+ pos = wcschr(current_proxy, L'=');
+ if (pos) {
+ *pos = L'\0';
+ if (wcscmp(current_proxy, pproto) != 0) {
+ current_proxy = wcstok_s(NULL, separators, &context);
+ continue;
+ }
+ current_proxy = pos + 1;
+ }
/* Let's check for a scheme and ignore it. */
if ((phost = wcsstr(current_proxy, L"://")) != NULL) {
@@ -152,7 +166,7 @@
}
}
/* goto next proxy if available... */
- current_proxy = wcstok_s(NULL, L"; ", &context);
+ current_proxy = wcstok_s(NULL, separators, &context);
}
return nr_elems;
}
@@ -268,7 +282,6 @@
if (win_proxy != NULL) {
wchar_t *context = NULL;
int defport = 0;
- WCHAR pproto[MAX_STR_LEN];
int nr_elems = 0;
/**
@@ -290,10 +303,7 @@
goto noproxy;
}
- /* Prepare protocol string to compare with. */
- _snwprintf(pproto, sizeof(pproto), L"%s=", lpProto);
-
- nr_elems = createProxyList(win_proxy, pproto, &head);
+ nr_elems = createProxyList(win_proxy, lpProto, &head);
if (nr_elems != 0 && head != NULL) {
int index = 0;
proxy_array = (*env)->NewObjectArray(env, nr_elems,
proxy_class, NULL);
-Chris.
P.S. I will take a look at the latest webrev.
On 30/01/17 15:31, Zeller, Arno wrote:
Hi Chris,
thanks for your comments! I adjusted the change to contain your suggestions and
slightly modified
src/java.base/share/classes/java/net/doc-files/net-properties.html to refer to
macOS .
Also found some missing adjustments of the copyright year myself...
Some smaller comments and observations ( I'm still reviewing and testing ):
...
2) unix/native/libnet/DefaultProxySelector.c
You have changes to make g_strv_length available, but then
count the proxies in the list manually. Maybe just use
g_strv_length.
Removed reference to g_strv_length - I simply forgot to delete it after
changing my code to iterate over the list.
Nit: L 167 / 358 parameter indentation
Fixed.
3) windows/native/libnet/DefaultProxySelector.c
The HINTERNET session could be set lazily, for the case
where there are no auto proxy settings. Not sure if it
worth it though.
I would like to keep the current approach because it avoids the need of locking
to ensure that the initialization is
done only by one thread.
The indentation of the main body of XXX_getSystemProxies
could be reduced a little if:
if (WinHttpGetIEProxyConfigForCurrentUser(&ie_proxy_config) == FALSE)
return NULL;
Done.
4) macosx/native/libnet/DefaultProxySelector.c
The indentation of the main body of XXX_getSystemProxies
could be reduced a little if:
proxyDicRef = CFNetworkCopySystemProxySettings();
if (proxyDicRef == NULL)
return NULL;
Done.
5) I personally prefer the code of the old createProxy. Was there
specific reason why you changed it?
Sorry for this - another leftover from a former change. I reverted back to the
original.
6) Can you generally check the line lengths in all files, and try
where possible to keep it below 100, e.g. some comments could
easily be wrapped.
Done.
7) I would like to add jtreg tags to test/java/net/ProxySelector/
SystemProxies.java to have it run in any automated test systems.
There would be no failure mode of this test, as it requires
manually inspecting the output, but it would at least give some
coverage to this new code. Something like:
/*
* @test
* @bug 8170868
* @summary Basic test to provide some coverage of system proxy code.
Will
* always pass. Should be run manually for specific systems to inspect
output.
* @run/othervm -Djava.net.useSystemProxies=true SystemProxies
*/
Done.
The new webrev can be found at:
http://cr.openjdk.java.net/~clanger/webrevs/8170868.4/
Best regards,
Arno
