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

Fix #166 On Unix-like OS, use unsafePerformIO (lookupEnv "TERM") #167

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

mpilgrem
Copy link
Collaborator

Relying on CI to test.

@mpilgrem mpilgrem force-pushed the fix166 branch 2 times, most recently from 36dd36f to 23e0edf Compare April 21, 2024 22:24
@Bodigrim
Copy link
Contributor

There is also hSupportsANSIColor, which also reads environment variables.

@Bodigrim
Copy link
Contributor

(As I said in #150 (comment), I think we should just define hSupportsANSIColor = hSupportsANSI - and maybe this PR is a good opportunity to do so)

@mpilgrem
Copy link
Collaborator Author

@Bodigrim, on hSupportsANSIColor, I did look at the source code for System.Environment.getEnvironment and I could not see that it used C function getenv(). It has:

foreign import ccall unsafe "__hscore_environ"
  getEnvBlock :: IO (Ptr CString)

Is that also problematic?

@Bodigrim
Copy link
Contributor

Is that also problematic?

I think so. The C definition is

#if !HAVE_DECL_ENVIRON
extern char** environ;
#endif
INLINE char **__hscore_environ(void) { return environ; }

Now if one thread got Ptr CString and another thread executed setenv, extending the environment and causing reallocation of char** environ elsewhere, the original pointer can be freed, causing the first thread to segfault.

Quoting Setenv is not Thread Safe:

As a final problem, environ is a NULL-terminated array of pointers (char**) that an application can read and assign to (see definition in POSIX.1-2017). This is how applications can iterate over all environment variables. Accesses to this array are not thread-safe.

@mpilgrem
Copy link
Collaborator Author

@Bodigrim, now with the same approach to hSupportsANSIColor.

Copy link
Contributor

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mpilgrem mpilgrem merged commit 660b10b into master Apr 23, 2024
23 checks passed
@mpilgrem mpilgrem deleted the fix166 branch April 23, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants