classification
Title: spwd returning different value depending on privileges
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: giampaolo.rodola, loewis, pitrou, r.david.murray, rbcollins, serhiy.storchaka, vajrasky
Priority: low Keywords: patch

Created on 2010-11-11 14:03 by giampaolo.rodola, last changed 2015-07-25 19:11 by rbcollins.

Files
File name Uploaded Description Edit
fix_error_message_getspall.patch vajrasky, 2013-08-20 14:27 review
fix_error_message_getspall_v2.patch vajrasky, 2014-01-10 09:59 review
Messages (19)
msg120950 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-11-11 14:03
As root:

>>> import spwd
>>> spwd.getspall()
[spwd.struct_spwd(sp_nam='root', sp_pwd='!', sp_lstchg=14895, sp_min=0, sp_max=99999, sp_warn=7, sp_inact=-1, sp_expire=-1, sp_flag=-1)
 ...
]

As limited user:

>>> import spwd
>>> spwd.getspall()
[]
>>> 

Wouldn't it be better for consistency to raise OSError EACCES instead?
msg195689 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-20 13:51
See also issue18787.
msg195692 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-08-20 14:27
Attached the patch to raise OSError instead of returning empty list if a normal user uses getspall function of spwd module.

I investigated about this issue. There exists situation where /etc/shadow file does not exist:
https://bugs.kde.org/show_bug.cgi?id=117202
https://dev.openwrt.org/ticket/2887

So I think it is better to differentiate the result of the function where a normal user trying to access /etc/shadow file (permission denied, therefore should raise exception) or root user trying to access non-existent /etc/shadow file (therefore should get empty list).
msg207793 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2014-01-09 21:06
Looking back at this: considering we may get errors != EACCESS I think we better be as generic as possible as in:

--- a/Modules/spwdmodule.c
+++ b/Modules/spwdmodule.c
@@ -153,6 +153,8 @@
     if ((d = PyList_New(0)) == NULL)
         return NULL;
     setspent();
+    if (errno != 0)
+        return PyErr_SetFromErrno(PyExc_OSError);
     while ((p = getspent()) != NULL) {
         PyObject *v = mkspent(p);
         if (v == NULL || PyList_Append(d, v) != 0) {


As for 2.7 and 3.3 I have a little concern in terms of backward compatibility as the user will suddenly receive an exception instead of [] but I think that for the sake of correctness the cost is justified.
msg207839 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-10 09:59
Here is the updated patch addressing Giampaolo's concern.
msg207841 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2014-01-10 10:19
LGTM
msg207843 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-10 10:24
I doubt that we can change behavior at this time.
msg207846 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2014-01-10 10:28
You mean this should be made in 3.4 only?
msg207856 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-10 12:00
Even for 3.4 it looks too late.

But I afraid we can't change this behavior never. Users of spwd don't expect 
any exception, raising an exception will break any existing code.

Only one safe way is design new API and deprecate old API. This will be great 
refactoring, it should also change APIs of the pwd and grp modules, perhaps 
these three modules should be merged, perhaps we should cross-platform API 
(available on Windows).
msg207860 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2014-01-10 13:47
I can agree with you that it's probably better to avoid changing existent python versions to avoid breakage but I see no need to be that strict for newer ones.

Current version should check errno but it doesn't. IMO that is clearly a bug.
Same goes for returning an empty list when users actually exists and for *not* raising FileNotFoundError if the shadow file is not available.

Also note that the doc does not mention that an empty list should be treated as an alias for "insufficient permissions" or "shadow file not available", therefore whoever made that assumption is someone who deliberately and erroneously decided to do so.
msg207863 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-01-10 14:23
I think that a new cross platform API would be great, but I don't think that fixing this bug should wait for that.  It may be borderline for being changed in a feature release (and definitely should not be changed in a maintenance release), but I think it would be OK.  Mostly likely a program that uses spwd will also be using other root-only functions and already be producing other errors if run as a non-privileged user.  The file-doesn't-exist case is a little more concerning, but my guess is that anyone actually checking for an empty return (as opposed to their program just failing when it gets one) would actually welcome the change, since it will give a more specific error message.

Or, to put it another way, I think the negative impact of this in a feature release will be low enough that we won't take much if any flak for it :)
msg233254 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2014-12-31 17:12
I still think this should be fixed (raise an exception) in the next major release.
msg233260 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-12-31 18:09
Unless Serhiy (or someone else) still objects, I say go ahead and commit it to default.
msg233261 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-12-31 18:11
Adding a test would be a good idea, though...
msg233266 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-12-31 18:30
This issue should be considered in conjunction with issue18787. And if we decise to break backward compatibility, this should be well documented in What's New.

May be discuss these issues on Python-Dev?
msg233289 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-01 17:47
The approach in the patch seems good to me.
msg247376 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-25 19:07
Too late for 3.5 for this IMO.
msg247377 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-25 19:11
The man page for setspent says this:
"The functions that return a pointer return NULL if no more entries are available or if an error occurs during processing.  The functions  which
       have int as the return value return 0 for success and -1 for failure, with errno set to indicate the cause of the error.

       For the nonreentrant functions, the return value may point to static area, and may be overwritten by subsequent calls to these functions.

       The reentrant functions return zero on success.  In case of error, an error number is returned.

"

-> that is, setspent's interaction with errno is undefined, at least on Linux. I'm a little worried about whether this may pickup false errors as a result.
msg247378 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-25 19:11
I'm moving this back to patch review - it needs a test, particularly because of the question I have around setspent.
History
Date User Action Args
2015-07-25 19:11:49rbcollinssetmessages: + msg247378
stage: commit review -> patch review
2015-07-25 19:11:13rbcollinssetmessages: + msg247377
2015-07-25 19:07:40rbcollinssetnosy: + rbcollins

messages: + msg247376
versions: + Python 3.6, - Python 3.5
2015-01-01 17:47:13pitrousetnosy: + pitrou
messages: + msg233289
2014-12-31 18:30:28serhiy.storchakasetmessages: + msg233266
2014-12-31 18:11:11r.david.murraysetmessages: + msg233261
2014-12-31 18:09:35r.david.murraysetmessages: + msg233260
stage: commit review
2014-12-31 17:12:46giampaolo.rodolasetmessages: + msg233254
2014-12-31 16:23:43akuchlingsetnosy: - akuchling
2014-01-10 14:23:46r.david.murraysetversions: + Python 3.5, - Python 2.7, Python 3.3, Python 3.4
nosy: + r.david.murray

messages: + msg207863

components: + Library (Lib)
type: behavior
2014-01-10 13:47:12giampaolo.rodolasetmessages: + msg207860
2014-01-10 12:00:57serhiy.storchakasetmessages: + msg207856
2014-01-10 10:28:05giampaolo.rodolasetmessages: + msg207846
2014-01-10 10:24:37serhiy.storchakasetmessages: + msg207843
2014-01-10 10:19:17giampaolo.rodolasetmessages: + msg207841
2014-01-10 09:59:45vajraskysetfiles: + fix_error_message_getspall_v2.patch

messages: + msg207839
2014-01-09 21:06:50giampaolo.rodolasetmessages: + msg207793
2014-01-09 20:41:33serhiy.storchakasetpriority: normal -> low
2013-08-20 14:27:05vajraskysetfiles: + fix_error_message_getspall.patch

nosy: + vajrasky
messages: + msg195692

keywords: + patch
2013-08-20 13:51:53serhiy.storchakasetnosy: + loewis, serhiy.storchaka, akuchling

messages: + msg195689
versions: + Python 3.3, Python 3.4, - Python 3.1, Python 3.2
2010-11-11 14:58:32eric.araujosetversions: - Python 2.6
2010-11-11 14:03:10giampaolo.rodolacreate