[v2,1/1] remote-curl: unbreak http.extraHeader with custom allocators

Commit Message

John Passaro via GitGitGadget Nov. 6, 2019, 10:04 a.m. UTC

From: Johannes Schindelin <[e-mail]>

In 93b980e58f5 (http: use xmalloc with cURL, 2019-08-15), we started to
ask cURL to use `xmalloc()`, and if compiled with nedmalloc, that means
implicitly a different allocator than the system one.

Which means that all of cURL's allocations and releases now _need_ to
use that allocator.

However, the `http_options()` function used `slist_append()` to add any
configured extra HTTP header(s) _before_ asking cURL to use `xmalloc()`,
and `http_cleanup()` would release them _afterwards_, i.e. in the
presence of custom allocators, cURL would attempt to use the wrong
allocator to release the memory.

A naïve attempt at fixing this would move the call to
`curl_global_init()` _before_ the config is parsed (i.e. before that
call to `slist_append()`).

However, that does work, as we _also_ parse the config setting
`http.sslbackend` and if found, call `curl_global_sslset()` which *must*
be called before `curl_global_init()`, for details see:
https://curl.haxx.se/libcurl/c/curl_global_sslset.html

So let's instead make the config parsing entirely independent from
cURL's data structures. Incidentally, this deletes two more lines than
it introduces, which is nice.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 http.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Jeff King Nov. 6, 2019, 11:29 a.m. UTC | #1

On Wed, Nov 06, 2019 at 10:04:55AM +0000, Johannes Schindelin via GitGitGadget wrote:

> A naïve attempt at fixing this would move the call to
> `curl_global_init()` _before_ the config is parsed (i.e. before that
> call to `slist_append()`).
> 
> However, that does work, as we _also_ parse the config setting
> `http.sslbackend` and if found, call `curl_global_sslset()` which *must*
> be called before `curl_global_init()`, for details see:
> https://curl.haxx.se/libcurl/c/curl_global_sslset.html

Yikes, good catch. It didn't even occur to me that there might be curl
things we had to do _before_ calling curl_global_init().

> So let's instead make the config parsing entirely independent from
> cURL's data structures. Incidentally, this deletes two more lines than
> it introduces, which is nice.

Yes, I actually find the resulting code easier to read. I had feared
having to add an extra step to initialize the slist, but it's all
handled quite neatly in http_copy_default_headers().

>  http.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

The patch itself looks good to me.

-Peff

Junio C Hamano Nov. 6, 2019, 12:12 p.m. UTC | #2

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> In 93b980e58f5 (http: use xmalloc with cURL, 2019-08-15), we started to
> ask cURL to use `xmalloc()`, and if compiled with nedmalloc, that means
> implicitly a different allocator than the system one.
>
> Which means that all of cURL's allocations and releases now _need_ to
> use that allocator.
>
> However, the `http_options()` function used `slist_append()` to add any
> configured extra HTTP header(s) _before_ asking cURL to use `xmalloc()`,
> and `http_cleanup()` would release them _afterwards_, i.e. in the
> presence of custom allocators, cURL would attempt to use the wrong
> allocator to release the memory.

s/allocator/de&/; perhaps, even though it is clear enough from the
context, so it is probably OK as is.

> A naïve attempt at fixing this would move the call to
> `curl_global_init()` _before_ the config is parsed (i.e. before that
> call to `slist_append()`).
>
> However, that does work, as we _also_ parse the config setting

s/does work/does not work/; presumably?

> `http.sslbackend` and if found, call `curl_global_sslset()` which *must*
> be called before `curl_global_init()`, for details see:
> https://curl.haxx.se/libcurl/c/curl_global_sslset.html
>
> So let's instead make the config parsing entirely independent from
> cURL's data structures. Incidentally, this deletes two more lines than
> it introduces, which is nice.

Yeah, string_list_clear() is more concise than curl_slist_free_all(),
and we have already been copying one list to another anyway, so we
lucked out ;-)

The patch looked good to me, too.

Johannes Schindelin Nov. 6, 2019, 7:34 p.m. UTC | #3

Hi Junio,

On Wed, 6 Nov 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > In 93b980e58f5 (http: use xmalloc with cURL, 2019-08-15), we started to
> > ask cURL to use `xmalloc()`, and if compiled with nedmalloc, that means
> > implicitly a different allocator than the system one.
> >
> > Which means that all of cURL's allocations and releases now _need_ to
> > use that allocator.
> >
> > However, the `http_options()` function used `slist_append()` to add any
> > configured extra HTTP header(s) _before_ asking cURL to use `xmalloc()`,
> > and `http_cleanup()` would release them _afterwards_, i.e. in the
> > presence of custom allocators, cURL would attempt to use the wrong
> > allocator to release the memory.
>
> s/allocator/de&/; perhaps, even though it is clear enough from the
> context, so it is probably OK as is.
>
> > A naïve attempt at fixing this would move the call to
> > `curl_global_init()` _before_ the config is parsed (i.e. before that
> > call to `slist_append()`).
> >
> > However, that does work, as we _also_ parse the config setting
>
> s/does work/does not work/; presumably?

Indeed. Could I ask you to fix up locally, or do you want me to send a
new revision of the patch?

Ciao,
Dscho

>
> > `http.sslbackend` and if found, call `curl_global_sslset()` which *must*
> > be called before `curl_global_init()`, for details see:
> > https://curl.haxx.se/libcurl/c/curl_global_sslset.html
> >
> > So let's instead make the config parsing entirely independent from
> > cURL's data structures. Incidentally, this deletes two more lines than
> > it introduces, which is nice.
>
> Yeah, string_list_clear() is more concise than curl_slist_free_all(),
> and we have already been copying one list to another anyway, so we
> lucked out ;-)
>
> The patch looked good to me, too.
>

Junio C Hamano Nov. 7, 2019, 5:39 a.m. UTC | #4

Johannes Schindelin <[e-mail]> writes:

> On Wed, 6 Nov 2019, Junio C Hamano wrote:
>
>> > presence of custom allocators, cURL would attempt to use the wrong
>> > allocator to release the memory.
>>
>> s/allocator/de&/; perhaps, even though it is clear enough from the
>> context, so it is probably OK as is.
>>
>> > A naïve attempt at fixing this would move the call to
>> > `curl_global_init()` _before_ the config is parsed (i.e. before that
>> > call to `slist_append()`).
>> >
>> > However, that does work, as we _also_ parse the config setting
>>
>> s/does work/does not work/; presumably?
>
> Indeed. Could I ask you to fix up locally, or do you want me to send a
> new revision of the patch?

You can certainly tell me to locally tweak, but you'd need to be
more specific when some of my suggestions were "perhaps" (aka "you
could do it this way, which may be better, but I do not care too
deeply---I am OK with whichever you chooose after you (re)think
about it, but I am not choosing for you").

For now, I'll do the "does not work" thing only.

Thanks.

Johannes Schindelin Nov. 7, 2019, 12:40 p.m. UTC | #5

Hi Junio,

On Thu, 7 Nov 2019, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > On Wed, 6 Nov 2019, Junio C Hamano wrote:
> >
> >> > presence of custom allocators, cURL would attempt to use the wrong
> >> > allocator to release the memory.
> >>
> >> s/allocator/de&/; perhaps, even though it is clear enough from the
> >> context, so it is probably OK as is.
> >>
> >> > A naïve attempt at fixing this would move the call to
> >> > `curl_global_init()` _before_ the config is parsed (i.e. before that
> >> > call to `slist_append()`).
> >> >
> >> > However, that does work, as we _also_ parse the config setting
> >>
> >> s/does work/does not work/; presumably?
> >
> > Indeed. Could I ask you to fix up locally, or do you want me to send a
> > new revision of the patch?
>
> You can certainly tell me to locally tweak, but you'd need to be
> more specific when some of my suggestions were "perhaps" (aka "you
> could do it this way, which may be better, but I do not care too
> deeply---I am OK with whichever you chooose after you (re)think
> about it, but I am not choosing for you").

Right, I should have been more specific.

> For now, I'll do the "does not work" thing only.

Thanks!

FWIW I would like to stick with "custom allocator" because even
releasing the memory is the duty of an "allocator", even if that
allocator happens to "deallocate" at that point what it has allocated
before.

Ciao,
Dscho

Junio C Hamano Nov. 8, 2019, 3:03 a.m. UTC | #6

Johannes Schindelin <[e-mail]> writes:

> FWIW I would like to stick with "custom allocator" because even
> releasing the memory is the duty of an "allocator", even if that
> allocator happens to "deallocate" at that point what it has allocated
> before.

My preference is to think of the set of functions we feed
curl-global-init-mem as a (allocator, deallocator, reallocator, ...)
tuple.

But you are calling that set itself as an allocator that has service
functions like (alloc, free, realloc), and as long as that (i.e. how
you defined the word "allocator") is clear to the readers that is
perfectly OK.  

To me as one reader, it was not clear and that was where my comment
came from.

Patch

diff --git a/http.c b/http.c
index 27aa0a3192..82f493c7fd 100644
--- a/http.c
+++ b/http.c
@@ -150,7 +150,7 @@  static unsigned long empty_auth_useless =
 
 static struct curl_slist *pragma_header;
 static struct curl_slist *no_pragma_header;
-static struct curl_slist *extra_http_headers;
+static struct string_list extra_http_headers = STRING_LIST_INIT_DUP;
 
 static struct active_request_slot *active_queue_head;
 
@@ -414,11 +414,9 @@  static int http_options(const char *var, const char *value, void *cb)
 		if (!value) {
 			return config_error_nonbool(var);
 		} else if (!*value) {
-			curl_slist_free_all(extra_http_headers);
-			extra_http_headers = NULL;
+			string_list_clear(&extra_http_headers, 0);
 		} else {
-			extra_http_headers =
-				curl_slist_append(extra_http_headers, value);
+			string_list_append(&extra_http_headers, value);
 		}
 		return 0;
 	}
@@ -1199,8 +1197,7 @@  void http_cleanup(void)
 #endif
 	curl_global_cleanup();
 
-	curl_slist_free_all(extra_http_headers);
-	extra_http_headers = NULL;
+	string_list_clear(&extra_http_headers, 0);
 
 	curl_slist_free_all(pragma_header);
 	pragma_header = NULL;
@@ -1624,10 +1621,11 @@  int run_one_slot(struct active_request_slot *slot,
 
 struct curl_slist *http_copy_default_headers(void)
 {
-	struct curl_slist *headers = NULL, *h;
+	struct curl_slist *headers = NULL;
+	const struct string_list_item *item;
 
-	for (h = extra_http_headers; h; h = h->next)
-		headers = curl_slist_append(headers, h->data);
+	for_each_string_list_item(item, &extra_http_headers)
+		headers = curl_slist_append(headers, item->string);
 
 	return headers;
 }