Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add libproxy support #977

Closed
wants to merge 1 commit into from
Closed

Add libproxy support #977

wants to merge 1 commit into from

Conversation

dwmw2
Copy link
Contributor

@dwmw2 dwmw2 commented Aug 23, 2016

This adds optional support for using libproxy to determine which proxy to use for a given URL.

The original libproxy has evolved into a C++ monstrosity which potentially loads a JavaScript interpreter into the address space of anything which uses it... but the API is clean and simple:
https://github.com/libproxy/libproxy/blob/master/libproxy/proxy.h

The PacRunner dæmon provides a service where you can simply ask it, over D-Bus, which proxy to use for a given URL. Not only does it have a plugin for the original libproxy, but it also provides its own replacement libproxy implementation which does nothing but field the call to PacRunner.

I am mentoring a Google Summer of Code project this year which is fixing NetworkManager to pick up proxy information from DHCP, VPN servers and other sources, and to allow it to be an explicit part of per-network configuration — and to poke it into PacRunner appropriately.

Thus the expectation will be that asking libproxy should consistently give correct answers. Once that's true, I hope to change Linux distribution packaging guidelines to suggest that packaged applications should be using libproxy's results (or at least PacRunner's results) by default.

This implements libproxy support for curl because it seems like the more generic option, rather than talking to PacRunner directly over D-Bus. This is just the basic functionality; we need to look at how we make it accessible from the curl command line tool, and/or enable it by default.

@bagder
Copy link
Member

bagder commented Aug 23, 2016

The new symbol is missing from docs/libcurl/symbols-in-versions which is causing the test failure. Test 1119.

@dwmw2
Copy link
Contributor Author

dwmw2 commented Aug 23, 2016

Thanks. I made it say 7.50.3 since we're not targetting 7.50.2.

@JCMais
Copy link
Contributor

JCMais commented Aug 23, 2016

If there are no plans to include that on the 7.50.2 release, you probably should target 7.51 directly. But I might be interpreting the release procedure wrongly.

@bagder bagder added the feature-window A merge of this requires an open feature window label Sep 4, 2016
@bagder
Copy link
Member

bagder commented Sep 19, 2016

I've updated the patch slightly to pass all tests: 0001-proxy-add-libproxy-support.patch

I'm a concerned about:

  1. There is no proper documentation for the libproxy API accessible. It makes me not really trust the library. It seems to live in a stealth/"pre-official API" mode?
  2. The fact that we call free() on data returned by the libproxy library might imply that this isn't working properly on windows? (where different memory models in different components are common) - which goes back to the trustworthiness of libproxy. Is libproxy intended only for *nix?
  3. The use a of a global variable in this implementation seems to me it might not be thread-safe? What exactly is held in that global and why not keep it per-easy-handle?

@bagder bagder added on-hold and removed feature-window A merge of this requires an open feature window labels Sep 20, 2016
@dwmw2 dwmw2 force-pushed the libproxy branch 2 times, most recently from 618553b to 006489b Compare September 26, 2016 11:09
@dwmw2
Copy link
Contributor Author

dwmw2 commented Sep 26, 2016

Apologies for the delay; I've been deep down the rathole of #974 (and will post an update there shortly).
I've updated my libproxy branch to include your latest changes; thanks.

  1. There is no proper documentation for the libproxy API accessible. It makes me not really trust the library. It seems to live in a stealth/"pre-official API" mode?

Hm? libproxy has had that API basically forever (December 2007 AFAICT, before they even set the licensing such that people could actually use it). The only real changes to the file since then have been to the documentation, which (such as it is) resides therein.

Even when its soname changed from libproxy.so.0 to libproxy.so.1 I don't believe it changed; that was just a case of "let's stop pretending it ever might change".

We even have other projects providing their own reimplementation of it, using precisely the same API: http://git.kernel.org/cgit/network/connman/pacrunner.git/tree/libproxy?id=HEAD

  1. The fact that we call free() on data returned by the libproxy library might imply that this isn't working properly on windows? (where different memory models in different components are common) - which goes back to the trustworthiness of libproxy. Is libproxy intended only for *nix?

Good point. The intent in libproxy, AIUI, is to be cross-platform. Even on platforms which typically have a single consistent location for proxy information, such as Windows, there's still benefit to something like curl, to being able to just use a simple cross-platform API for getting the information from wherever it comes from.

For Windows I think it's a bug that it requires the caller to free the data, for precisely the reasons you state. I've filed libproxy/libproxy#43 — ironically asking for a change (well, an addition) to that API that I've just said has been stable for almost a decade... :)

  1. The use a of a global variable in this implementation seems to me it might not be thread-safe? What exactly is held in that global and why not keep it per-easy-handle?

You really want that to be global; it's the libproxy state where it goes off and finds the PAC file via DHCP or WPAD or whatever, and loads a JS interpreter to handle it. It's bad enough that in the original libproxy implementation you get that JS interpreter in the context of every process (that's what PacRunner fixes, and gives you just a simple DBus query). But you certainly don't want a new one for every curl easy-handle!

The pxProxyFactory has its own locking and is intended to be thread-safe. So as long as curl_global_init() and curl_global_cleanup() really are being invoked only once and in a threadsafe fashion, this should be fine.

@bagder
Copy link
Member

bagder commented Apr 6, 2017

libproxy/libproxy#43 and libproxy/libproxy#52 with no action or even response for a very long time tells me we cannot adopt libproxy at this point in time.

@bagder bagder closed this Apr 6, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants