async_die_is_recursing: fix use of pthread_getspecific for Fedora

From: Victoria Dye <[email protected]>

Correct an issue encountered in the dockerized(pedantic, fedora) CI build, first appearing after the release of v36 as the latest stable version of the Fedora docker image. This image includes a version of glibc with the attribute __attr_access_none added to pthread_getspecific [1], the implementation of which only exists for GCC 11.X - the version included in Fedora. The attribute requires that the pointer provided in the second argument of pthread_getspecific must, if not NULL, be a pointer to a valid object. In the usage in async_die_is_recursing, (void *)1 is not valid, resulting in the error.

The fix imitates a workaround added in SELinux [2] by using the pointer to ret as the second argument to pthread_getspecific. This guaranteed non-NULL, valid pointer adheres to the current intended usage of pthread_getspecific while not triggering the build error.

[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8
[2] https://lore.kernel.org/all/[email protected]/

---
async_die_is_recursing: fix use of pthread_getspecific for Fedora

Following up on Johannes' report earlier [1], this patch fixes a compiler error in the Fedora CI build (the same issue was identified in a local developer build about a week ago [2]). This fix changes the second argument in the call to pthread_getspecific from '(void *)1' to a valid pointer, thus preventing the error in the use of __attr_access_none.

[1]
https://lore.kernel.org/git/[email protected]/
[2]
https://lore.kernel.org/git/[email protected]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1072%2Fvdye%2Ffix-fedora-build-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1072/vdye/fix-fedora-build-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1072

 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7ef5cc712a9..a82cf69e7d3 100644
@@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)
 static int async_die_is_recursing(void)
 {
 	void *ret = pthread_getspecific(async_die_counter);
-	pthread_setspecific(async_die_counter, (void *)1);
+	pthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */
 	return ret != NULL;
 }
 

base-commit: 876b1423317071f43c99666f3fc3db3642dfbe14

gitgitgadget

On Wed, Nov 3, 2021 at 6:48 PM Victoria Dye via GitGit

the SELinux workaround uses a valid global pointer though; while this won't

Yeah, I had the same thought. I think what's in the patch above is OK in practice, but it sure _feels_ wrong to store the address of an auto variable that goes out of scope.

I'm OK with it as a minimal fix, though, to get things unstuck. The commit message nicely explains what's going on, and the original (which it looks like blames to me ;) ) is pretty gross, too.

Keeping an actual counter variable would be the least-confusing thing, IMHO, but that implies allocating per-thread storage (which means having to clean it up). And we really only care about counting up to "1", so the boolean "do we have a pointer" is fine. The static variable you suggest might be a good middle ground there, and we could even use it for the comparison to make things more clear. Something like:

static int async_die_is_recursing(void)

  {
	  static int async_recursing_flag;
          void *ret = pthread_getspecific(async_die_counter);
	  pthread_setspecific(async_die_counter, &async_recursing_flag);
	  return ret == &async_recursing_flag;

}

But I don't feel that strongly either way.

-Peff

Agree it is OK as a minimal fix, but also AFAIK nothing is really stuck either, so something as simple as :

s/&ret/&async_die_counter/g

Would make it as minimal, and less likely to trigger something else in the future (I am surprised nothing warns about local variables being used out of scope).

Carlo

It's in scope when we call the function. The potential for bugs only happens because we know pthread_setspecific() is going to hold on to it. So the compiler complaining would imply that it knows the semantics of the pthread function.

-Peff

From: Victoria Dye <[email protected]>

This fix corrects an issue found in the dockerized(pedantic, fedora) CI build, first appearing after the introduction of a new version of the Fedora docker image version. This image includes a version of glibc with the attribute __attr_access_none added to pthread_setspecific [1], the implementation of which only exists for GCC 11.X - the version included in the Fedora image. The attribute requires that the pointer provided in the second argument of pthread_getspecific must, if not NULL, be a pointer to a valid object. In the usage in async_die_is_recursing, (void *)1 is not valid, causing the error.

This fix imitates a workaround added in SELinux [2] by using the pointer to the static async_die_counter itself as the second argument to pthread_setspecific. This guaranteed non-NULL, valid pointer matches the intent of the current usage while not triggering the build error.

[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8
[2] https://lore.kernel.org/all/[email protected]/

---
async_die_is_recursing: fix use of pthread_getspecific for Fedora

Following up on Johannes' report earlier [1], this patch fixes a compiler error in the Fedora CI build (the same issue was identified in a local developer build about a week ago [2]). This fix changes the second argument in the call to pthread_setspecific from '(void *)1' to a valid pointer, thus preventing the error in the use of __attr_access_none.

Changes since V1
================

* Change &ret to &async_die_counter ("dummy" argument to pthread_setspecific) so that it's using a global (rather than local) variable
* Fix typo in commit message (pthread_getspecific -> pthread_setspecific)

[1]
https://lore.kernel.org/git/[email protected]/
[2]
https://lore.kernel.org/git/[email protected]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1072%2Fvdye%2Ffix-fedora-build-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1072/vdye/fix-fedora-build-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1072

Range-diff vs v1:

1: efe0b398f54 ! 1: 7f10e09269a async_die_is_recursing: fix use of pthread_getspecific for Fedora
@@ Metadata
Author: Victoria Dye <[email protected]>

## Commit message ##
- async_die_is_recursing: fix use of pthread_getspecific for Fedora
+ async_die_is_recursing: work around GCC v11.x issue on Fedora

- Correct an issue encountered in the dockerized(pedantic, fedora) CI build,
- first appearing after the release of v36 as the latest stable version of the
- Fedora docker image. This image includes a version of glibc with the
- attribute __attr_access_none added to pthread_getspecific [1], the
+ This fix corrects an issue found in the dockerized(pedantic, fedora) CI
+ build, first appearing after the introduction of a new version of the Fedora
+ docker image version. This image includes a version of glibc with the
+ attribute __attr_access_none added to pthread_setspecific [1], the
implementation of which only exists for GCC 11.X - the version included in
- Fedora. The attribute requires that the pointer provided in the second
- argument of pthread_getspecific must, if not NULL, be a pointer to a valid
- object. In the usage in async_die_is_recursing, (void *)1 is not valid,
- resulting in the error.
+ the Fedora image. The attribute requires that the pointer provided in the
+ second argument of pthread_getspecific must, if not NULL, be a pointer to
+ a valid object. In the usage in async_die_is_recursing, (void *)1 is not
+ valid, causing the error.

- The fix imitates a workaround added in SELinux [2] by using the pointer to
- ret as the second argument to pthread_getspecific. This guaranteed
- non-NULL, valid pointer adheres to the current intended usage of
- pthread_getspecific while not triggering the build error.
+ This fix imitates a workaround added in SELinux [2] by using the pointer to
+ the static async_die_counter itself as the second argument to
+ pthread_setspecific. This guaranteed non-NULL, valid pointer matches the
+ intent of the current usage while not triggering the build error.

[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8
[2] https://lore.kernel.org/all/[email protected]/
@@ run-command.c: static NORETURN void die_async(const char *err, va_list params)
{
void *ret = pthread_getspecific(async_die_counter);
- pthread_setspecific(async_die_counter, (void *)1);
-+ pthread_setspecific(async_die_counter, &ret); /* set to any non-NULL valid pointer */
++ pthread_setspecific(async_die_counter, &async_die_counter); /* set to any non-NULL valid pointer */
return ret != NULL;
}

 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7ef5cc712a9..f40df01c772 100644
@@ -1099,7 +1099,7 @@ static NORETURN void die_async(const char *err, va_list params)
 static int async_die_is_recursing(void)
 {
 	void *ret = pthread_getspecific(async_die_counter);
-	pthread_setspecific(async_die_counter, (void *)1);
+	pthread_setspecific(async_die_counter, &async_die_counter); /* set to any non-NULL valid pointer */
 	return ret != NULL;
 }
 

base-commit: 876b1423317071f43c99666f3fc3db3642dfbe14

gitgitgadget

Thanks, this version looks good to me!

-Peff

Looks like they timed their update to disrupt us most effectively, but we are gifted with watchful eyes and competent hands ;-).

Thanks for coming up with a clearly written description and a clean fix so quickly.

Will apply.

I don't know what this would be for the "fedora" and other images, but it seems to me like the below is something we should do. This replaces "latest" with whatever "latest" currently maps onto.

I.e. I don't think it's a good thing that the carpet gets swept from under you as far as these CI images go. We could subscribe to some feed of when these images are bumped to see when to update, but having our base change from under us just leads to a waste of time for a bunch of people wondering why their CI is failing, which now they'll need to rebase on some on-list or only-in-upstream-master patch to have it "pass".

index 6ed6a9e8076..6b7dab01269 100644
@@ -7,7 +7,7 @@ env:
 
 jobs:
   ci-config:
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-20.04
     outputs:
       enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
     steps:

@@ -79,7 +79,7 @@ jobs:
   windows-build:
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
-    runs-on: windows-latest
+    runs-on: windows-2019
     steps:
     - uses: actions/checkout@v2
     - uses: git-for-windows/setup-git-for-windows-sdk@v1

@@ -97,7 +97,7 @@ jobs:
         name: windows-artifacts
         path: artifacts
   windows-test:
-    runs-on: windows-latest
+    runs-on: windows-2019
     needs: [windows-build]
     strategy:
       fail-fast: false

@@ -132,7 +132,7 @@ jobs:
     env:
       NO_PERL: 1
       GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'"
-    runs-on: windows-latest
+    runs-on: windows-2019
     steps:
     - uses: actions/checkout@v2
     - uses: git-for-windows/setup-git-for-windows-sdk@v1

@@ -178,7 +178,7 @@ jobs:
         name: vs-artifacts
         path: artifacts
   vs-test:
-    runs-on: windows-latest
+    runs-on: windows-2019
     needs: vs-build
     strategy:
       fail-fast: false

@@ -218,22 +218,22 @@ jobs:
         vector:
           - jobname: linux-clang
             cc: clang
-            pool: ubuntu-latest
+            pool: ubuntu-20.04
           - jobname: linux-gcc
             cc: gcc
-            pool: ubuntu-latest
+            pool: ubuntu-20.04
           - jobname: osx-clang
             cc: clang
-            pool: macos-latest
+            pool: macos-10.15
           - jobname: osx-gcc
             cc: gcc
-            pool: macos-latest
+            pool: macos-10.15
           - jobname: linux-gcc-default
             cc: gcc
-            pool: ubuntu-latest
+            pool: ubuntu-20.04
           - jobname: linux-leaks
             cc: gcc
-            pool: ubuntu-latest
+            pool: ubuntu-20.04
     env:
       CC: ${{matrix.vector.cc}}
       jobname: ${{matrix.vector.jobname}}

@@ -265,7 +265,7 @@ jobs:
           image: fedora
     env:
       jobname: ${{matrix.vector.jobname}}
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-20.04
     container: ${{matrix.vector.image}}
     steps:
     - uses: actions/checkout@v1

@@ -314,7 +314,7 @@ jobs:
     if: needs.ci-config.outputs.enabled == 'yes'
     env:
       jobname: Documentation
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-20.04
     steps:
     - uses: actions/checkout@v2
     - run: ci/install-dependencies.sh

Having our CI go red because the 'latest' feed changed something is probably the best "feed" to subscribe to, since we only get notified if it matters.

Better to have automation go red than for us to not realize our code doesn't work on newer platforms because our CI hasn't been updated.

Thanks,
-Stolee

I consider this a better implementation of what Æver suggested ;-)

And an incident like this is one of the reasons why I like the "CI does not stop after seeing the first problem" behaviour. In a short term, people can ignore a particular known failure and ensure they do not introduce any new ones.

Thanks.

FWIW, this is now in all my integration branches that may trigger CI tests. Again, thanks for a quick fix.

Hi Victoria,

excellent work! The patch looks very good to me. I just want to add a little context below, and thank you _so much_ for letting me sleep while you tie it all up in a neat patch.

My initial reading last night was that the none mode means that the pointer does not have to be valid, but you're right, the documentation at
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
clearly spells it out:

Unless the pointer is null the pointed-to object must exist and have at least the size as denoted by the size-index argument. The object need not be initialized. The mode is intended to be used as a means to help validate the expected object size, for example in functions that call __builtin_object_size.

Which means that yes, (void *)1 was incorrect and _had_ to be changed. The patch is therefore a fix, not a work-around (contrary to my initial impression).

So then I got puzzled by this while sleeping: why does it fail on Fedora, when it does _not_ fail in Git for Windows' SDK (where we also recently upgraded to GCC v11.x, thanks to the hard work of the MSYS2 project)?

My best guess is that my GCC (which is v11.2.0) and Fedora's GCC (which is v11.2.1 plus the usual Fedora customization) behave slightly differently with respect to that optional size-index argument:
pthread_setspecific() is essentially declared without a size-index, so I guess GCC v11.2.1 probably defaults to size-index = 1.

Got to read the whole explanation that Victoria put together.

for the warning to trigger on that gcc, you got also to have the attribute in the header as shown by the link[1] she provided, and which I am sure could be a nice thing to send toward those helpful folks of the MSYS2 project so it can be added to their WinPthread headers, as was done in glibc.

Carlo

[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8

Hi Carlo,

D'oh. Of course we do not have glibc in the Git for Windows SDK! Thanks for helping me understand this.

Ciao,
Dscho