Re: [PATCH v5 1/5] wrapper: move inclusion of CSPRNG headers the wrapper.c file

"Neeraj Singh via GitGitGadget" [off-list ref] 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 [off-list ref] wrote:

"Neeraj Singh via GitGitGadget" [off-list ref] writes:

quoted

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?

quoted

-/*
- * 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