Closed Bug 356831 Opened 18 years ago Closed 7 years ago

Proxy autodiscovery doesn't check DHCP (option 252)

Categories

(Core :: Networking: HTTP, defect, P3)

Product:
Core
Component:
Networking: HTTP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: flavour, Assigned: polly.shaw)

References

(Blocks 3 open bugs)

Details

(Keywords: qawanted, user-doc-needed, Whiteboard: [necko-backlog])

Attachments

(2 files, 14 obsolete files)

diff-2018-01-10.txt
71.86 KB, patch
Details | Diff | Splinter Review
Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252)
59 bytes, text/x-review-board-request
bagder
: review+
valentin
: review+
Details
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7 proxy autodiscovery can theoretically use either DNS or DHCP. DNS is working perfectly for me. DHCP is also working great for IE, but not for current Firefox (1.5.0.7). Wireshark suggests that it isn't even checking for this feature (it looks for WPAD via DNS & WINS broadcast, but no DHCP INFORM which is what IE uses) Reproducible: Always Steps to Reproduce: 1.Configure Network for DHCP auto-proxy detection 2.Configure Firefox to use Auto proxy detection 3.Launch Firefox Actual Results: No access to Internet (if direct access is blocked) Expected Results: Proxy access configured according to PAC file DHCP is setting the 252 option to: http://x.x.x.x:8080/wpad.dat (x.x.x.x is the IP of the Proxy server, port 80 is used by the proxy service, hence using 8080)
Component: Preferences → Networking: HTTP
Product: Firefox → Core
Version: unspecified → 1.8 Branch
QA Contact: preferences → networking.http
There is some code in attachment 121707 [details] [diff] [review] (see bug 28998), but it was never checked in.
Bug 444071 seems to be similar issue to this bug, and the bug opener of Bug 444071 says following in Bug 444071 Comment #8. > using "Work offline", immediately followed by "Work online" indeed solves the problem! To Fran Boon(bug opener): (same question as Bug 444071 Comment #6) Can following be a workaround when the problem occurs? "Work offline", then return to "Work online" again.
Jo: As I recall, the WPAD implementation ignored DHCP. Should we work on implementing that here? If so, lets make this NEW. Then we need to find out if the DHCP implementation is OS-specific. If so, then we should make this a meta bug, and then create one bug per-OS implementation. Fran: can you confirm the DHCP setting is changing (are you using ipconfig /all)?
Summary: Proxy autodiscovery doesn't check DHCP → Proxy autodiscovery doesn't check DHCP (option 252)
Hi, I have same problem. Can somebody repair it? In our company is it problem and colleagues must manual setup web proxy server, but after that doesnt take in matter internal address exceptions... Please look on it! Thank you so much!
(In reply to comment #6) > I have same problem. Can somebody repair it? > In our company is it problem and colleagues must manual setup web proxy server, > but after that doesnt take in matter internal address exceptions... The DHCP option will return the location of the PAC-file : your colleagues should use that same PAC file when configuring their proxy-server, in the field 'Automatic proxy configuration URL', *not* the ipaddress of the proxy-server in the manual configuration. The PAC-file also contains those internal address exceptions. If the PAC-file can be stored at the URL <http://wpad.example.com/wpad.dat> (replacing example.com with the domain of your company), then you can also use the 'Auto-detect proxy settings' field, and it's even easier (it's also supported by Internet Explorer). The DHCP option is mainly used when the PAC-file can't be found at that particular URL, and the user needs some help in the automatic configuration. Unless the whole URL is configured manually ofcourse. If you do not have a PAC-file at all, then you can't use the 'Automatic' or 'Auto-detect' settings at all, and neither can you use the DHCP option.
Thanks for answer! I will try it, but it is not so much usable for all peoples... in every location they must change their proxy settings (yeah i know that exist proxy "changers"..) Is really big problem to add this feature in Firefox?
btw: another problem is with Toolbars (icq, google, yahoo, so on...) They dont accept configuration scripts or manual configuration...
This is the same problem I have. I can successfully use WPAD and auto detect in IE using DHCP, but not in Firefox. Can this be looked at? Auto proxy should support both methods.
Base on the advice from Jo Hermans comment in above, I have tried this and put in comments in Google Chrome bug's report as it is a similar problem. https://code.google.com/p/chromium/issues/detail?id=18575#c5
At my company we use a multi-site, hierarchical web proxy model primarily for two purposes; first, to route users to a geographically local proxy for performance and bandwidth usage considerations, and second to even the load across our internet-connected web proxy devices. To accomplish this we utilize location-aware load balancing to direct users to a local web server to retrieve a location-specific version of the WPAD configuration file. This is a fairly complex configuration to maintain across >10 widely dispersed locations, and is flawed since the load balancing solution is limited in its ability to determine user locations (it is fairly broad). The proxy devices service both end-user web access and application web access, as well as B2B socks-based traffic. Legacy configuration on application servers make it difficult to reconfigure these servers to segregate user and application traffic on the proxy devices. We would like to use DHCP to replace our DNS-based solution for locating the WPAD configuration file, as all user desktops are configured to use DHCP. DHCP scopes are easily mapped to specific geographic locations which would enable more granular routing and load balancing. Unfortunately we cannot move forward with this solution because of this bug (and the same bug in Chrome). We would greatly appreciate this bug being resolved in a not-too-distant release. Thank you.
after more than 4 years still no answer about future implementation! would be nice to see the feature to get option 252 into firefox - DNS is sometimes tricky and not possible to server!
It is disappointing to see that there is no update on this issue! I am now force to block Firefox on all office PC (all 100+ of them) due to the fact that we are deploying proxy that is being deployed via DHCP. Sad so sad!
Same here. Have to block 150+ Firefox users because the DNS-way of delivering WPAD is not currently possible in my company and Firefox still can't get wpad via DHCP.
Bumping up the status, which is not a promise that it would be fixed (I don't have anything to say over that). I noticed that Chrome has finally implemented support for this (Windows only !!!). The hard part is to support it on all platforms.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to fyrch from comment #15) > Same here. Have to block 150+ Firefox users because the DNS-way of > delivering WPAD is not currently possible in my company and Firefox still > can't get wpad via DHCP. The same for us. We can't use dns way because of company restriction.
Still no progress on this issue ? Resolving this issue is crucial for corporate users.
Blocks: 350759
I'm too lazy to file a new bug and I just wanted to mention that after getting the PAC settings in network1, putting the computer in standby then waking it up in a PAC-free network2, firefox insists on using the other networks' proxy (which is unreachable, thus displaying the message that it cannot connect to proxy), even if I explicitly perform a dhcp release/renew. It only resumes working if I restart it. firefox 12 beta, windows xp sp3
firefox version 11 and 12.0 does not receive Proxy auto-config file by DHCP 252
(In reply to bibl2008 from comment #20) > firefox version 11 and 12.0 does not receive Proxy auto-config file by DHCP > 252 What about version 13? Does anyone know if Mozilla is about to fix it? Using proxy through auto-config file by DHCP is the only reasonable solution for companies using DirectAccess.
I'm using 14 beta and it still doesn't implement DHCP for WPAD. This is a particular issue for us because our company is split between multiple geographical locations each with its own proxy server, but using a single DNS name for the internal domain. This makes using wpad.domain.com more difficult. However, we already have the proxy info configured via DHCP for use by IE and Chrome. If Firefox would support DHCP for proxy auto-detect, it would allow the browser to integrate into existing networks far easier and without having to manually configure each one to point to the .pac file.
Could anyone confirm version 17.0 (ESR) has fixed the lack of support for DHCP option 252?
No patch has been checked in (or even proposed), so how could it have been fixed ?
(In reply to Daniel Veditz [:dveditz] from comment #25) > Equivalent Chromium bug (now fixed, on windows at least) is > https://code.google.com/p/chromium/issues/detail?id=18575 Seems like Firefox is the only important browser left that still won't support this feature. Is there a plan to finally assign this to someone? This would be crucial to rollout Firefox in corporate networks!
OS: Windows XP → All
Hardware: x86 → All
Version: 1.8 Branch → Trunk
Any chances that this bug got some attention in near future?
The is a great increase in users demands for this feature inside vpn private networks. DNS Query for WPAD should not be the first inly thing to do for AutoDetect Proxy settings. Sometimes DNS Query for WPAD is not possible, because the pcs are not is a domain, or dns do not know the entry, or simply some routers does not relay the request DHCP Comming first, before DNS, Firefox should check for option 252, in the dhcp settings, by requesting windows the DhcpRequestParams command. If windows has not requested the option252, a dhcpinform should be submitted to get the option. This is really important for trusted dhcp servers in private secured networks. If of course firefox do not get the option 252 from windows, it should fallback to DNS WPAD METHOD. But first we would like Firefox to implement correctly the DHCP WPAD METHOD Thanks
I have to say 'bye-bye' to firefox. I switched the browser of over 300 PCs in my corporate environment to Google Chrome and very satisfied with the outcome. I'm tired of the undefined waiting for this bug to be settled.
When this 7 years old bug will be fixed ? I use DHCP WPAD in a school environment with more than 500 computers, I would be glad to have this feature working on Firefox...
we exploit a network of ~1000 computers, the most part from which has firefox. It's really painfull for me, as software administrator, to maintain dns-wpad. Please, please, fix it BUG and add dhcp-wpad method!
As another large corporation with tens thousands of computers, this is stopping us from implementing much more elegant and efficient solutions. We are considering de-supporting firefox for our user base because of this... PLEASE fix, or at least get some traction! Thanks.
Flags: needinfo?(nobody)
Firefox is not usable in corporate environment, forget about it and use IE
Flags: needinfo?(nobody)
(In reply to comment #31 and #32) There is 2 solutions to meet your request : Either you paid an expert to path Firefox or your change the organization of your corporate network. ;) If no company pays to change Firefox, then Firefox will not be able to use in business.
Option 252 is for reserved private use: http://www.iana.org/assignments/bootp-dhcp-parameters/bootp-dhcp-parameters.xhtml It's Microsoft choice, http://superuser.com/questions/382964/dhcp-option-252-what-is-it Unfortunatelly, DHCP hasn't an standard option for indicating de proxy. DNS discovery must be used if you want to find the proxy: http://findproxyforurl.com/wpad-introduction/
Whiteboard: [necko-backlog]
I am thinking of working on this as it would help me out a lot. The link at the bottom of Josep's post now mentions DHCP.
Attached patch diff.txt (obsolete) — — Splinter Review
The approach I have tried follows the pattern of SystemProxySettings - I created an interface called nsIDHCPClient, implemented it for Windows, and injected the class into nsPACMan. I've used the PAC Thread for querying DHCP. I need guidance on testing this because I haven't written any automated tests.
Attachment #8867355 - Flags: feedback?(zxcvbnm)
Attachment #8867355 - Flags: feedback?(zxcvbnm) → feedback?(daniel)
Attachment #8867355 - Flags: feedback?(mcmanus)
Attached patch diff.txt (obsolete) — — Splinter Review
I updated the patch because the previous one was in the wrong direction (i.e. how to undo my changes to make it the same as trunk.)
Assignee: nobody → polly.shaw
Attachment #8867355 - Attachment is obsolete: true
Attachment #8867355 - Flags: feedback?(mcmanus)
Attachment #8867355 - Flags: feedback?(daniel)
Attachment #8867481 - Flags: feedback?(mcmanus)
Attachment #8867481 - Flags: feedback?(daniel)
Comment on attachment 8867481 [details] [diff] [review] diff.txt Review of attachment 8867481 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your contribution and work on this patch! It seems to be the right direction and is certainly a very good start, but I'll admit I got a bit distracted when I noticed it had a fairly large amount of white space changes that should be avoided and would make the patch smaller and easier to review if they wouldn't be there. The other main concern I have is the memory copies done at a few places without properly checking that the data fits in the destination buffer. ::: netwerk/base/nsIDHCPClient.idl @@ +10,5 @@ > + */ > +[scriptable, uuid(aee75dc0-be1a-46b9-9e0c-31a6899c175c)] > +interface nsIDHCPClient : nsISupports > +{ > + Please avoid adding trailing white space. ::: netwerk/base/nsPACMan.cpp @@ +223,5 @@ > mSetupPACData.Assign(text, datalen); > mSetupPACURI = pacURI; > } > > + void ConfigureWPAD(){ Nit: the placement of the open brace when declaring a function/method should be in the first column. This mistake occurs on several places. @@ +423,5 @@ > return NS_OK; > } > > nsresult > +nsPACMan::LoadPACFromURI(const nsCString &spec, bool forceReload, bool calledFromScheduledReload) "called from" ? Isn't the option simply "scheduled reload" ? What's the forceReload option for and how do these two differ from each other? @@ +466,5 @@ > +nsresult > +nsPACMan::GetPACFromDHCP(nsCString &pacSpec) > +{ > + if (mDHCPClient){ > + return mDHCPClient->GetOption(252, pacSpec); 252 like this is very magic. I'd prefer a define/const or a very clear comment explaining what this value comes from and means. @@ +480,5 @@ > + GetPACFromDHCP(mPACThreadURISpec); > + > + if (mPACThreadURISpec.IsEmpty()) { > + mPACThreadURISpec.AssignLiteral(WPAD_URL); > + } I can see how a LOG() could be great here for future debugging and understanding users' situations, probably for both branches in this conditional. @@ +486,5 @@ > +} > + > +void nsPACMan::CopyPACURIFromPACThread(){ > + MOZ_ASSERT(NS_IsMainThread(), "wrong thread"); > + mPACURISpec.Assign(mPACThreadURISpec); Is this a thread-safe manipulation? @@ +768,5 @@ > // Even if the PAC file could not be parsed, we did succeed in loading the > // data for it. > mLoadFailureCount = 0; > } else { > + trailing white space ::: netwerk/base/nsProtocolProxyService.cpp @@ +1026,4 @@ > nsresult rv; > if (mSystemProxySettings && > + NS_SUCCEEDED(mSystemProxySettings->GetMainThreadOnly(&mainThreadOnlyForSystemProxySettings)) && > + !mainThreadOnlyForSystemProxySettings) { This name change seems unrelated, and I think the new much longer name makes the code harder to read. If you think we need to explain that this section is only for system proxy settings, then add a comment. ::: netwerk/base/nsProtocolProxyService.h @@ +300,5 @@ > nsresult SetupPACThread(); > nsresult ResetPACThread(); > nsresult ReloadNetworkPAC(); > > + white space only change! ::: toolkit/system/windowsDHCPClient/DHCPUtils.cpp @@ +6,5 @@ > +#include <memory> > +#include "mozilla\Logging.h" > + > + > +#define WORKING_BUFFER_SIZE 15000 Perhaps a mention what buffer this is for? @@ +7,5 @@ > +#include "mozilla\Logging.h" > + > + > +#define WORKING_BUFFER_SIZE 15000 > +#define MAX_TRIES 3 Perhaps a mention what max tries is for? @@ +14,5 @@ > +#pragma comment(lib, "dhcpcsvc.lib" ) > + > +mozilla::LazyLogModule gDhcpLog("dhcp"); > + > +bool IsCurrentAndHasDHCP(PIP_ADAPTER_ADDRESSES pAddresses) { nit: wrong open brace placements for function declarations in this file too. You should also put the return type on the line above the name. This goes for several other function declarations. @@ +38,5 @@ > + > + PIP_ADAPTER_ADDRESSES pCurrAddresses = nullptr; > + > + > + memset(networkAdapterName, 0, NETWORK_ADAPTER_NAME_LENGTH + 1); Why memset the whole buffer? Isn't it enough to zero the first byte? @@ +63,5 @@ > + } > + > + iterations++; > + > + } while ((dwRetVal == ERROR_BUFFER_OVERFLOW) && (iterations < MAX_TRIES)); Can this reasonably ever loop more than one extra lap? Why is MAX_TRIES 3? Also, the check for "dwRetVal == ERROR_BUFFER_OVERFLOW" seems superfluous in the while() expression since that will always match due to the check above. @@ +72,5 @@ > + bool found = false; > + pCurrAddresses = pAddresses; > + while (pCurrAddresses) { > + if (IsCurrentAndHasDHCP(pCurrAddresses)) { > + memcpy(networkAdapterName, pCurrAddresses->AdapterName, strlen(pCurrAddresses->AdapterName) + 1); scary memcpy. Can this be done in a way that makes it more certain that the destination has room for the copied data? I suggested the method also accepts a target name buffer size and then add a run-time check that the data fits. @@ +103,5 @@ > + uint8_t option > +) > +{ > + DWORD dwRetVal, dwSize; > + CHAR TmpBuffer[100]; // option result is limted to 256 characters; not sure how much else is needed for 100 or 256. Which is it? Is it supposed to be 0x100 ? @@ +137,5 @@ > + ); > + > + switch (dwRetVal){ > + case NO_ERROR: > + if (DhcpRequestedOptionParams.nBytesData==0) nit: add spaces around '==' @@ +138,5 @@ > + > + switch (dwRetVal){ > + case NO_ERROR: > + if (DhcpRequestedOptionParams.nBytesData==0) > + return NS_ERROR_NOT_AVAILABLE; place add { braces } @@ +143,5 @@ > + > + CopyMemory( > + pszOptionValueBuf, DhcpRequestedOptionParams.Data, > + DhcpRequestedOptionParams.nBytesData > + ); Why the funny indentation here? Is there a reason this uses CopyMemory() while you used memcpy() before? There's no precaution done that the copied data actually fits in the provided target buffer. I propose you pass in a buffer size as well for this purpose. ::: toolkit/system/windowsDHCPClient/DHCPUtils.h @@ +9,5 @@ > +nsresult GetActiveDHCPNetworkAdapterName(char* networkAdapterName); > + > +nsresult RetrieveOption( > + LPWSTR pszAdapterName, > + char pszHostNameBuf[257], Please explain the chosen maximum size in a comment. @@ +10,5 @@ > + > +nsresult RetrieveOption( > + LPWSTR pszAdapterName, > + char pszHostNameBuf[257], > + uint8_t option nit: wrong indentation ::: toolkit/system/windowsDHCPClient/nsWindowsDHCPClient.cpp @@ +48,5 @@ > + > + char optionValue[MAX_OPTION_LENGTH + 1]; > + size_t retVal; > + wchar_t wideNetworkAdapterName[(NETWORK_ADAPTER_NAME_LENGTH + 1) * 2]; > + mbstowcs_s(&retVal, wideNetworkAdapterName, strlen(networkAdapterName) * 2, networkAdapterName, strlen(networkAdapterName)); Please make precautions that this can't overflow the target buffer. ::: toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp @@ +23,5 @@ > class nsWindowsSystemProxySettings final : public nsISystemProxySettings > { > public: > + NS_DECL_THREADSAFE_ISUPPORTS > + NS_DECL_NSISYSTEMPROXYSETTINGS Pretty please do not do (white space) changes to unrelated code! @@ +75,5 @@ > + InternetGetConnectedStateExW(&connFlags, connName, > + mozilla::ArrayLength(connName), 0); > + } MOZ_SEH_EXCEPT(EXCEPTION_EXECUTE_HANDLER) { > + return NS_ERROR_FAILURE; > + } The indent levels in this code turned funny. They are 4 spaces since before for each level so please keep to that! Also, this is more unrelated white space changes that make this patch larger and harder to review.
Attachment #8867481 - Flags: feedback?(daniel) → feedback-
Comment on attachment 8867481 [details] [diff] [review] diff.txt this is daniel's area of expertise, so I'm just going to defer..
Attachment #8867481 - Flags: feedback?(mcmanus)
Attached patch diff.txt (obsolete) — — Splinter Review
Hi Badger - I've addressed your concerns from the last review (I think), made an effort with complying with the coding guide, and written some gtests. Also I put braces round the single-line conditionals & whiles in nsProtocolProxyService.cpp but if this makes reviewing more difficult I can submit a patch without these changes. (There are also some logic changes in this class, so I can see that it might obscure them.)
Attachment #8867481 - Attachment is obsolete: true
Attachment #8884649 - Flags: review?(daniel)
Attached patch diff.txt (obsolete) — — Splinter Review
On reviewing the patch in the browser, I noticed that I had removed a couple of lines from nsProtocolProxyService.cpp which I shouldn't, also that I needed to move a comment from this class to nsPACMan. also sorry for calling you badger rather than bagder in the last comment.
Attachment #8884649 - Attachment is obsolete: true
Attachment #8884649 - Flags: review?(daniel)
Attachment #8884670 - Flags: review?(daniel)
Attached patch diff.txt (obsolete) — — Splinter Review
fixed error in reload conditional in nsProtocolProxyService. Also removed a local fix for a problem in windows.configure that was causing the build to fail on my machine (I haven't looked into this deeply enough to work out whether this is likely to be affecting anyone else.)
Attachment #8884670 - Attachment is obsolete: true
Attachment #8884670 - Flags: review?(daniel)
Attachment #8884679 - Flags: review?(daniel)
review ping?
Flags: needinfo?(daniel)
Flags: needinfo?(Daniel.Stenberg)
review ping? Sorry to keep on needling but I wonder if Daniel is away, so I am asking Patrick.
review ping? Sorry to keep on needling but I wonder if Daniel is away, so I am asking Patrick.
Flags: needinfo?(mcmanus)
Attached patch diff.txt (obsolete) — — Splinter Review
Updated to use std::vector for char arrays rather than manually allocating memory.
Attachment #8884679 - Attachment is obsolete: true
Attachment #8884679 - Flags: review?(daniel)
Attachment #8886823 - Flags: review?(daniel)
polly, if this were a short patch I would try and review it myself.. but there's quite a bit of pac specific code in there and daniel is maintaining that right now.. so I think its in the best interest of firefox to wait for him to return thanks for the patch and your patience
Flags: needinfo?(mcmanus)
Attached patch diff 2017-07-31.txt (obsolete) — — Splinter Review
Tidying up format of by-reference arguments; also edited MockWindowsNetworkFunctions so that they use provided buffer for GetOption.
Attachment #8886823 - Attachment is obsolete: true
Attachment #8886823 - Flags: review?(daniel)
Attachment #8891973 - Flags: review?(daniel)
It seems you put the same patch twice in that diff file, is that correct? Did you run it on try? (Or should I do that for you?)
Flags: needinfo?(Daniel.Stenberg)
Additionally, you've changed *a lot* of braces only in this patch, which significantly increases the size of the patch and makes it a lot harder to review only the functional changes. It is MUCH better for reviewers if you would avoid doing that.
Attached patch patch with nsprotocolproxy brace-fixes removed. (obsolete) — — Splinter Review
Yes, sorry the last diff was a double diff (I must have used '>>' rather than '>')I have removed fixes for braces in nsProtocolProxyService. No, I haven't used the try server - I'm not sure I've got permission.
Attachment #8891973 - Attachment is obsolete: true
Attachment #8891973 - Flags: review?(daniel)
Flags: needinfo?(daniel)
Attachment #8893772 - Flags: review?(daniel)
Attached patch diff-2017-08-17.txt (obsolete) — — Splinter Review
Hi Daniel (and Patrick), thanks for vouching for me for try-server access. I have now `mach try`d (for the latest, see https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0cd0a395f7e9dc670c2719359d0c32ee67e5025), and I have seen a bit of red and orange, but I'm not sure if this should bother me, as the errors are generally related to things which wouldn't be affected by this change. I don't have the experience to know whether everything should be 'green', although the issues are in general flagged with 'intermittent' messages, and links to bugzilla. Having said that, the debug builds did initially break on the try server due to three unit tests written by me, and so I have removed those, which is why I'm submitting another patch for review.
Attachment #8893772 - Attachment is obsolete: true
Attachment #8893772 - Flags: review?(daniel)
Attachment #8898464 - Flags: review?(daniel)
Polly, you can generally be fairly sure that the reds are yours to fix since the builds should not cause reds. You can also compare with for example the latest code (https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound) to see how it performs to get a feel for what in your build that stands out. This said, the red Windows builds you got here seems to be Bug 1385053 and I *think* that implies that you should select another set of targets in trychooser (Bug 1384706). The orange intermittent builds are the tough ones to spot if they're a concern or not.
Comment on attachment 8898464 [details] [diff] [review] diff-2017-08-17.txt Review of attachment 8898464 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks really good now. Due to the huge size of the patch I think I'll appreciate a second set of reviewer eyes on this so I'm also asking Valentin to take a look.
Attachment #8898464 - Flags: review?(valentin.gosu)
Attachment #8898464 - Flags: review?(daniel)
Attachment #8898464 - Flags: review+
Comment on attachment 8898464 [details] [diff] [review] diff-2017-08-17.txt Review of attachment 8898464 [details] [diff] [review]: ----------------------------------------------------------------- I have yet to thoroughly review the most critical parts of the patch, but overall it looks good. Keeping the r? to do another pass. In the meantime I have a bunch of styling issues, and a proposal to add a pref for this feature. --- Could you also add a descriptive comment to the patch? The usual format is: ``` Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) r=bagder,valentin <insert long description here> You should mention largely what the problem is, what the patch does, and how it does it. One should be able to understand why the patch was needed without reading the bug. ``` Also, if Daniel agrees, I would love to see this feature have a pref to turn it off in case we discover a bug. ::: netwerk/base/nsPACMan.cpp @@ +481,5 @@ > nsCOMPtr<nsIStreamLoader> loader = > do_CreateInstance(NS_STREAMLOADER_CONTRACTID); > NS_ENSURE_STATE(loader); > > + LOG(("nsPACMan::LoadPACFromURI %s\n", aSpec.get())); also log aIsScheduledReload - might prove to be useful. @@ +519,5 @@ > +nsresult > +nsPACMan::GetPACFromDHCP(nsCString &aSpec) > +{ > + MOZ_ASSERT(!NS_IsMainThread(), "wrong thread"); > + if (mDHCPClient) { nit: it would make it a bit better if you reversed the error handling: if (!mDHCPClient) return error... then do stuff :) @@ +841,4 @@ > // data for it. > mLoadFailureCount = 0; > } else { > + nit: no whitespace-only changes please ::: netwerk/base/nsPACMan.h @@ +238,4 @@ > > private: > ProxyAutoConfig mPAC; > + nsCOMPtr<nsIThread> mPACThread; nit: no whitespace-only changes please ::: netwerk/base/nsProtocolProxyService.cpp @@ +223,4 @@ > bool pacAvailable = true; > if (mStatus == NS_ERROR_NOT_AVAILABLE && !mProxyInfo) { > // If the PAC service is not avail (e.g. failed pac load > + // or shutdown) then we will be go ing direct. Make that nit: extra space "go ing" - please remove @@ +715,4 @@ > reloadPAC = true; > } > > + nit: no whitespace-only changes please @@ +812,5 @@ > // Get System Proxy settings if available > AsyncConfigureFromPAC(false, false); > } > + if (!tempString.IsEmpty() || mProxyConfig == PROXYCONFIG_WPAD) > + ConfigureFromPAC(tempString, true); nit: add curly-braces around this {} @@ +1181,4 @@ > return SetupPACThread(); > } > > + nit: no whitespace-only changes please @@ +1194,5 @@ > + ( > + (!autodetect && mPACMan->IsPACURI(spec)) > + || (autodetect && mPACMan->IsUsingWPAD()) > + ) > + ) { nit: very odd indentation. Try this: + if (!forceReload && ((!autodetect && mPACMan->IsPACURI(spec)) || + (autodetect && mPACMan->IsUsingWPAD()))) { @@ +1203,5 @@ > > return mPACMan->LoadPACFromURI(spec); > } > > + nit: no whitespace-only changes please ::: toolkit/system/windowsDHCPClient/DHCPUtils.cpp @@ +221,5 @@ > +{ > + > + std::vector<wchar_t> wideNetworkAdapterName(aAdapterName.Length() + 1); > + > + nsresult rv = ConvertNetworkAdapterNameToWideString( I wonder if we can use the nsString class instead of doing the conversion manually? https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#UTF-8_UTF-16_conversion
Comment on attachment 8898464 [details] [diff] [review] diff-2017-08-17.txt Review of attachment 8898464 [details] [diff] [review]: ----------------------------------------------------------------- Overall it looks pretty good. I have a few more minor nits, nothing major. The fact that the patch includes tests is just fantastic. My only suggestion is that we add the ability to pref off this feature, as mentioned in my previous comment. ::: netwerk/base/nsPACMan.cpp @@ +293,5 @@ > return NS_OK; > } > > + if (mConfigureWPAD) { > + nsCString spec; nit: nsAutoCString ::: netwerk/base/nsPACMan.h @@ +132,4 @@ > * The non normalized uri spec of this URI used for comparison with > * system proxy settings to determine if the PAC uri has changed. > */ > + nsresult LoadPACFromURI(const nsCString &aSpec, bool aIsScheduledReload = false); make the first argument const nsACString &aSpec This way we can used nsAutoCString at the call site. @@ +232,5 @@ > void PostCancelPendingQ(nsresult); > bool ProcessPending(); > + nsresult GetPACFromDHCP(nsCString &aSpec); > +public: > + nsresult ConfigureWPAD(nsCString &aSpec); nit: const nsACString & for all of these and all other methods that take a string type ::: toolkit/system/windowsDHCPClient/DHCPUtils.h @@ +17,5 @@ > +namespace windowsDHCPClient { > + > +nsresult GetActiveDHCPNetworkAdapterName( > + nsCString& aNetworkAdapterName, > + RefPtr<WindowsNetworkFunctionsWrapper> aWindowsNetworkFunctionsWrapper); nit: this method could take a WindowsNetworkFunctionsWrapper* (not a refPtr) to prevent unnecessary addRefing @@ +24,5 @@ > + const nsCString& aAdapterName, > + uint8_t aOption, > + std::vector<char>& aOptionValueBuf, > + uint32_t* aOptionSize, > + RefPtr<WindowsNetworkFunctionsWrapper> aWindowsNetworkFunctionsWrapper same here
Attachment #8898464 - Flags: review?(valentin.gosu) → review+
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Hi Valentin, I'm just looking at putting this behind a pref, and I was wondering which bit of the proxy auto-detect should be affected by the pref. Specifically, in order to put the potentially thread-blocking DHCP query on a background thread, I made the whole of ConfigureWPAD be called asynchronously. Should the preference (if switched off) simply remove the DHCP query, or should it revert to the previous synchronous behaviour? Code-wise, it is simpler just to keep the call asynchronous, even when the DHCP is turned off or not available, but perhaps this doesn't provide you with the assurance you need about being able to toggle the feature off if there are bugs. The method is question is: nsresult nsPACMan::ConfigureWPAD(nsACString &aSpec) { MOZ_ASSERT(!NS_IsMainThread(), "wrong thread"); aSpec.Truncate(); GetPACFromDHCP(aSpec); if (aSpec.IsEmpty()) { // We diverge from the WPAD spec here in that we don't walk the // hosts's FQDN, stripping components until we hit a TLD. Doing so // is dangerous in the face of an incomplete list of TLDs, and TLDs // get added over time. We could consider doing only a single // substitution of the first component, if that proves to help // compatibility. aSpec.AssignLiteral(MOZ_WPAD_URL); } return NS_OK; } There is nothing in it which prevents it being called synchronously except the blocking GetPACFromDHCP call and the debug assert. I could change this assertion to be MOZ_ASSERT(!NS_IsMainThread() || !mDHCPClient, "wrong thread"); and call the method synchronously if mDHCPClient was null.
Flags: needinfo?(valentin.gosu)
(In reply to polly.shaw from comment #60) > Hi Valentin, > > I'm just looking at putting this behind a pref, and I was wondering which > bit of the proxy auto-detect should be affected by the pref. Specifically, > in order to put the potentially thread-blocking DHCP query on a background > thread, I made the whole of ConfigureWPAD be called asynchronously. Should > the preference (if switched off) simply remove the DHCP query, or should it > revert to the previous synchronous behaviour? I meant that we should only set mConfigureWPAD/mAutoDetect to true if the pref is enabled. > There is nothing in it which prevents it being called synchronously except > the blocking GetPACFromDHCP call and the debug assert. I could change this > assertion to be > > MOZ_ASSERT(!NS_IsMainThread() || !mDHCPClient, "wrong thread"); I think it's better just to make sure we just don't call it on the main thread. It's OK in its current form, but if you want to make sure it's never executed on the MT, you could move GetPACFromDHCP or ConfigureWPAD to be a method of ExecutePACThreadAction.
Flags: needinfo?(valentin.gosu)
Attached patch Response to feedback from Valentin (obsolete) — — Splinter Review
Feedback implemented, with the exception of the pref, which I left because we hadn't had a response from Daniel and also I think the patch description might help clarify the question over what exactly should be toggled off by the pref.
Attachment #8898464 - Attachment is obsolete: true
Attachment #8913162 - Flags: review?(valentin.gosu)
Thanks for the patch. This is awesome. I'll take another look at it tonight, and I'll add in the pref for you. We'll probably be able to land the patch next week. Thanks a lot for the great work!
Attached patch Add pref for WPAD proxy (obsolete) — — Splinter Review
MozReview-Commit-ID: DMfhpJlHoAR
Attachment #8915388 - Flags: review?(daniel)
Comment on attachment 8913162 [details] [diff] [review] Response to feedback from Valentin Review of attachment 8913162 [details] [diff] [review]: ----------------------------------------------------------------- There are a couple of remaining issues. I also uploaded a patch with the pref. Please check that proxy detection still works with the pref on and off. Thanks! ::: netwerk/base/nsPACMan.cpp @@ +485,5 @@ > nsCOMPtr<nsIStreamLoader> loader = > do_CreateInstance(NS_STREAMLOADER_CONTRACTID); > NS_ENSURE_STATE(loader); > > + LOG(("nsPACMan::LoadPACFromURI aspec: %s, aIsScheduledReload:%s\n", aSpec, aIsScheduledReload? "true" : "false")); use aSpec.BeginReading() and also change aspec to aSpec @@ +532,5 @@ > + rv = mDHCPClient->GetOption(MOZ_DHCP_WPAD_OPTION, aSpec); > + if (NS_FAILED(rv)) { > + LOG(("nsPACMan::GetPACFromDHCP DHCP option %d query failed with result %d\n", MOZ_DHCP_WPAD_OPTION, rv)); > + } else { > + LOG(("nsPACMan::GetPACFromDHCP DHCP option %d query succeeded, finding PAC URL %s\n", MOZ_DHCP_WPAD_OPTION, aSpec)); use aSpec.BeginReading() ::: netwerk/base/nsPACMan.h @@ +232,5 @@ > void PostCancelPendingQ(nsresult); > bool ProcessPending(); > + nsresult GetPACFromDHCP(nsACString &aSpec); > +public: > + nsresult ConfigureWPAD(nsACString &aSpec); Make this private and add ExecutePACThreadAction to the list of friend classes. ::: netwerk/test/gtest/TestPACMan.cpp @@ +129,5 @@ > +TEST_F(TestPACMan, CreateDHCPClient) { > + > + SetOptionResult(TEST_WPAD_DHCP_OPTION); > + nsCString spec; > + mPACMan->mDHCPClient->GetOption(252, spec); I tried to run this on Linux, and the test crashed on this line. Can you do a try push again, to make sure all the tests pass on all platforms?
Attachment #8913162 - Flags: review?(valentin.gosu)
Attachment #8915388 - Flags: review?(daniel) → review+
Hi Valentin, Sorry to have been silent for a while. I've imported your pref patch and when it is set to 'true' everything still works on a network with proxy only available by DHCP. When the pref is set to 'false', the effect is that in the 'Connections Settings' dialog, the 'Auto-detect proxy settings for this network' option is same as the 'No proxy' option. Does this sound right to you? I haven't had a chance to look at the Linux issue but I'll do that tomorrow - maybe I left a reference to a Windows header lying around somewhere.
Attached patch diff-2018-01-10.txt — — Splinter Review
Sorry about the long delay in this: it was because I had a new computer and a new job, and it was actually quite hard problem to solve with the Linux tests (Linux doesn't load the registered classes in the same way so I had to load them programmatically.) Also I have removed the `#define private public` hack in the tests and instead made the friend test classes friends of nsPACMan - hope that's better. Also I have implemented @valentin's last bit of feedback. I did have a go at using Review Board to submit this patch, but I got an error message (we do not allow replacements to modify files) and then decided to go down this old route instead for now but I'll simultaneously look into what's wrong with my submission via Mercurial.
Attachment #8913162 - Attachment is obsolete: true
Attachment #8915388 - Attachment is obsolete: true
Attachment #8941630 - Flags: review?(valentin.gosu)
I've now managed to submit the patch to Review Board, and it has been assigned (automatically) to Daniel and Valentin. I think it failed to submit because I had merge commits in my history, so I re-merged the diff to a new branch and submitted that. (Which unfortunately has meant that it has lost all the history. If it's too difficult to extract the new stuff, I could maybe try creating a review request which has two commits, one for the last patch which Valentin has already reviewed, and an additional one for the changes I've made since then. Let me know if this would help.
Attachment #8942435 - Attachment is obsolete: true
Attachment #8942435 - Flags: review?(valentin.gosu)
Attachment #8942435 - Flags: review?(daniel)
I haven't finished the review, but before we land this we should consider the security implications of this feature. I recently came across this article: https://googleprojectzero.blogspot.se/2017/12/apacolypse-now-exploiting-windows-10-in_18.html If we decide to land this, is should probably be preffed off by default.
Thanks for getting back to me Valentin! If we decide to land this and pref it off, we'll need to move the pref, because as it currently stands, the the pref also turns off the pre-existing feature of WPAD by DNS (as opposed to DHCP), which is currently used by anyone who selects `Auto-detect proxy settings for this network` in the Connection Settings Options dialog. Do you think I should work on making the new pref apply only to the DHCP query part? <-- this question is the reason why I've selected 'Need more information from' :) When I first submitted this bug I emailed Patrick McManus directly about it, and he did warn me about the security issue. I hope he doesn't mind me quoting from his email but Daniel was also CC'd so I don't think it was private. > 1] it should require explicit config opt in like the dns version already does - the system default isn't enough for security > reasons. (but it is no worse than the dns version - so the same policies can apply). I don't think you're going to run > afoul of this - but just mentioning it So in the implementation that I've submitted for review, DHCP query is turned on by the same settings as the DNS query (which are not the default settings), and cannot be turned on implicitly by choosing 'Use system proxy settings', which appears to be the default setting. Incidentally it was interesting to read that Chrome's implementation of WPAD was not viewed as a threat by the authors of the blog post because the JavaScript is evaluated in a sandbox. I had a quick look at how it is executed in Firefox, and it seems to use the standard engine (whose attack surface is presumably well scrutinised) on the proxy thread; quoting a comment in 'ProxyAutoConfig.cpp': ``` // JSContextWrapper is a c++ object that manages the context for the JS engine // used on the PAC thread. It is initialized and destroyed on the PAC thread. ``` But I guess, even though the JavaScript engine tries very hard to protect the user from unauthorised reads and privilege escalations for all sorts of reasons beyond PAC, PAC is a special threat, as it can direct the user to an evil proxy server which can serve up all sorts of attacks (though increasing use of HTTPS mitigates this.)
Flags: needinfo?(valentin.gosu)
currently wpad (dns only) is only accessible via the "auto detect proxy setting". Using the system proxy settings does not enable wpad even if the system is configured to do so. This is intentional because we understood the project zero blog post a long time ago :) This patch must not change that behavior. Its fine to do dhcp along with DNS when we're doing wpad, but we're not going to do wpad under any new circumstances. wpad really is awful. valentin please take note :)
(In reply to polly.shaw from comment #72) > Thanks for getting back to me Valentin! > > If we decide to land this and pref it off, we'll need to move the pref, > because as it currently stands, the the pref also turns off the pre-existing > feature of WPAD by DNS (as opposed to DHCP), which is currently used by > anyone who selects `Auto-detect proxy settings for this network` in the > Connection Settings Options dialog. Do you think I should work on making the > new pref apply only to the DHCP query part? <-- this question is the reason > why I've selected 'Need more information from' :) Hi Polly, thanks for all the work. Yes, since we alredy have a pref/UI to enable proxy autodiscovery, I think the new pref should control only WPAD by DHCP. This is so that in case there is a bug in this patch, we can have the users change the pref (or push a hot patch to do that), and not break WPAD by DNS as well.
Flags: needinfo?(valentin.gosu)
Attachment #8942520 - Flags: review?(valentin.gosu)
Attachment #8941630 - Flags: review?(valentin.gosu)
Attachment #8952551 - Flags: review?(valentin.gosu)
Attachment #8952551 - Flags: review?(daniel)
Comment on attachment 8952551 [details] Bug 356831 - Changed pref to be auto-detect via DHCP, not whole of WPAD https://reviewboard.mozilla.org/r/221786/#review228366
Attachment #8952551 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review228368 Thanks for updating the patch. This looks good. Only a minor nit. ::: netwerk/test/gtest/TestPACMan.cpp:264 (Diff revision 2) > + ProcessAllEventsTenTimes(); > + > + mPACMan->LoadPACFromURI(EmptyCString(), true); > + > + ProcessAllEventsTenTimes(); > + Multiple trailing whitespace instances in this file.
Attachment #8942520 - Flags: review?(valentin.gosu) → review+
Polly, I talked with Patrick about the patch for a bit, there are a few other touches he suggested. First of all, could you merge all the patches? It would make it easier to follow. Second of all, could we MOZ_RELEASE_ASSERT in nsPACMan::ConfigureWPAD that the mProxyConfig == PROXYCONFIG_WPAD ? We want to make sure we never ever use wpad unless the pref is set correctly.
Flags: needinfo?(polly.shaw)
Attachment #8952551 - Attachment is obsolete: true
Attachment #8952551 - Flags: review?(daniel)
Attachment #8953235 - Attachment is obsolete: true
@valentin I have submitted another patch. I had to re-read the pref within nsPACMan as it didn't have access to it from nsProtocolProxyService, which doesn't sound like it's quite what you intended - is it OK? Other prefs are read within the class so it wasn't a *very* radical thing. Also I set the pref in the set-up for the unit tests to stop them from crashing as I was running them in non-debug mode (and I think the ability to be able to run the unit tests in non-debug mode is quite useful.)
Flags: needinfo?(polly.shaw)
@valentin the patch is failing on debug unit tests so ignore for now - I'll take another look at this & submit another patch.
The reason for the test failures was an assertion that the prefs should not be read other than on the main thread, but the proxy auto-detection takes place on the PAC thread. So I have moved the pref-reading to the code which triggers ConfigureWPAD, and put the pref into a field called mProxyConfigType. Then ConfigureWPAD checks that the value of mProxyConfigType is appropriate.
Hi Valentin, I've just put a need info on this ticket because I'm not sure whether the workflow I used in review board (submitting a review, then assigning in it) actually notified you about anything.
Flags: needinfo?(valentin.gosu)
Attachment #8942520 - Flags: review?(daniel)
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review234698 This looks really good to me. I've asked Daniel to take another quick looks at this, and hopefully we can then land it. Thanks! ::: commit-message-d97ff:1 (Diff revisions 2 - 4) > -Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) r=bagder,valentin > +Bug 356831 Leave the description where it was. ::: netwerk/test/gtest/TestPACMan.cpp:26 (Diff revisions 2 - 4) > + nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID); > + > + if (!prefs) { > + return NS_ERROR_FACTORY_NOT_REGISTERED; > + } > + return prefs->SetIntPref("network.proxy.type", 4); We need to reset the pref when the test ends (Teardown)
Attachment #8942520 - Flags: review+
Hi all, It looks we are close to getting a fix for the dhcp / wpad issue. Do we have timescale for a release with the fix in it. I am happy to beta test a version with the fix. we use 1 dns for two sites with a per site proxy server. using dhcp would automatically send the user to the local proxy. Regards Robert
(In reply to Robert Fwernando from comment #88) > Hi all, > It looks we are close to getting a fix for the dhcp / wpad issue. > Do we have timescale for a release with the fix in it. Provided we land it soon, it should be in Firefox 61. > I am happy to beta test a version with the fix. > we use 1 dns for two sites with a per site proxy server. using dhcp would > automatically send the user to the local proxy. You can try a version from here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48295b341014d2b6cbd3f1e9fdd021f12e60c14b Click on the B opt (optimized) version for your platform. Then download target.tar.bz2 (Linux), or target.zip for Windows. If you want an OS X or Android build let me know, and I'll trigger one for you.
Flags: needinfo?(valentin.gosu)
Have updated patch on Review board with the old commit message and the pref reset in the tear-down.
Flags: needinfo?(valentin.gosu)
Daniel, can you take a final look so we can land this?
Flags: needinfo?(valentin.gosu) → needinfo?(daniel)
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review244894
Attachment #8942520 - Flags: review?(daniel) → review+
Flags: needinfo?(daniel)
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2a760b1c0759 Proxy autodiscovery doesn't check DHCP (option 252) r=bagder,valentin
Thank you for all the work, Polly.
Backed out changeset 2a760b1c0759 (bug 356831) for adding files without having a bugzilla product and component https://hg.mozilla.org/integration/autoland/rev/9c2af62ba0cd59f8bd0b4cdbf037576db7a150bd Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175362837&repo=autoland
Flags: needinfo?(polly.shaw)
Hi Polly, last fixup hopefully, can you add with Files('**'): BUG_COMPONENT = ('Core', 'Networking: HTTP') to toolkit/system/windowsDHCPClient/moz.build and toolkit/system/windowsDHCPClient/tests/gtest/moz.build ? Thanks!
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/204bb43af943 Proxy autodiscovery doesn't check DHCP (option 252) r=bagder,valentin
Flags: needinfo?(polly.shaw)
Keywords: qawanted, user-doc-needed
@Robert Fwernando It's worth saying that this patch _only_ fixes Windows. I think it would be fairly straightforward to implement it for MacOS, and that would be the platform which would be most worthwhile after Windows, as Macs seem to becoming increasingly popular in corporate environments (note that I have no stats to back this up, only an impression.) It would be a case of implementing a DHCP client for MacOS and using this method within it to get the option https://developer.apple.com/documentation/systemconfiguration/1500228-dhcpinfogetoptiondata. I would be interested in having a stab at this but a Mac is necessary and although I have one, it's rather old and sick. (And if anyone else wants to submit a Mac patch that would be good, too.)
https://hg.mozilla.org/mozilla-central/rev/204bb43af943
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Honza: we need to test that with this patch we never use wpad (either via dns or dhcp), even when we are in the default "use system proxy settings" (on windows in particular) and even if "system proxy settings" are using wpad. Can you use wireshark or something to verify that you don't see any WPAD requests happening on Windows? (It's obviously still ok for WPAD to be used if user has the use wpad setting selected in firefox).
Flags: needinfo?(honzab.moz)
Given the leaks in bug 1456918 and bug 1456863 and the fact that we're in the soft code freeze period, I thought it was best to backout the patch while I fix the leaks and land it again next week after the merge.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Valentin Gosu [:valentin] from comment #103) > Given the leaks in bug 1456918 and bug 1456863 and the fact that we're in > the soft code freeze period, I thought it was best to backout the patch > while I fix the leaks and land it again next week after the merge. ...and maybe on landing again some of the formatting nits could be fixed? (In reply to Jason Duell [:jduell] (needinfo me) from comment #102) > Honza: we need to test that with this patch we never use wpad (either via > dns or dhcp), even when we are in the default "use system proxy settings" > (on windows in particular) and even if "system proxy settings" are using > wpad. > > Can you use wireshark or something to verify that you don't see any WPAD > requests happening on Windows? > > (It's obviously still ok for WPAD to be used if user has the use wpad > setting selected in firefox). This request is kinda weird... isn't the patch introducing the support? Can you more explain why it's expected to _not_ see any request? Bug's too long to go through it to get a context. Thanks.
Flags: needinfo?(jduell.mcbugs)
Patrick, can you sync with Honza and lay out exactly what you want tested?
Flags: needinfo?(mcmanus)
comment 73
Flags: needinfo?(mcmanus)
status-firefox61: fixed → ---
Target Milestone: mozilla61 → ---
I have submitted another diff for review after looking through this for possible memory leaks. The thing I've changed is RefPtr<ConfigureWPADComplete> runnable = new ConfigureWPADComplete(mPACMan, spec); NS_DispatchToMainThread(runnable); to RefPtr<ConfigureWPADComplete> runnable = new ConfigureWPADComplete(mPACMan, spec); mPACMan->Dispatch(runnable.forget()); which is consistent with similar operations in the file. I haven't, however, used any automated tools to look for memory leaks. I've also managed to get this working on OS X but I'll submit that as another patch after this one has landed to avoid muddying the waters.
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review248918 ::: netwerk/base/nsPACMan.h:251 (Diff revisions 8 - 10) > LinkedList<PendingPACQuery> mPendingQ; /* pac thread only */ > > // These specs are not nsIURI so that they can be used off the main thread. > // The non-normalized versions are directly from the configuration, the > // normalized version has been extracted from an nsIURI > - nsCString mPACURISpec; > + nsACString mPACURISpec; I don't even know if this would build. This should remain nsCString.
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review248918 > I don't even know if this would build. > This should remain nsCString. Sorry. I am using Review board in a rather exploratory way because I haven't got the tests which fail due to the memory leak running locally, and `mach try` doesn't support mozharness. Will try to optimise.
Testing if we DON'T use 252-provided WPAD file with the default Firefox settings: All in virtual box * A virtual NAT'ed, no-DHCP network with ordinary direct internet access, all machines running inside that network * WinServer'16 running as an DHCP server for the network (having a static IP of 10.0.2.100), `PS> Add-DHCPServerv4OptionValue` for 252 (not avail by default), enabled it in Admin Tools/DHCP for the server * Win10 with [1], checked for having Windows settings at the default 'Automatically detect settings' in Internet Properties/Connections/LAN settings, result: a) Firefox with 'Auto-detect proxy ...' - this is the default - doesn't use the WPAD config file (we go straight to the internet) b) Firefox with 'Use system proxy' - this has to be manually changed in Firefox preferences - does use the WPAD config file For completeness, Edge with the default settings uses the provided WPAD. I didn't do any checks if we do the DHCP option request or in any way do a request for the WPAD with the default settings, but I don't think it's needed. Let me know if you need this test as well. ---- To be honest I'm still kinda lost how this has to work, also when looking at the patch and how we internally call proxy type settings and how we present them in the UI: PROXYCONFIG_SYSTEM = 5 (default), being "Auto-detect proxy settings for this network" in the UI PROXYCONFIG_WPAD = 4, being "Use system proxy settings" in the UI and allowing the WPAD read from DHCP (with this patch) [1] https://queue.taskcluster.net/v1/task/EahgT-XNRsK2wNiQ5nwHHQ/runs/0/artifacts/public/build/install/sea/target.installer.exe from https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=204bb43af943&filter-searchStr=build
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #114) > PROXYCONFIG_SYSTEM = 5 (default), being "Auto-detect proxy settings for this > network" in the UI > PROXYCONFIG_WPAD = 4, being "Use system proxy settings" in the UI and > allowing the WPAD read from DHCP (with this patch) Something weird is happening here. In the build with the patch ([1] in the prev comment) the text in the preferences UI is swapped for "system" and "auto-detect".
@mayhemer - yep - I filed a bug about the UI issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1458794 (now fixed.) Btw I have got the mochitests running on my machine with the aim of looking into the memory leak when I get time. It looks as though the object graph which is put onto the PAC thread is being reported.
@mayhemer - thanks for highlighting that.. I was totally confused about your comments wrt what was the default until I read the further comments and bugs. Can you just retest with the fixed nightly? It seems like everything is working ok other than the labeling.
This wouldn't work in the current nightly @mcmanus & @mayhemer because the patch has been backed out due to the memory leak (just saying this in case anyone isn't aware). Not sure if it's standard practice to integrate the patch into the current nightly for manual testing. Also my most recent submission to review board is pretty broken, as I was (wrongly) using reviewboard as an automation server for my hypotheses for the source of the memory leak before I realised that I could reproduce the bugs on .mach try - so if you do import a patch into nightly for test it please import the one that was approved (again sorry if this is just standard practice.)
Flags: needinfo?(jduell.mcbugs)
(In reply to Honza Bambas (:mayhemer) from comment #114) > To be honest I'm still kinda lost how this has to work, also when looking at > the patch and how we internally call proxy type settings and how we present > them in the UI: > > PROXYCONFIG_SYSTEM = 5 (default), being "Auto-detect proxy settings for this > network" in the UI > PROXYCONFIG_WPAD = 4, being "Use system proxy settings" in the UI and > allowing the WPAD read from DHCP (with this patch) Dear Honza, it's easy how it has to work. "Use system settings" should really use the system settings and that means check what's in Windows Registry (or in case of Linux / UNIX systems, what the system proxy settings say). Assuming Windows 10 as underlying system, in Windows Registry under HKCU\Software\Microsoft\Windows\CurrentVersion\Internet Settings\Connections you can find two keys. Both are REG_BINARY. The key to be checked is "DefaultConnectionSettings". The byte no. 9 is responsible for the type of proxy setting being used by the system: - 01 f. e. is direct access (no proxy). - 03 is manual proxy server. - 05 is auto config script with URL. - 09 is auto detect. After this you can find the other settings (including strings) of the "Local Area Network (LAN) Settings" UI from Windows, such as Address of auto script, if any, and the address of a manual proxy, if set. However, keeping in mind that "system settings" is working in any other case, f. e. when an automatic script is being used and the option of Windows is set to "use automatic script", the only case where Firefox does fail, is the system setting "Auto detect". So, the byte no. 9 of the registry key mentioned above will be 09. If that's the case, Firefox should check DHCP option 252 for a PAC URL. Hope that helps you to improve Firefox in the future. We, in our company, just changed to DHCP option 252 and all client devices use Firefox. So, I think you can guess that this bug is really annoying for us. Thanks and best regards Nico
(In reply to nico.domagalla from comment #119) > (In reply to Honza Bambas (:mayhemer) from comment #114) > > To be honest I'm still kinda lost how this has to work, also when looking at > > the patch and how we internally call proxy type settings and how we present > > them in the UI: > > > > PROXYCONFIG_SYSTEM = 5 (default), being "Auto-detect proxy settings for this > > network" in the UI > > PROXYCONFIG_WPAD = 4, being "Use system proxy settings" in the UI and > > allowing the WPAD read from DHCP (with this patch) > > [...] > > Assuming Windows 10 as underlying system, in Windows Registry under > HKCU\Software\Microsoft\Windows\CurrentVersion\Internet Settings\Connections > you can find two keys. Both are REG_BINARY. The key to be checked is > "DefaultConnectionSettings". The byte no. 9 is responsible for the type of > proxy setting being used by the system: > > - 01 f. e. is direct access (no proxy). > - 03 is manual proxy server. > - 05 is auto config script with URL. > - 09 is auto detect. > > After this you can find the other settings (including strings) of the "Local > Area Network (LAN) Settings" UI from Windows, such as Address of auto > script, if any, and the address of a manual proxy, if set. > > However, keeping in mind that "system settings" is working in any other > case, f. e. when an automatic script is being used and the option of Windows > is set to "use automatic script", the only case where Firefox does fail, is > the system setting "Auto detect". So, the byte no. 9 of the registry key > mentioned above will be 09. If that's the case, Firefox should check DHCP > option 252 for a PAC URL. > > [...] Just to add: It's an XOR. Saying that, the bit positions are: - 1: nothing - 2: manual proxy - 4: PAC (not via DHCP) - 8: Auto detect So, to correct my sentence: Firefox should check for PAC via DHCP option 252 if the byte no. 9 of above mentioned reg key contains the bit position 8. Best regards Nico
Oh, and I have to add something else. Even the option "Detect proxy settings from network" in Firefox itself does not recognize DHCP option 252 in version 60.0.2 64 bit. So ... does the DHCP detection function work at all? I think there're many people out there waiting for a fix. And as you can see, this function is more common than you've expected.
(In reply to nico.domagalla from comment #121) > Oh, and I have to add something else. Even the option "Detect proxy settings > from network" in Firefox itself does not recognize DHCP option 252 in > version 60.0.2 64 bit. So ... does the DHCP detection function work at all? No. This is what this bug is all about - adding DHCP detection.
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review257894 ::: netwerk/base/nsProtocolProxyService.cpp:1135 (Diff revision 12) > } else if (mSystemProxySettings) { > // Get System Proxy settings if available > AsyncConfigureFromPAC(false, false); > } > - if (!tempString.IsEmpty()) > + if (!tempString.IsEmpty() || mProxyConfig == PROXYCONFIG_WPAD) { > ConfigureFromPAC(tempString, false); This was the critical line that was causing the leak in the previous versions.`forceReload` was set to true as a parameter to `ConfigureFromPAC`. The leak was always a ProcessQueue message beeing put on the PAC Thread, and it leaked even when DHCP wasn't being called. Fixing this has removed the leak from the tests, but is it just hiding it, because `ConfigureFromPAC` with `forceReload` is called more often?
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a7a1006e2f52 Proxy autodiscovery doesn't check DHCP (option 252) r=bagder,valentin
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review262968 ::: toolkit/system/windowsDHCPClient/DHCPUtils.cpp:87 (Diff revision 13) > + // rather than just the two that would be neccessary if we could rely on the > + // value returned in outBufLen being the true size needed. > + > + std::vector<IP_ADAPTER_ADDRESSES> pAddresses; > + do { > + pAddresses.resize(outBufLen/sizeof(IP_ADAPTER_ADDRESSES)); I think the asan error is a off by one. 15000 is not a multiple of IP_ADAPTER_ADDRESSES, so we end up allocating one less. So while outBufLen==15000, pAddresses.size() == 14823
Thanks Valentin - that was it. Have now re-opened the review and updated with a fix.
Flags: needinfo?(polly.shaw)
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review263056 ::: toolkit/system/windowsDHCPClient/DHCPUtils.cpp:89 (Diff revisions 13 - 14) > > std::vector<IP_ADAPTER_ADDRESSES> pAddresses; > do { > + // if outBufLen is not a multiple of the size of IP_ADAPTER_ADDRESSES, make it the next biggest multiple. > + uint32_t outBufLenModSizeofStruct = outBufLen % sizeof(IP_ADAPTER_ADDRESSES); > + if (outBufLen != 0) { This should have been outBufLenModSizeofStruct, right? Anyway, can we try something like: outBufLen = ((outBufLen + sizeof(IP_ADAPTER_ADDRESSES) - 1) / sizeof(IP_ADAPTER_ADDRESSES)) * sizeof(IP_ADAPTER_ADDRESSES); It should round up to the nearest multiple of sizeof(IP_ADAPTER_ADDRESSES).
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review263056 > This should have been outBufLenModSizeofStruct, right? > > Anyway, can we try something like: > outBufLen = ((outBufLen + sizeof(IP_ADAPTER_ADDRESSES) - 1) / sizeof(IP_ADAPTER_ADDRESSES)) * sizeof(IP_ADAPTER_ADDRESSES); > > It should round up to the nearest multiple of sizeof(IP_ADAPTER_ADDRESSES). Oops. Yes. Have implemented your line and is currently running on https://treeherder.mozilla.org/#/jobs?repo=try&revision=926bdaec74a901dd82a32c237835bc62cdce0af9
Comment on attachment 8942520 [details] Bug 356831 - Proxy autodiscovery doesn't check DHCP (option 252) https://reviewboard.mozilla.org/r/212778/#review262968 > I think the asan error is a off by one. 15000 is not a multiple of IP_ADAPTER_ADDRESSES, so we end up allocating one less. > So while outBufLen==15000, pAddresses.size() == 14823 thanks Valentin - that ws it. Have now made outBufLen the next multiple of IP_ADAPTER_ADDRESSES and it has passed on https://treeherder.mozilla.org/#/jobs?repo=try&revision=2750072b7dd3872bfeda161c0ec2026714168c6f
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3e47ebfc2f42 Proxy autodiscovery doesn't check DHCP (option 252) r=bagder,valentin
https://hg.mozilla.org/mozilla-central/rev/3e47ebfc2f42
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Blocks: 1476899
Depends on: 1489229
I just tried this on 62.0.3 (64bit) under Windows 10 and it is still not working. Edge, IE, Chrome all respect the presences (or lack of) this DHCP option. Expectation: When networking settings set to auto-detect or to systems settings (and system setting is auto detect), that the browser respects the current proxy pac/WPAD/DHCP-Option-252. i.e. When present on the current network connection, use it; and when not present connect directly. This can change while the browser is running, however this not even working when the browser is restarted. We really need this working in ESR! not just mainline.
(In reply to Thorben J. from comment #139) > I just tried this on 62.0.3 (64bit) under Windows 10 and it is still not > working. This bug is fixed in Firefox 63. :) You can try using Firefox Beta.
Thank you for the info, Valentin. Will it be backported to ESR? This bug is far more critical in enterprise environments, who mostly use ESR (as we do). I doubt any home user on mainline FF ever noticed this problem.
I just confirmed that it works in FF Beta. Please port to ESR!
ESR only takes security sensitive bug fixes or major regressions fixes. This is a new feature that doesn't qualify for ESR uplift. Note that this bug (original feature request) is more than 10 years old. I'm really sorry.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: