ci: fix windows-build with GCC v12.x

compat/win32/syslog.c +1 -1

 		if (!str) {

@@ -50,6 +51,7 @@ void syslog(int priority, const char *fmt, ...)
 			warning_errno("realloc failed");
 			return;
 		}
+		pos = str + offset;
 		memmove(pos + 2, pos + 1, strlen(pos));
 		pos[1] = ' ';
 	}
--

gitgitgadget

Quoted 1 line

str = realloc(str, st_add(++str_len, 1));

Since it has been raised elsewhere: Why is that ++str_len not turned into an st_add()?
...
Now you know,

compat/win32/syslog.c void syslog(int priority, const char *fmt, ...) · line 54

Quoted 5 lines

compat/win32/syslog.c +1 -0

@@ -50,6 +51,7 @@ void syslog(int priority, const char *fmt, ...)
 			warning_errno("realloc failed");
 			return;
 		}
+		pos = str + offset;

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 compat/nedmalloc/nedmalloc.c | 1 -
 1 file changed, 1 deletion(-)

compat/nedmalloc/nedmalloc.c +0 -0

index edb438a7776..2c0ace7075a 100644

This seems sensible, I thought "why not submit it to upstream", i.e. see:
https://github.com/ned14/nedmalloc/blob/master/nedmalloc.c#L1298

But that repository was last updated in 2014, I wonder if it's just because nobody's submitted a patch since then, or if it's inactive. Have you tried making Njall Douglas (the nedmalloc author) aware of this issue?

Am 24.05.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:

Quoted 5 lines

This seems sensible, I thought "why not submit it to upstream", i.e. see:
https://github.com/ned14/nedmalloc/blob/master/nedmalloc.c#L1298

Quoted 5 lines

But that repository was last updated in 2014, I wonder if it's just because nobody's submitted a patch since then, or if it's inactive. Have you tried making Njall Douglas (the nedmalloc author) aware of this issue?

https://github.com/ned14/nedmalloc says at the top: "This repository has been archived by the owner. It is now read-only.".

Hi René,

Quoted 12 lines · René Scharfe

Am 24.05.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:

Quoted 10 lines

This seems sensible, I thought "why not submit it to upstream", i.e. see:
https://github.com/ned14/nedmalloc/blob/master/nedmalloc.c#L1298

But that repository was last updated in 2014, I wonder if it's just because nobody's submitted a patch since then, or if it's inactive. Have you tried making Njall Douglas (the nedmalloc author) aware of this issue?

Quoted 2 lines

https://github.com/ned14/nedmalloc says at the top: "This repository has been archived by the owner. It is now read-only.".

Indeed, maintenance of nedmalloc has stopped a few years ago (see e.g.
https://github.com/ned14/nedmalloc/issues/20#issuecomment-343432314).

About five years ago I tried to upgrade us to the latest nedmalloc version but ran into a performance regression that I was unable to justify the time to investigate further.

From: Johannes Schindelin <[email protected]>
[...]
Let's drop that local variable and introduce a new flag in the slot that is used to indicate that even while the slot is no longer in use, it is still reserved until further notice. It is the responsibility of run_active_slot() to clear that flag once it is done with that slot.

Initial-patch-by Junio C Hamano <[email protected]>

Quoted 7 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>
[...]
Let's drop that local variable and introduce a new flag in the slot that is used to indicate that even while the slot is no longer in use, it is still reserved until further notice. It is the responsibility of run_active_slot() to clear that flag once it is done with that slot.

Quoted 1 line

Initial-patch-by Junio C Hamano <[email protected]>

Don't you mean by me?
I.e. https://lore.kernel.org/git/[email protected]/

This seems to be derived from that, or perhaps you just came up with something similar independently. Junio then came up with the smaller
https://lore.kernel.org/git/[email protected]/

The "clear slot->finished", by the way, is what I think is the right thing to do, especially that the objective is to squelch the false positive warning from a new compiler. If there is a way to annotate the line for the compiler to tell it not to warn about it, that would have been even better.

Hi Junio,

Quoted 5 lines · Junio C Hamano

The "clear slot->finished", by the way, is what I think is the right thing to do, especially that the objective is to squelch the false positive warning from a new compiler. If there is a way to annotate the line for the compiler to tell it not to warn about it, that would have been even better.

We could do something like this:

-- snip --

http.c +7 -1

index b08795715f8a..2ac8d51d3668 100644
@@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
 	struct timeval select_timeout;
 	int finished = 0;
 
+#if __GNUC__ >= 12
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdangling-pointer"
+#endif
 	slot->finished = &finished;
+#if __GNUC__ >= 12
+#pragma GCC diagnostic pop
+#endif
 	while (!finished) {
 		step_active_slots();
-- snap --

That's quite ugly, though. And what's worse, it is pretty unreadable, too.

Hi Junio,

Quoted 5 lines · Junio C Hamano

The "clear slot->finished", by the way, is what I think is the right thing to do, especially that the objective is to squelch the false positive warning from a new compiler. If there is a way to annotate the line for the compiler to tell it not to warn about it, that would have been even better.

We could do something like this:

-- snip --

http.c +7 -1

index b08795715f8a..2ac8d51d3668 100644
@@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
 	struct timeval select_timeout;
 	int finished = 0;
 
+#if __GNUC__ >= 12
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdangling-pointer"
+#endif
 	slot->finished = &finished;
+#if __GNUC__ >= 12
+#pragma GCC diagnostic pop
+#endif
 	while (!finished) {
 		step_active_slots();
-- snap --

That's quite ugly, though. And what's worse, it is pretty unreadable, too.

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · Junio C Hamano

The "clear slot->finished", by the way, is what I think is the right thing to do, especially that the objective is to squelch the false positive warning from a new compiler. If there is a way to annotate the line for the compiler to tell it not to warn about it, that would have been even better.

We could do something like this:

Yuck.

Quoted 21 lines

-- snip --

http.c +7 -1

index b08795715f8a..2ac8d51d3668 100644
@@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
 	struct timeval select_timeout;
 	int finished = 0;
 
+#if __GNUC__ >= 12
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdangling-pointer"
+#endif
 	slot->finished = &finished;
+#if __GNUC__ >= 12
+#pragma GCC diagnostic pop
+#endif
 	while (!finished) {
 		step_active_slots();
-- snap --

Quoted 1 line

That's quite ugly, though. And what's worse, it is pretty unreadable, too.

Yes, very ugly. Would an unconditional

	slot->finished = NULL;

at the end squelch the warning?

Or there is a way to say "we make all warnings into errors with -Werror, but we do not want to turn this dangling-pointer warning to an error, because it has false positives"?

Or we could add "-Wno-dangling-pointer" globally, perhaps.

Hi Junio,

Quoted 39 lines · Junio C Hamano

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · Junio C Hamano

The "clear slot->finished", by the way, is what I think is the right thing to do, especially that the objective is to squelch the false positive warning from a new compiler. If there is a way to annotate the line for the compiler to tell it not to warn about it, that would have been even better.

We could do something like this:

Yuck.

Quoted 21 lines

-- snip --

http.c +7 -1

index b08795715f8a..2ac8d51d3668 100644
@@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
 	struct timeval select_timeout;
 	int finished = 0;
 
+#if __GNUC__ >= 12
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdangling-pointer"
+#endif
 	slot->finished = &finished;
+#if __GNUC__ >= 12
+#pragma GCC diagnostic pop
+#endif
 	while (!finished) {
 		step_active_slots();
-- snap --

Quoted 1 line

That's quite ugly, though. And what's worse, it is pretty unreadable, too.

Yes, very ugly. Would an unconditional

Quoted 3 lines

	slot->finished = NULL;

at the end squelch the warning?

No, unfortunately not:
https://github.com/dscho/git/actions/runs/2383492484

As you mentioned elsewhere, the error is clearly about the assignment in the first place, and it does not matter that the function will rectify the situation. It's the correct thing to do for the compiler, too: since slot->finished already has the pointer, and since the active_request_slot struct is declared globally, code outside the current file might squirrel that pointer away for later use.

Quoted 5 lines

Or there is a way to say "we make all warnings into errors with -Werror, but we do not want to turn this dangling-pointer warning to an error, because it has false positives"?

Or we could add "-Wno-dangling-pointer" globally, perhaps.

I'd like to avoid that because it would quite likely hide legitimate issues elsewhere.

It currently seems to be the easiest solution to simply turn the local variable into a heap variable:

-- snip --

http.c +3 -3

index f92859f43fa..0712debd558 100644
@@ -1327,10 +1327,10 @@ void run_active_slot(struct active_request_slot *slot)
 	fd_set excfds;
 	int max_fd;
 	struct timeval select_timeout;
-	int finished = 0;
+	int *finished = xcalloc(1, sizeof(int));
 
-	slot->finished = &finished;
-	while (!finished) {
+	slot->finished = finished;
+	while (!*finished) {
 		step_active_slots();
 
 		if (slot->in_use) {

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 dir.c | 9 +++++++++
 1 file changed, 9 insertions(+)

dir.c +0 -0

index 5aa6fbad0b7..ea78f606230 100644

dir.c +2 -0

+		      "Please specify a directory on the command line"));
+
 	/*
 	 * Strip trailing port number if we've got only a
 	 * hostname (that is, there is no dir separator but a

Quoted 36 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 dir.c | 9 +++++++++
 1 file changed, 9 insertions(+)

dir.c +0 -0

index 5aa6fbad0b7..ea78f606230 100644
@@ -3076,6 +3076,15 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare)
 			end--;
 	}
 

dir.c char *git_url_basename(const char *repo, int is_bundle, int is_bare) · line 3085

Quoted 7 lines

dir.c +7 -0

+	/*
+	 * It should not be possible to overflow `ptrdiff_t` by passing in an
+	 * insanely long URL, but GCC does not know that and will complain
+	 * without this check.
+	 */
+	if (end - start < 0)
+		die(_("No directory name could be guessed.\n"

This should start with a lower-case letter, see CodingGuidelines.

Quoted 6 lines

dir.c +2 -0

+		      "Please specify a directory on the command line"));
+
 	/*
 	 * Strip trailing port number if we've got only a
 	 * hostname (that is, there is no dir separator but a

Quoted 10 lines · Johannes Schindelin via GitGitGadget

dir.c char *git_url_basename(const char *repo, int is_bundle, int is_bare) · line 3085

Quoted 7 lines

dir.c +7 -0

+	/*
+	 * It should not be possible to overflow `ptrdiff_t` by passing in an
+	 * insanely long URL, but GCC does not know that and will complain
+	 * without this check.
+	 */
+	if (end - start < 0)
+		die(_("No directory name could be guessed.\n"

This should start with a lower-case letter, see CodingGuidelines.

This message is copied from existing code later in the same function. Since it is a translateable message, I do not want to edit it because that would cause unnecessary work of the translators. Especially given that we do not even expect this message to be shown, ever, but we only add this hunk for GCC's benefit.

Quoted 11 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · Johannes Schindelin via GitGitGadget

dir.c char *git_url_basename(const char *repo, int is_bundle, int is_bare) · line 3085

Quoted 7 lines

dir.c +7 -0

+	/*
+	 * It should not be possible to overflow `ptrdiff_t` by passing in an
+	 * insanely long URL, but GCC does not know that and will complain
+	 * without this check.
+	 */
+	if (end - start < 0)
+		die(_("No directory name could be guessed.\n"

This should start with a lower-case letter, see CodingGuidelines.

Quoted 5 lines

This message is copied from existing code later in the same function. Since it is a translateable message, I do not want to edit it because that would cause unnecessary work of the translators. Especially given that we do not even expect this message to be shown, ever, but we only add this hunk for GCC's benefit.

Perhaps this should be a BUG() statement, then? Without any translation?

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.

These patches address that, and they are based on maint-2.34 (earlier maintenance tracks would have a trivial merge conflict due to 013c7e2b070 (http: drop support for curl < 7.16.0, 2021-07-30) removing support for USE_CURL_MULTI).

Johannes Schindelin (4):
compat/win32/syslog: fix use-after-realloc
nedmalloc: avoid new compile error
http.c: avoid danging pointer to local variable finished dir.c: avoid "exceeds maximum object size" error with GCC v12.x

  compat/nedmalloc/nedmalloc.c |  1 -
  compat/win32/syslog.c        |  2 ++
  dir.c                        |  9 +++++++++
  http-walker.c                |  4 ----
  http.c                       | 15 +++++++--------
  http.h                       |  2 +-
  6 files changed, 19 insertions(+), 14 deletions(-)

base-commit: 2f0dde7852b7866bb044926f73334ff3fc30654b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1238%2Fdscho%2Ffix-win-build-with-gcc-12-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1238/dscho/fix-win-build-with-gcc-12-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1238