[PATCH] D77696: [Darwin] Teach `AtosSymbolizerProcess` to work on a copy of the environment.

[Julian Lettner via Phabricator via llvm-commits]

yln added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:101
+    CHECK_LE(entries_to_copy, kMaxNumEnvPEntries);
+    internal_memcpy(custom_env_, current_env, entries_to_copy * sizeof(uptr));
+    // Null terminate the array.
----------------
delcypher wrote:
> kubamracek wrote:
> > Is it actually correct to not do a "deep copy" and only copy the pointers? I mean, who "owns" these strings? Are they guaranteed to live forever?
> Darwin's Libc setenv implementation owns these strings. When users call `setenv(...)` it makes copies of them which it keeps track of. There are technically some circumstances where this is unsafe.
> 
> * setenv() is called on an existing environment variable in the environment array. In some cases Libc will call realloc() on the pointer to the existing string. In the right circumstances this could potentially leave a dangling pointer in our copy of the environment array.
> * unsetenv() is called on an existing environment variable in the environment array. This will cause the string to be free'd. Again this could leave a dangling pointer in our copy of the environment array.
> 
> So no. They are not guaranteed to live for ever.
> 
> Possible solutions:
> 
> * Do nothing.
> * Implement interceptors to prevent environment changes being made while symbolizer process is being launched. Technically this will never be sufficient because there are APIs to directly access the environment array directly.
> 
> Deep copying the strings on its own is actually not a good solution **because it suffers exactly from the same problem we have already**. If the environment gets changed while we're in the middle of copying the strings we could dereference free'd memory. 
I am assuming that `posix_spawn` makes a deep copy of the passed-in environment when it starts the new process.  Dan, do you know if this is indeed the case?

If so, I think we should ignore these issues for the sake of simplicity because the windows for things to go wrong (time between `StartSymbolizerSubprocess()` and `posix_spawn`) is small.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77696/new/

https://reviews.llvm.org/D77696