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 crash when HOME env variable is not set #1738

Merged
merged 8 commits into from May 19, 2020

Conversation

lolgab
Copy link
Contributor

@lolgab lolgab commented Mar 6, 2020

Steps to reproduce:

  • Link any Scala Native binary using the java.lang.System object
  • unset the HOME environment variable
  • run the binary:
$ unset HOME
$ target/scala-2.11/hello-out

And it crashes with:

java.lang.NullPointerException
        at java.lang.Throwable.fillInStackTrace(Unknown Source)
        at java.util.Hashtable.put(Unknown Source)
        at java.util.Properties.setProperty(Unknown Source)
        at java.lang.System$.loadProperties(Unknown Source)
        at java.lang.System$.<init>(Unknown Source)
        at <none>._SM17java.lang.System$G4load(Unknown Source)
        at scala.sys.package$.env(Unknown Source)
        at JsonAction$class.main(Unknown Source)
        at Main$.main(Unknown Source)
        at <none>.main(Unknown Source)

@lolgab
Copy link
Contributor Author

lolgab commented Mar 7, 2020

On Mac Os X and Linux (tried with alpine on docker) it is set correctly regardless of the HOME environment variable. So this PR doesn't solve the problem. I faced the crash on a docker environment.. I don't know if we want to have a proper solution with platform dependent code, for sure I don't want my applications to suddenly crash in some environments.
As a workaround you can call:

import scala.scalanative.posix.stdlib.setenv
setenv(c"HOME", c"", 0)

as first instruction in the main.

@ekrich
Copy link
Member

ekrich commented Mar 9, 2020

Here is a discussion I found on SOF - https://stackoverflow.com/questions/2910377/get-home-directory-in-linux
Not sure without playing with this if it could be an option.

@ekrich
Copy link
Member

ekrich commented Mar 25, 2020

@lolgab Maybe some code like this? Not sure if you need to look at HOME at all.
Tested in Xcode on macOS. The only question here is POSIX ok for the core system if we are trying to get more modular?

#include <unistd.h>
#include <pwd.h> // POSIX only

char* home = getenv("HOME"); // STD C
printf("HOME: %s\n", home);    
unsetenv("HOME");
const char *homedir;

if ((homedir = getenv("HOME")) == NULL) {
    printf("HOME NULL\n");
    uid_t uid = getuid();
    struct passwd *pw = getpwuid(uid);
    homedir = pw->pw_dir;
}
printf("HOME2: %s\n", homedir);

Output:

HOME: /Users/eric
HOME NULL
HOME2: /Users/eric

@lolgab
Copy link
Contributor Author

lolgab commented Mar 25, 2020

Nice! I'll try it on Linux and update the PR if it works!

@lolgab
Copy link
Contributor Author

lolgab commented Mar 25, 2020

@ekrich I applied your suggestion! Thank you 🙏

@ekrich
Copy link
Member

ekrich commented Mar 25, 2020

Based on your JVM test, I was thinking that is what they might be doing.

@ekrich
Copy link
Member

ekrich commented Mar 30, 2020

The function you are calling (5fc1189#diff-9f67b7df156c5096453ed90792be0ad2R69) is a Scala Native wrapper function that you are proposing to remove in #1743

This is the reason I was confused. We are not using error number at all after calling struct passwd *getpwuid(uid_t); , just some logic from the wrapper. https://github.com/scala-native/scala-native/blob/master/nativelib/src/main/resources/pwd.c#L25-L30

I am unclear what should be done as the POSIX pwd needs updating too. These wrappers and struct copying perhaps were created earlier because we didn't have UInt and ULong support.

If you add the real getpwuid function in this PR and just check whether the Ptr returned is NULL or not that might be good for this PR and then the other PR can also stand as well.

ekrich
ekrich approved these changes Apr 8, 2020
Copy link
Member

@ekrich ekrich left a comment

Choose a reason for hiding this comment

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

@lolgab Looks good to me.

Copy link
Collaborator

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Would you mind rebasing on top of the latest master to make sure that changes to scalafmt do not affect this?

Thank you.

@@ -61,7 +63,14 @@ object System {
sysProps.setProperty("user.language", userLang)
sysProps.setProperty("user.country", userCountry)
}
sysProps.setProperty("user.home", getenv("HOME"))
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using

Suggested change
{
locally {

to make this more readable.

Also, consider wrapping the following code for user.dir in a similar locally {} block, since it also has a val buf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not know about locally existence! Thank you :-) I'll implement the suggested changes!

@lolgab
Copy link
Contributor Author

lolgab commented May 16, 2020

@sjrd Ready for another review round.

sjrd
sjrd approved these changes May 19, 2020
@sjrd sjrd merged commit b49e74e into scala-native:master May 19, 2020
1 check passed
@lolgab lolgab deleted the fix-home-env-bug branch May 19, 2020 14:18
ekrich pushed a commit to ekrich/scala-native that referenced this pull request May 21, 2021
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

3 participants