On Friday 18 April 2025 21:27:06 Lasse Collin wrote:
> On 2025-04-18 Pali Rohár wrote:
> > On Saturday 25 January 2025 18:17:23 Lasse Collin wrote:
> > > On 2025-01-25 Pali Rohár wrote:
> > > > On Saturday 25 January 2025 12:39:19 Lasse Collin wrote:
> > > > > Other things in libmingwex seem friendly to Win9x still. With a
> > > > > quick search with grep, I only spotted one extremely minor
> > > > > thing: getopt.c calls GetEnvironmentVariableW to read
> > > > > POSIXLY_CORRECT. That function exists on Win98 as a stub so the
> > > > > program still runs. It just won't obey the POSIXLY_CORRECT
> > > > > environment variable on Win9x, which is not a problem at all.
> > > > >
> > > >
> > > > IMHO this is a bug. We should use getenv() in POSIX/CRT functions
> > > > and not the GetEnvironmentVariable[AW]() because the WinAPI
> > > > function does not address things like direct modification of 3rd
> > > > main argument env, or similar thing which is used in POSIX
> > > > applications. There is also _enviroment symbol (or function which
> > > > returns pointer to this symbol) which can be and also is used by
> > > > windows (msvcrt) applications.
> > >
> > > I understand what you mean. Luckily POSIXLY_CORRECT is such an
> > > environment variable that there's no problem *in practice*. It's
> > > highly unlikely that the value would be changed after main() has
> > > been called.
> > >
> > > getopt.c used to call getenv(). It was changed in 2020 in the commit
> > > 8917aca09469 ("crt: use GetEnvironmentVariableW in getopt"). I
> > > suppose there's no perfect solution but the current way shouldn't
> > > cause any problems in practice. In some other place than getopt.c
> > > it could be different.
> >
> > In attachment is my attempt to fix this problem. Could you look at it?
> > getenv() is replaced by the getenv_s() which is implemented via
> > GetEnvironmentVariableA() for UWP builds.
>
> I didn't test, but I read the code fairly carefully. It looks good,
> although one thing I don't understand: if i386 and x64 getenv_s is
> the emulated version with MSVCRT and old CRTs, isn't the problem in
> getopt() still there when using these CRTs (changes to environment
> won't be detected after process startup)?
It should not be a problem. Implementation of emulated version of
getenv_s() for older CRTs uses getenv(), which we know that is correct.
And for msvcrt.dll builds it uses msvcrt_or_emu_glue.h which dynamically
use system version of getenv_s() if available in the msvcrt.dll.
> With getenv(), the problem
> wouldn't occur with old CRTs. If so, then the commit message in the
> patch 4 could mention it. As I wrote in the earlier email, I think the
> problem is extremely minor with the POSIXLY_CORRECT env var, so if the
> fix only works with newer CRTs, it's perfectly fine.
Fix should work with all CRT versions (older and new). Both getenv() and
getenv_s() uses environment from CRT storage (the correct one storage).
> Below are a minor few extra comments.
>
>
> (1)
> It could be nice to include an explanation in the commit message why
> getenv() is kept as a stub in winstorecompat when adding getenv_s(). I
> suppose it's because getenv() would need a static or even thread-local
> buffer to hold the result.
I did not touch winstorecompat getenv implementation. But basically it
is not possible to implement getenv in winstorecompat. Or at least I do
not know how. WinAPI does not provide a function which can return the
pointer to specific ENV variable.
>
> (2)
> getenv_s() is in C11 Annex K but MS had it earlier already. As
> expected, your code matches MS docs, so all is good. It's just curious
> that Annex K and MS seem to differ slightly:
>
> - Annex K allows pReturnValue to be NULL while MS docs say it results
> in EINVAL.
>
> - Annex K possibly means that *pReturnValue doesn't include the
> terminating null character while MS' docs (including the example
> code) say that the null char is counted.
C11 Annex K differs from MS implementation and whole mingw-w64 provides
only MS API. It does not provide Annex K. Also usage of Annex K by
application requires defining some macros before including C std headers
and then checking if some other macro is defined. That is because C11
Annex K is optional. And I doubt that there are requests for Annex K in
mingw-w64.
>
> (3)
> MS docs say that errno is set for the EINVAL cases. It doesn't say it
> about ERANGE.
It says, see:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-s-wgetenv-s
"Also, if the buffer is too small, these functions return ERANGE."
> I see your comments in the code. I suppose you have tested
> that errno really isn't set by MS CRTs after the EINVAL checks. It just
> feels weird.
Yes, I checked it. But it is how it works. All these MS functions have
as a return value those ESOMETHING constants and these functions do not
set errno on error at all.
The only errno which is set is EINVAL when the parameter validation
fails, and in this case these MS _s functions calls also invalid
parameter handler. But mingw-w64 does not provide wrappers for calling
invalid parameter handler and Dr. Watson yet.
>
> (4)
> Most of winstorecompat is under the MIT license instead of being public
> domain. Maybe the new file in winstorecompat should be MIT-licensed too.
>
> (There already are multiple copyright/author notice variants above the
> MIT license text in the src/*.c files. To legally distribute binaries
> that link against static winstorecompat, one needs to extract all
> variants of those notices and make them available with the binaries...)
I have not problem with MIT license. If there is a request for it I can
add it into header.
>
> (5)
> The commit message of patch 3 has a typo:
>
> CRT storage and and process block any be out of the sync,
> ^^^ may
>
> --
> Lasse Collin
Ok, I will fix both duplicate "and" word and missing "may".
_______________________________________________
Mingw-w64-public mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
