ci: fix windows-build with GCC v12.x
#509
Merged
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
dscho
force-pushed
the
fix-win-build-with-gcc-12-msftgit
branch
from
a68db69
to
35d5036
Compare
jeffhostetler
approved these changes
May 24, 2022
| @@ -43,13 +43,15 @@ void syslog(int priority, const char *fmt, ...) | |||
| va_end(ap); | |||
|
|
|||
| while ((pos = strstr(str, "%1")) != NULL) { | |||
| size_t offset = pos - str; | |||
| char *oldstr = str; | |||
| str = realloc(str, st_add(++str_len, 1)); | |||
There was a problem hiding this comment.
this is probably off-topic, but IIUC the st_add() is used to guard against overflow, but we're also doing a ++str_len inside the args. it seems like we should cut this apart.
There was a problem hiding this comment.
You are correct, and I should have explained the following in the commit message:
Before this while() loop, we allocate one more than str_len, and we do that already using st_add().
Therefore, the first time we enter the loop, we know that ++str_len is safe.
In this line, then we increment str_len and the reallocate one more, again guarding it via st_add(). So every subsequent iteration will already have checked that ++str_len is safe, too.
By induction, it follows that this line is safe, and we do not have to change it to a clunkier two-step assignment.
| @@ -323,7 +323,6 @@ static NOINLINE void RemoveCacheEntries(nedpool *p, threadcache *tc, unsigned in | |||
| } | |||
| static void DestroyCaches(nedpool *p) THROWSPEC | |||
| { | |||
| if(p->caches) | |||
| { | |||
| * insanely long URL, but GCC does not know that and will complain | ||
| * without this check. | ||
| */ | ||
| if (end - start < 0) |
|
This all looks good to me. Thanks! |
Git for Windows' SDK recently upgraded to GCC v12.x which points out that the `pos` variable might be used even after the corresponding memory was `realloc()`ed and therefore potentially no longer valid. Since a subset of this SDK is used in Git's CI/PR builds, we need to fix this to continue to be able to benefit from the CI/PR runs. Note: This bug has been with us since 2a6b149 (mingw: avoid using strbuf in syslog, 2011-10-06), and while it looks tempting to replace the hand-rolled string manipulation with a `strbuf`-based one, that commit's message explains why we cannot do that: The `syslog()` function is called as part of the function in `daemon.c` which is set as the `die()` routine, and since `strbuf_grow()` can call that function if it runs out of memory, this would cause a nasty infinite loop that we do not want to re-introduce. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
GCC v12.x complains thusly:
compat/nedmalloc/nedmalloc.c: In function 'DestroyCaches':
compat/nedmalloc/nedmalloc.c:326:12: error: the comparison will always
evaluate as 'true' for the address of 'caches'
will never be NULL [-Werror=address]
326 | if(p->caches)
| ^
compat/nedmalloc/nedmalloc.c:196:22: note: 'caches' declared here
196 | threadcache *caches[THREADCACHEMAXCACHES];
| ^~~~~~
... and it is correct, of course.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Technically, the pointer difference `end - start` _could_ be negative,
and when cast to an (unsigned) `size_t` that would cause problems. In
this instance, the symptom is:
dir.c: In function 'git_url_basename':
dir.c:3087:13: error: 'memchr' specified bound [9223372036854775808, 0]
exceeds maximum object size 9223372036854775807
[-Werror=stringop-overread]
CC ewah/bitmap.o
3087 | if (memchr(start, '/', end - start) == NULL
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
While it is a bit far-fetched to think that `end` (which is defined as
`repo + strlen(repo)`) and `start` (which starts at `repo` and never
steps beyond the NUL terminator) could result in such a negative
difference, GCC has no way of knowing that.
See also https://gcc.gnu.org/bugzilla//show_bug.cgi?id=85783.
Let's just add a safety check, primarily for GCC's benefit.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This branch brings a couple of fixes to be able to compile Git in `DEVELOPER=1` mode again. The only remaining issue is a local variable's address in `run_active_slot()` in `http.c` that is assigned to a struct that is still valid after the function scope was left. Junio wanted to do this differently from my proposed solution, and we'll merge Junio's work-around next. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
fix-win-build-with-gcc-12-msftgit
branch
from
35d5036
to
a42ce5f
Compare
In http.c, the run_active_slot() function allows the given "slot" to make progress by calling step_active_slots() in a loop repeatedly, and the loop is not left until the request held in the slot completes. Ages ago, we used to use the slot->in_use member to get out of the loop, which misbehaved when the request in "slot" completes (at which time, the result of the request is copied away from the slot, and the in_use member is cleared, making the slot ready to be reused), and the "slot" gets reused to service a different request (at which time, the "slot" becomes in_use again, even though it is for a different request). The loop terminating condition mistakenly thought that the original request has yet to be completed. Today's code, after baa7b67 (HTTP slot reuse fixes, 2006-03-10) fixed this issue, uses a separate "slot->finished" member that is set in run_active_slot() to point to an on-stack variable, and the code that completes the request in finish_active_slot() clears the on-stack variable via the pointer to signal that the particular request held by the slot has completed. It also clears the in_use member (as before that fix), so that the slot itself can safely be reused for an unrelated request. One thing that is not quite clean in this arrangement is that, unless the slot gets reused, at which point the finished member is reset to NULL, the member keeps the value of &finished, which becomes a dangling pointer into the stack when run_active_slot() returns. Clear the finished member before the control leaves the function, which has a side effect of unconfusing compilers like recent GCC 12 that is over-eager to warn against such an assignment. Signed-off-by: Junio C Hamano <gitster@pobox.com>
This work-around shuts up GCC when it complains about a local variable's address being assigned to a struct that lives on after returning from the function. Technically, GCC is wrong not to complain that the pointer could have been copied and used later, but hey, the code compiles so we're happy. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This work-around shuts up GCC when it complains about a local variable's address being assigned to a struct that lives on after returning from the function. Technically, GCC is wrong not to complain that the pointer could have been copied and used later, but hey, the code compiles so we're happy. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This topic fixes the `windows-build` job of our CI/PR build after Git for Windows' SDK upgraded GCC to v12.x. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Let's use the correct data type when calling cURL. Pointed out by GCC v12.x. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
fix-win-build-with-gcc-12-msftgit
branch
from
a42ce5f
to
8123b74
Compare
dscho
added a commit
that referenced
this pull request
Jun 17, 2022
ci: fix `windows-build` with GCC v12.x
dscho
added a commit
that referenced
this pull request
Jun 17, 2022
ci: fix `windows-build` with GCC v12.x
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.
I noticed in #508 that a recent update of GCC in Git for Windows' SDK (a subset of which is used in Git's CI/PR builds) broke the build. Let's fix it.
This is a companion of gitgitgadget#1238 and git-for-windows#3864.