Implement a thread safe version of rcutils_get_env

[mikaelarguedas]

While ensuring thread safety may be impossible we could improve robustness by using thread local storage for the result of the rcutils_get_env function.
This would also be a good time to refactor every part of the code reading environment variables to use this function
Related ticket ros2/rcl#97

[wjwwood]

It would be possible to make it thread-safe with a global mutex, but perhaps it would be better to avoid doing that in rcutils_getenv, but protect higher level functions that use it using a mutex. For example, maybe the "right thing" to do is to just mark rcl_create_node as not thread safe, and let rclpy/rclcpp/rclc make it thread safe.

It (probably) wouldn't hurt to also have a thread-safe getenv here, but I'm not sure it should be used in create_node, for example. It would potentially hurt the portability of this library to start requiring mutexes.

[clalancette]

Looking at rcutils_get_env() in more detail, it looks like there are at least 2 current problems with the thread-safety. The first problem is obvious and has to do with the global __env_buffer[] on Windows, which is clearly not thread-safe. The second problem is a bit more subtle. Since getenv()/getenv_s() both operate on the statically allocated environment for the program, it could be the case that the string is changing (via putenv()/setenv()) while reading it. The Windows documentation says as much.

One way to tackle the first problem is to make the contract for rcutils_get_env() such that the string returned is malloc'ed, and responsibility for freeing the memory falls on the caller. This also mitigates much of the second problem, though doesn't completely remove it. We could completely mitigate the second problem by adding the mutex, but we would also have to be careful about adding a mutex to any putenv()/setenv() callers. Since the only callers of putenv()/setenv() are in either tests, or the OpenSplice implementation, my feeling is that we should change the contract here to a malloc'ed string, fix the callers, and go from there. If we start mucking with the environment more in the future, we can add the mutex then. Thoughts on that approach?

[wjwwood]

I'd rather just make it not thread-safe, but returning a malloc'ed string would be ok I guess. I was think that you could just make the __env_buffer thread local (has it's own issues avoiding malloc).

If it does return a copy of the environment variable's value, it should take an allocator.

---

Really, actually what would be best is if the reading of environment variables and checking of the filesystem (used in various stuff but more recently in the security stuff) was injected as a function pointer into these functions like "create node". That way we could say this is thread-safe so long as the "hook functions" you pass in are also thread-safe, then we could mark the security ones not thread-safe. The caller could then decide how to enforce thread-safety. This has more to do with supporting portability with systems that may not have things like env vars or filesystems (like to OS-less micros).

So realistically, it would probably just be better to mark the functions like "create node" as non thread-safe.