remote-curl: unbreak http.extraHeader with custom allocators #453
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.453.git.1572991158.gitgitgadget@gmail.com |
dscho
added a commit
to git-for-windows/build-extra
that referenced
this pull request
Nov 5, 2019
Using `http.extraHeader` [no longer results in spurious crashes](gitgitgadget/git#453). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
In 93b980e (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 <johannes.schindelin@gmx.de>
dscho
force-pushed
the
fix-curl-xmalloc-regression
branch
from
d47a2aa to
3168ba2
Compare
|
/submit |
|
Submitted as pull.453.v2.git.1573034695.gitgitgadget@gmail.com |
This comment has been minimized.
This comment has been minimized.
|
This branch is now known as |
|
This patch series was integrated into pu via git@94ada8c. |
|
And I almost updated, but decided to wait a few more days, appreciate all the bug hunting! |
|
This patch series was integrated into pu via git@bad5ed3. |
|
This patch series was integrated into next via git@bad5ed3. |
|
This patch series was integrated into master via git@bad5ed3. |
|
Closed via bad5ed3. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is one of those bugs that can cost entire days of work.
The symptom of this bug is that
git -c http.extraheader push ...will fail frequently in Git for Windows v2.24.0, but not always. When it fails, though, it will cause a segmentation fault ingit remote-httpswhile callinghttp_cleanup()and leave no visual indication of that problem, the only indication of a problem will be the exit code of the caller (in this instance,git pushwill fail with exit code 1).In my tests during the pre-release period, I pushed many a time, it probably failed a lot, in the way indicated above, and due to the absence of any error message, I failed to realize that there was a problem in the first place.
Fun side note: this bug haunted me for the best part of this past Monday, when I tried to get Git for Windows v2.24.0 out the door. Large parts of Git for Windows' release management are scripted, and that script failed, claiming to have been unsuccessful in pushing the tag
v2.24.0.windows.1, just after printing a message to the extent that the tag is already up to date (except in the first attempt, when it reported to have been successful in pushing the tag). My attempts to fix the release script were doomed to fail because the root cause was not a bug in the script, but the bug fixed in this patch.Changes since v1:
curl_global_init()(which would have broken support forhttp.sslbackend), this iteration instead replaces the usage of cURL'sslistin the config handling by using Git's ownstring_list.Cc: Carlo Marcelo Arenas Belón carenas@gmail.com