rcutils icon indicating copy to clipboard operation
rcutils copied to clipboard

Implement more stringent thread-safetyness in `rcutils_getenv`

Open clalancette opened this issue 4 years ago • 2 comments

There's a lot of detail in https://github.com/ros2/rcutils/pull/237#discussion_r408151034 , but to summarize the situation we are now in:

As of https://github.com/ros2/rcutils/pull/237, on all platforms rcutils_get_env is thread-safe for simultaneously getting environment variables from separate threads. It is currently unsafe in the following cases:

  1. Getting an environment variable, holding onto the pointer, and then having a later method (in the same or different thread) call setenv. In that case, the pointer may be invalidated, but there is no way of knowing.
  2. Getting an environment variable in one thread while setting an environment variable from a separate thread at the same time (this is a well-known limitation of glibc, for instance).

The first issue can be solved by changing the contract of rcutils_get_env to take an allocator, allocate space, copy the contents of the environment variable into that space, and then having the caller free the memory when they are done.

The second issue can be solved by adding locking around getting environment variables and setting environment variables.

clalancette avatar Apr 27 '20 13:04 clalancette

The first issue can be solved by changing the contract of rcutils_get_env to take an allocator, allocate space, copy the contents of the environment variable into that space, and then having the caller free the memory when they are done.

:+1:

The second issue can be solved by adding locking around getting environment variables and setting environment variables.

Client code, or the libraries it pulls in, can always call setenv or putenv avoiding that lock. So I don't think we can do much about that.

hidmic avatar Apr 30 '20 13:04 hidmic

Client code, or the libraries it pulls in, can always call setenv or putenv avoiding that lock. So I don't think we can do much about that.

Yeah, that's a good point. "Solved" is the wrong term here; "mitigated in certain circumstances" is more correct. Luckily, doing setenv/putenv isn't a common operation.

clalancette avatar Apr 30 '20 14:04 clalancette