| Message ID | 685b1db888079c83573cfd984ae64f46284544af.1646866998.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | A design for future-proofing fsync() configuration | expand |
"Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes: > Including NTSecAPI.h in git-compat-util.h causes build errors in any > other file that includes winternl.h. That file was included in order to > get access to the RtlGenRandom cryptographically secure PRNG. This > change scopes the inclusion of all PRNG headers to just the wrapper.c > file, which is the only place it is really needed. It is true that wrapper.c is the only thing that needs these headers included as part of its implementation detail of csprng_bytes(), and I think I very much like this change for that reason. Having said that, if it true that including these two header files in the same file will lead to compilation failure? That sounds like either (1) they represent two mutually incompatible APIs that will cause breakage when code that use them, perhaps in two separate files to avoid compilation failures, are linked together, or (2) these system header files are simply broken. Or something else? > -/* > - * Including the appropriate header file for RtlGenRandom causes MSVC to see a > - * redefinition of types in an incompatible way when including headers below. > - */ > -#undef HAVE_RTLGENRANDOM This comment hints it is more like (1) above? A type used in one part of the system is defined differently in other parts of the system? I cannot imagine anything but bad things happen when a piece of code uses one definition of the type to declare a function, and another piece of code uses the other definition of the same type to declare a variable and passes it as a parameter to that function. I do not know this patch makes the situation worse, and I am not a Windows person with boxes to dig deeper to begin with. Hence I do not mind the change itself, but justifying the change primarily as a workaround for some obscure header type clashes on a single system leaves a bad taste. If the first sentence weren't there, I wouldn't have spent this many minutes wondering if this is a good change ;-)
On Wed, Mar 9, 2022 at 3:29 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > Including NTSecAPI.h in git-compat-util.h causes build errors in any > > other file that includes winternl.h. That file was included in order to > > get access to the RtlGenRandom cryptographically secure PRNG. This > > change scopes the inclusion of all PRNG headers to just the wrapper.c > > file, which is the only place it is really needed. > > It is true that wrapper.c is the only thing that needs these headers > included as part of its implementation detail of csprng_bytes(), and > I think I very much like this change for that reason. > > Having said that, if it true that including these two header files > in the same file will lead to compilation failure? That sounds like > either (1) they represent two mutually incompatible APIs that will > cause breakage when code that use them, perhaps in two separate > files to avoid compilation failures, are linked together, or (2) > these system header files are simply broken. Or something else? > > > -/* > > - * Including the appropriate header file for RtlGenRandom causes MSVC to see a > > - * redefinition of types in an incompatible way when including headers below. > > - */ > > -#undef HAVE_RTLGENRANDOM > > This comment hints it is more like (1) above? A type used in one > part of the system is defined differently in other parts of the > system? I cannot imagine anything but bad things happen when a > piece of code uses one definition of the type to declare a function, > and another piece of code uses the other definition of the same type > to declare a variable and passes it as a parameter to that function. > > I do not know this patch makes the situation worse, and I am not a > Windows person with boxes to dig deeper to begin with. Hence I do > not mind the change itself, but justifying the change primarily as a > workaround for some obscure header type clashes on a single system > leaves a bad taste. If the first sentence weren't there, I wouldn't > have spent this many minutes wondering if this is a good change ;-) This is (2), these system header files are simply broken. I've been looking deeper into why, but haven't bottomed out yet. Thanks, Neeraj
On 2022-03-09 at 23:03:14, Neeraj Singh via GitGitGadget wrote: > From: Neeraj Singh <neerajsi@microsoft.com> > > Including NTSecAPI.h in git-compat-util.h causes build errors in any > other file that includes winternl.h. That file was included in order to > get access to the RtlGenRandom cryptographically secure PRNG. This > change scopes the inclusion of all PRNG headers to just the wrapper.c > file, which is the only place it is really needed. We generally prefer to do system includes in git-compat-util.h because it allows us to paper over platform incompatibilities in one place and to deal with the various ordering problems that can happen on certain systems. It may be that Windows needs additional help here; I don't know, because I don't use Windows. I personally find it unsavoury that Windows ships with multiple incompatible header files like this, since such problems are typically avoided by suitable include guards, whose utility has been well known for several decades. However, if that's the case, let's move only the Windows changes there, and leave the Unix systems, which lack this problem, alone. It would also be helpful to explain the problem that Windows has in more detail here, including any references to documentation that explains this incompatibility, so those of us who are not Windows users can more accurately reason about why we need to be so careful when including header files there and why this is the best solution (and not, say, providing our own include guards in a compat file).
On Wed, Mar 9, 2022 at 5:26 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > On 2022-03-09 at 23:03:14, Neeraj Singh via GitGitGadget wrote: > > From: Neeraj Singh <neerajsi@microsoft.com> > > > > Including NTSecAPI.h in git-compat-util.h causes build errors in any > > other file that includes winternl.h. That file was included in order to > > get access to the RtlGenRandom cryptographically secure PRNG. This > > change scopes the inclusion of all PRNG headers to just the wrapper.c > > file, which is the only place it is really needed. > > We generally prefer to do system includes in git-compat-util.h because > it allows us to paper over platform incompatibilities in one place and > to deal with the various ordering problems that can happen on certain > systems. > > It may be that Windows needs additional help here; I don't know, because > I don't use Windows. I personally find it unsavoury that Windows ships > with multiple incompatible header files like this, since such problems > are typically avoided by suitable include guards, whose utility has been > well known for several decades. However, if that's the case, let's move > only the Windows changes there, and leave the Unix systems, which lack > this problem, alone. > > It would also be helpful to explain the problem that Windows has in more > detail here, including any references to documentation that explains > this incompatibility, so those of us who are not Windows users can more > accurately reason about why we need to be so careful when including > header files there and why this is the best solution (and not, say, > providing our own include guards in a compat file). > -- > brian m. carlson (he/him or they/them) > Toronto, Ontario, CA I wasn't able to find any documentation from other people who hit this problem. The root cause is that NtSecAPI.h has a typedef like this: ``` #ifndef _NTDEF_ typedef LSA_UNICODE_STRING UNICODE_STRING, *PUNICODE_STRING; typedef LSA_STRING STRING, *PSTRING ; #endif ``` That's not really appropriate since NtSecAPI.h isn't the correct place to define this core primitive NT type. It should be including winternl.h or a similar header. I'll update the change to only move the Windows definition to the .c file.
diff --git a/compat/winansi.c b/compat/winansi.c index 936a80a5f00..3abe8dd5a27 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -4,11 +4,6 @@ #undef NOGDI -/* - * Including the appropriate header file for RtlGenRandom causes MSVC to see a - * redefinition of types in an incompatible way when including headers below. - */ -#undef HAVE_RTLGENRANDOM #include "../git-compat-util.h" #include <wingdi.h> #include <winreg.h> diff --git a/git-compat-util.h b/git-compat-util.h index 876907b9df4..a25ebb822ee 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -197,12 +197,6 @@ #endif #include <windows.h> #define GIT_WINDOWS_NATIVE -#ifdef HAVE_RTLGENRANDOM -/* This is required to get access to RtlGenRandom. */ -#define SystemFunction036 NTAPI SystemFunction036 -#include <NTSecAPI.h> -#undef SystemFunction036 -#endif #endif #include <unistd.h> @@ -273,12 +267,6 @@ #else #include <stdint.h> #endif -#ifdef HAVE_ARC4RANDOM_LIBBSD -#include <bsd/stdlib.h> -#endif -#ifdef HAVE_GETRANDOM -#include <sys/random.h> -#endif #ifdef NO_INTPTR_T /* * On I16LP32, ILP32 and LP64 "long" is the safe bet, however diff --git a/wrapper.c b/wrapper.c index 3258cdb171f..2a1aade473b 100644 --- a/wrapper.c +++ b/wrapper.c @@ -4,6 +4,20 @@ #include "cache.h" #include "config.h" +#ifdef HAVE_RTLGENRANDOM +/* This is required to get access to RtlGenRandom. */ +#define SystemFunction036 NTAPI SystemFunction036 +#include <NTSecAPI.h> +#undef SystemFunction036 +#endif + +#ifdef HAVE_ARC4RANDOM_LIBBSD +#include <bsd/stdlib.h> +#endif +#ifdef HAVE_GETRANDOM +#include <sys/random.h> +#endif + static int memory_limit_check(size_t size, int gentle) { static size_t limit = 0;