So I think this is the wrong way to go. The problem is that on Linux/macOS, this would return a string that the caller does not free, and on Windows it would return a string that the caller does free. That's much too difficult to get right, so we should make it consistent.

But let's back up a second. There are three levels of thread-safety that are currently being violated by this current version of rcutils_get_env. The first level is that, on Windows, we are returning a pointer to a global array, so if two threads try to get an environment variable at the same time, they'll stomp all over each other. The second level is that, on all platforms, if a thread gets an environment variable, and then somewhere down the line something else calls setenv, that can cause the pointer to be invalidated. The third level of thread safety being violated is that if something calls {un}setenv at the same time that something else is calling getenv, a race and/or crash can occur.

With that being said, I propose that we split this into three different patches:

1. We remove the global Windows buffer completely, and switch from getenv_s to getenv on Windows. That will make the behavior consistent between Linux, macOS, and Windows, and won't cause us to change this function's signature. This doesn't cause us to be any less thread-safe then we are on Linux and macOS now, and improves thread safety for Windows. (I'll suggest we just modify this simple PR to accomplish this, but we can open a new one as well).
2. Later do a follow-up where we getenv and copy the string for all platforms. This will solve the second thread-safety issue, since a later setenv won't affect outstanding pointers. That will cause us to change the signature and the contract of rcutils_get_env so we can have an allocator, and so downstream users free the buffer we give them.
3. Even later, do an audit of any places that call rcutils_get_env and setenv, and add locking as appropriate.

Doing it this way addresses our most pressing thread-safety issue (number 1), while leaving the less likely ones to follow-up later.

Does this make sense? @hidmic @ivanpauno @wjwwood thoughts?

That sounds right to me.

@clalancette just wanted to point out that getenv is deprecated so, should we just go ahead with 2 and 3 instead?

@clalancette just wanted to point out that getenv is deprecated so, should we just go ahead with 2 and 3 instead?

Personally, I'd still go with using the deprecated getenv for Windows (with appropriate changes to silence the compiler warnings). If you want to go straight for 2, go for it, but I warn you that it will probably be complicated and span several repositories.

Sounds good to me, and I agree with @clalancette that option 1 is the first thing to do.
The deprecated warning can be silenced using pragma warning appropriately.

This note should be above the line it talks about, and should be something more like:

Sure, will update this and create a new issue

@clalancette do we want to close this and create a new issue or just update it with these discussions?

Let's close #30 (which will happen just by merging this PR) and open a new one.

CI is showing unknown pragmas from GCC and clang. You'll need to wrap this in an #ifdef _WIN32.

I've updated this bit and the comment

@suyash458 aren't we consciously ignoring that deprecation? Why keeping a TODO?

I asked for that. It's mostly so that we can make the even better (but bigger) change in step 2 in #237 (comment) . At that point, we most likely will switch back to getenv_s.