RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome

[Chris Hegarty]

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
>