Issue2534

classification
Title: os.getlogin() returns a wrong user or returns an exception
Type: behaviour Severity: normal
Components: Library Versions: Jython 2.7
Milestone: Jython 2.7.1
process
Status: pending Resolution: invalid
Dependencies: Superseder:
Assigned To: stefan.richthofer Nosy List: jamesmudd, pguermo, stefan.richthofer, zyasoft
Priority: normal Keywords: console

Created on 2016-11-28.10:57:59 by pguermo, last changed 2017-02-24.11:31:16 by stefan.richthofer.

Messages
msg10992 (view) Author: Guermonprez Patrick (pguermo) Date: 2016-11-28.10:57:58
Hello,

I'm migrating from Jython 2.2.1 to 2.7.0 and I face a problem with the method os.getlogin. 

It was working fine in 2.2.1 but using 2.7.0, it returns either an exception, either a wrong login.

    user = os.getlogin()
	at org.python.core.PyString.<init>(PyString.java:62)
	at org.python.core.PyString.<init>(PyString.java:70)
	at org.python.modules.posix.PosixModule.getlogin(PosixModule.java:510)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:497)

java.lang.IllegalArgumentException: java.lang.IllegalArgumentException: Cannot create PyString from null

Thanks,
Patrick
msg11100 (view) Author: James Mudd (jamesmudd) Date: 2017-02-19.20:00:54
I have been having a look at this issue as I am also seeing this. But I think it is OS dependent. It seems to work ok on RHEL6 but it is failing on Ubuntu 16.04 64 bit.

Jython 2.7.1b3 (, Feb 19 2017, 19:57:56) 
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_121
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.getlogin()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
	at org.python.core.PyString.<init>(PyString.java:62)
	at org.python.core.PyString.<init>(PyString.java:70)
	at org.python.modules.posix.PosixModule.getlogin(PosixModule.java:528)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)

java.lang.IllegalArgumentException: java.lang.IllegalArgumentException: Cannot create PyString from null
>>> 

I was running a just built version of jython master.

Could you provide some more details of the OS and java you were using.
msg11102 (view) Author: James Mudd (jamesmudd) Date: 2017-02-20.14:06:11
I have also tested this now on RHEL6  and it works ok

Jython 2.7.1b3 (, Feb 20 2017, 12:05:03) 
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_101
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.getlogin()
'james'
>>> 

So i'm sure this is something to do with the jffi calls to the OS. Would be interesting to test other platforms, I will try Windows when I get a chance
msg11103 (view) Author: Stefan Richthofer (stefan.richthofer) Date: 2017-02-20.15:44:58
I see this too, but it even fails in CPython for me:

Python 2.7.12 (default, Nov 19 2016, 06:48:10) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.getlogin()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 2] No such file or directory
>>> 

Did you check if os.getlogin works in CPython?

Then I found the following: https://mail.python.org/pipermail/python-bugs-list/2002-July/012691.html

(Yes it is rather old, but it seems to cover the error I observe.)


So, given that os.getlogin is supposed to be a thin wrapper for https://linux.die.net/man/3/getlogin (?) Jython's behavior might actually be right if that call fails due to other reasons. Only the error message might better be more sophisticated.

That said, it would be trivial to build some fallbacks into PosixModule.getlogin, e.g.:

    public static PyObject getlogin() {
        String login = posix.getlogin();
        if (login == null) {
            login = System.getenv("LOGNAME");
        }
        if (login == null) {
            login = System.getenv("USER");
        }
        if (login == null) {
            login = System.getProperty("user.name");
        }
        return new PyString(login);
    }

originally:
    public static PyObject getlogin() {
        return new PyString(posix.getlogin());
    }
(posix is JNR's posix-wrapper)

However os.getlogin shall only work on terminal/live console if at all, which would not hold then any more. So code using this to detect console-mode would break (bad way for detection anyway?).
So maybe we should better keep it that way, like a thin wrapper and apply such fallbacks on Python-level, i.e. os.getenv('LOGNAME'), os.getenv('USER'), os.getenv('USERNAME') (windows)

I am actually undecided, because I favor portable wrappers and guess these fallbacks could prevent failures in naive real-world existing code.

Opinions?
msg11104 (view) Author: James Mudd (jamesmudd) Date: 2017-02-20.21:09:37
Your right it fails for me in CPython as well in the same way on Ubuntu 16.04 64 bit

Python 2.7.12 (default, Nov 19 2016, 06:48:10) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.getlogin()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 2] No such file or directory
>>> 

This is discussed in the Python docs https://docs.python.org/2/library/os.html#os.getlogin where they suggest using the environment variable 'LOGNAME' or "pwd.getpwuid(os.getuid())[0]" both appear to work correctly in Jython, so maybe these are more reliable approaches which should be used instead.

Jython 2.7.1b3 (, Feb 19 2017, 19:57:56) 
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_121
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.getenv('LOGNAME')
'james'
>>> import pwd
>>> pwd.getpwuid(os.getuid())[0]
'james'
>>> 

So I think this issue should be closed as the Jython behaviour seems consistent with CPython? I don't really like the idea of adding the fallbacks to PosixModule think it's quite clean as it is.
msg11105 (view) Author: Stefan Richthofer (stefan.richthofer) Date: 2017-02-20.21:49:46
Maybe you're right. Let me make one more point for the fallbacks:
One could say that Jython's platform is Java and so the wrapper can be a bit more portable.

On the other hand in strict sense the posix module shouldn't be available then anyway: Java isn't posix, is it?

I looked into JNR, which backs the posix module and they also have the option to use a purely Java-backed posix. So in that sense Java actually is posix. Its getlogin wrapper is in turn bakced by getlogin() in https://github.com/jnr/jnr-posix/blob/master/src/main/java/jnr/posix/JavaLibCHelper.java, which plainly uses System.getProperty("user.name").

If we changed
private static final POSIX posix = POSIXFactory.getPOSIX(posixHandler, true);
into
private static final POSIX posix = POSIXFactory.getJavaPOSIX(posixHandler, true);

this would force using the Java backend, yielding the same behavior as the proposed fallbacks.
However, compared to this I would still prefer the fallbacks, because System.getenv("LOGNAME")/System.getenv("USER") is said to be more reliable than os.user property.

All in all I'd say we'd better include the fallbacks if there is some (major?) framework affected by this. If it's 'only' enduser code the endusers should better move on to a more suitable solution as suggested in https://docs.python.org/2/library/os.html#os.getlogin.

Patrick: What is your view on this?

I also wonder how this can have worked in Jython 2.2.1. Maybe PosixModule has been rewritten since then and they had the System.property variant in old days.

Finally, I don't like the error message. That could be improved. Downside here is that CPython's variant isn't ideal either, so I see no point in reproducing that one. I suppose they also just put through some original cause that wasn't especially caught. Anyway, maybe we could still improve on that point? Suggestions?
msg11113 (view) Author: James Mudd (jamesmudd) Date: 2017-02-22.19:46:48
I like the idea of making the platform pure Java, but then I agree Jython shouldn't implement anything that you can do within the JVM. (By default I really like JyNI though! Hoping to get some more time to look at it soon)

Had another idea for a solution for this maybe we should just return None if the OS call fails I did this in https://github.com/jythontools/jython/pull/56 It's actually easy as getlogin() already returns PyObject. I'm not sure its a great solution though what do you think? It avoids the error message issue and is quite faithful to https://linux.die.net/man/3/getlogin If this was adopted it would still be down to the user code to handle the None in some way but the error would be gone.

I don't think the "private static final POSIX posix = POSIXFactory.getJavaPOSIX(posixHandler, true);" would be useful almost all the methods in PosixModule disappear in this case because of the "@Hide(value=OS.NT, posixImpl = PosixImpl.JAVA)" annotation.

Also the github repo is a bit behind the mercurial one I have another issue for that http://bugs.jython.org/issue2556
msg11116 (view) Author: Stefan Richthofer (stefan.richthofer) Date: 2017-02-23.16:28:44
Your're right: I overlooked that @Hide also rules out PosixImpl.JAVA.
Regarding None I'm not sure how that benefits the user: He/she can as well catch the exception. IMO it would be more convenient to let at least the exception type match that of CPyton so the except-statement can be easily written in a portable way.

So given that CPython happens to raise OSError we should do so as well. I'd suggest to let the error message point user directly to the better workaround, e.g. something like:
"getlogin failed. It is recommended to rather use os.getenv('LOGNAME')/os.getenv('USER') on posix or os.getenv('USERNAME') on Windows."
msg11118 (view) Author: James Mudd (jamesmudd) Date: 2017-02-23.20:26:16
I agree making the Jython behaviour consistent with CPython is probably the best. So throw OSError but with a more helpful messages.

I have a pull request to do this https://github.com/jythontools/jython/pull/57

I haven't handled the Windows case as the getlogin method is hidden on Windows by the annotation.
msg11122 (view) Author: Stefan Richthofer (stefan.richthofer) Date: 2017-02-24.11:31:16
Fixed via PR by James Mudd as of https://hg.python.org/jython/rev/07724b0547e0.

Note that I marked Resolution as "invalid" to display that "fix" in this case means that we only  improved the error-message.
History
Date User Action Args
2017-02-24 11:31:16stefan.richthofersetstatus: open -> pending
messages: + msg11122
priority: normal
assignee: stefan.richthofer
milestone: Jython 2.7.1
keywords: + console
resolution: invalid
2017-02-23 20:26:17jamesmuddsetmessages: + msg11118
2017-02-23 16:28:45stefan.richthofersetmessages: + msg11116
2017-02-22 19:46:48jamesmuddsetmessages: + msg11113
2017-02-20 21:49:46stefan.richthofersetmessages: + msg11105
2017-02-20 21:09:37jamesmuddsetmessages: + msg11104
2017-02-20 15:44:59stefan.richthofersetnosy: + stefan.richthofer, zyasoft
messages: + msg11103
2017-02-20 14:06:12jamesmuddsetmessages: + msg11102
2017-02-19 20:00:55jamesmuddsetnosy: + jamesmudd
messages: + msg11100
2016-11-28 10:58:20pguermosettitle: os.getlogin() returns a wrong user or returns an exceptions -> os.getlogin() returns a wrong user or returns an exception
2016-11-28 10:58:13pguermosettitle: os.getlogin() returns a wrong user or return an exception -> os.getlogin() returns a wrong user or returns an exceptions
2016-11-28 10:57:59pguermocreate