Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_wputenv() memory semantics #1425

Open
adamyg opened this issue Mar 18, 2025 · 9 comments
Open

_wputenv() memory semantics #1425

adamyg opened this issue Mar 18, 2025 · 9 comments
Labels
bug CRTL C run-time library

Comments

@adamyg
Copy link

adamyg commented Mar 18, 2025

The windows UCRT copies the buffer that's passed to _[w]putenv, whereas OW20 assumes the same semantics as putenv, being ownership of the storage.

Interfaces utilize const interfaces, compared to putenv(), one could imply non ownership.

  int putenv( char *envstring ); 

  int _putenv( const char *envstring ); 
  int _wputenv( const wchar_t *envstring );

One example usage, see glib g_setenv() assumes copy semantics g_setenv / G_OS_WIN32

  _wputenv (wassignment); 
  g_free  (wassignment);  

Another example of this assumption python issue 39406.

UCRT Source Reference, SDK-19041:

C:/Program Files (x86)/Windows Kits/10/Source/10.0.19041.0/ucrt/env/putenv.cpp

OWC20 clearenv() may release the assigned storage clearenv, whereas OWC19 wont. Updates assume ownership and realloc addenv/waddenv. Both can result in runtime errors.

Note:
Believe the realloc selection ignore the WIDECHAR condition as it references _RWD_env_mask unconditionally.

@jmalak
Copy link
Member

jmalak commented Mar 18, 2025

I am not understanding your issue.
I don't see any problem, OW 2.0 define 3 corresponding functions

_WCRTLINK extern int _wputenv( const wchar_t *__string ); // MS specific
_WCRTLINK extern int _putenv( const char *__string ); // MS naming for POSIX functions
_WCRTLINK extern int putenv( const char *__string );

Any of this is not covered by C standard
only POSIX define putenv as
int putenv( char *__string );

We can discuse compatibility with POSIX, but OW use historicaly old MS syntax and it doesn't case issue with POSIX code.
It was not changed to hold OW backward compatibility and we doesn't plan to change this prototype.

If we are talking about clearenv then we talk about following class of functions.

Only getenv is covered by C standard and POSIX.
_WCRTLINK extern char *getenv( const char *__name );

setenv and unsetenv is POSIX standard
_WCRTLINK extern int setenv( const char *__name, const char *__newvalue, int __overwrite );
_WCRTLINK extern int unsetenv( const char *__name );

clearenv is not covered by any standard.
I think it is Linux specific (OW has some implementation)
_WCRTLINK extern int clearenv( void );

Please specify more clearly what problem is.
Is it about implementation of some functions?

@jmalak
Copy link
Member

jmalak commented Mar 18, 2025

Take into account there are a few CRTL configuration parameters (in bld/lib_misc/variety.h) which handle environment processing code.

#if !defined( __UNIX__ ) && !defined(__RDOS__) && !defined(__RDOSDEV__) && !defined( __NETWARE__ )
    #define CLIB_USE_MBCS_TRANSLATION
#endif
#if !defined( __UNIX__ ) && !defined( __RDOS__ ) && !defined( __RDOSDEV__ )
    #define CLIB_USE_OTHER_ENV
#endif
#if defined( __NT__ ) || defined( __RDOS__ ) || defined( __RDOSDEV__ )
    #define CLIB_UPDATE_OS_ENV
#endif

@adamyg
Copy link
Author

adamyg commented Mar 18, 2025

Main issue is inconsistent memory semantics with the Windows ucrt for _[w]putenv(), assumed by 3rd libraries,
creating portability issues between owc and other windows tool-chains; g_setenv / G_OS_WIN32 was one example, _wputenv is assumed to copy the value.

Can you review the realloc logic within waddenv, reference, should _RWD_env_mask is be referenced?

+ #ifndef __WIDECHAR__
                 old_val = _RWD_env_mask[index] ? envp[index] : NULL;
+ #else
+                old_val = envp[index]
+ #endif

@jmalak
Copy link
Member

jmalak commented Mar 18, 2025

Sorry our CRTL is not replacement of Microsoft UCRT.
We use C standard or POSIX definitions for CRTL functions and some aliases for MS naming.
But you rely on MS ucrt internal implementation which is not our rule.
By example our CRTL support Windows NT from version 3.1 up to Windows 11, but without UCRT.
If you relate on bug in our implementation be explicit, but argument of UCRT or other implementation is useless.

You must specify platform and function and problem, because some implementations can be very OS specific.
Espetialy environment related processing is one of this area.
If I understand you correctly then all what you report is WIN32 platform related only.

If you have a fix for your problem then you can create and submit PR.

@jmalak
Copy link
Member

jmalak commented Mar 18, 2025

To use _RWD_env_mask, it is flag array which flag environment entries globaly it has nothing to do with wide version of environment.

@adamyg
Copy link
Author

adamyg commented Mar 18, 2025

Shall continue investigating, correct i was referencing win32 semantics.

FYI on reviewing the original MSVC 6/VC98 (1998) runtime, _wputenv() copies the value, which represents one the first published implementations; the semantics have not changed since,

@jmalak
Copy link
Member

jmalak commented Mar 18, 2025

But there is no semantic difference in wputenv(), OW and MS use const char pointer for item that owner is always caller.
There is only difference in implementation.

@adamyg
Copy link
Author

adamyg commented Mar 18, 2025

OWC _wputev() only references the passed value, whereas the original msvc win32 implementation copies.

Output from simple test application, yet owc is undefined:

MSVC:
1: MYVAR=XXX 
2: MYVAR=XXX

OWC:
1: MYVAR=XXX 
2: MYVAR=

Source:

#include <stdlib.h>
#include <wchar.h>

static void dump(unsigned call)
{
    wprintf(L"%u: MYVAR=%s\n", call, _wgetenv(L"MYVAR")); 
}

static void set()
{
    wchar_t buffer[] = {L"MYVAR=XXX"};
    _wputenv(buffer);  // == buffer locally scoped
    dump(1);
}

int
main()
{
    set();
    dump(2);
    return 0;
}

@jmalak
Copy link
Member

jmalak commented Mar 18, 2025

It is implementation bug and need to fix it.
but it can be also bug in _wgetenv that need to check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug CRTL C run-time library
Projects
None yet
Development

No branches or pull requests

2 participants