Re: [PATCH 1/1] remote-curl: unbreak http.extraHeader with custom allocators

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 6, 2019, 09:14 UTC
Message-ID: <nycvar.QRO.7.76.6.1911061009230.46@tvgsbejvaqbjf.bet>
In-Reply-To: <20191106041644.GE4307@sigill.intra.peff.net>

Hi Peff,

On Tue, 5 Nov 2019, Jeff King wrote:

Show 26 quoted lines

> On Tue, Nov 05, 2019 at 09:59:18PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
> >
> > Let's fix this by moving the initialization _before_ the
> > `http_options()` function is called.
>
> Nicely explained.
>
> Another option would be to separate our config mechanism from curl
> entirely by putting the list of headers into a string_list, and then
> transforming it later into a curl_slist. I don't think that really buys
> us much, though.

Alas, it _does_ buy us a lot, as I *just* found out (can you imagine how glad I am not to have rushed out another Git for Windows release?). It buys us one less bug: my patch introduces the bug where `http.sslbackend` can no longer be used, because `curl_global_sslset()` needs to be called _before_ `curl_global_init()`, but my patch breaks that because we _need_ to parse the config before we can ask cURL for a specific backend.

> This is all inside http.c, so it's fairly contained.  It's not like
> other random parts of Git are using curl's slist before calling
> http_init().

Indeed. We cannot use cURL's slist anywhere outside of the cURL-dependent code because we want to keep `NO_CURL=Yep` working.

> I did briefly grep around for other slist users, but they're all what
> you'd expect: code in http-push.c and remote-curl.c creating header
> lists while working with an active http request (so well after
> http_init() has been called).

Indeed, I came to the same conclusion that Carlo's patch only broke support for `http.extraheaders` (and only with custom allocators), nothing else.

I will change the patch to avoid using `slist` early and send another iteration.

Thanks, Dscho

Show 8 quoted lines

> > ---
> >  http.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
>
> The patch itself looks good.
>
> -Peff
>