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

On Wed, Mar 9, 2022 at 5:26 PM brian m. carlson
[off-list ref] wrote:

On 2022-03-09 at 23:03:14, Neeraj Singh via GitGitGadget wrote:

quoted

From: Neeraj Singh <redacted>

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.