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)? 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.
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.
(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.
(3)
MS docs say that errno is set for the EINVAL cases. It doesn't say it
about 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.
(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...)
(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
_______________________________________________
Mingw-w64-public mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
