msg120950 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
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) * |
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) * |
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) * |
Date: 2014-01-10 10:19 |
LGTM
|
msg207843 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-01-10 10:24 |
I doubt that we can change behavior at this time.
|
msg207846 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2014-01-10 10:28 |
You mean this should be made in 3.4 only?
|
msg207856 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2014-12-31 18:11 |
Adding a test would be a good idea, though...
|
msg233266 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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) * |
Date: 2015-01-01 17:47 |
The approach in the patch seems good to me.
|
msg247376 - (view) |
Author: Robert Collins (rbcollins) * |
Date: 2015-07-25 19:07 |
Too late for 3.5 for this IMO.
|
msg247377 - (view) |
Author: Robert Collins (rbcollins) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:08 | admin | set | github: 54597 |
2015-07-25 19:11:49 | rbcollins | set | messages:
+ msg247378 stage: commit review -> patch review |
2015-07-25 19:11:13 | rbcollins | set | messages:
+ msg247377 |
2015-07-25 19:07:40 | rbcollins | set | nosy:
+ rbcollins
messages:
+ msg247376 versions:
+ Python 3.6, - Python 3.5 |
2015-01-01 17:47:13 | pitrou | set | nosy:
+ pitrou messages:
+ msg233289
|
2014-12-31 18:30:28 | serhiy.storchaka | set | messages:
+ msg233266 |
2014-12-31 18:11:11 | r.david.murray | set | messages:
+ msg233261 |
2014-12-31 18:09:35 | r.david.murray | set | messages:
+ msg233260 stage: commit review |
2014-12-31 17:12:46 | giampaolo.rodola | set | messages:
+ msg233254 |
2014-12-31 16:23:43 | akuchling | set | nosy:
- akuchling
|
2014-01-10 14:23:46 | r.david.murray | set | versions:
+ 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:12 | giampaolo.rodola | set | messages:
+ msg207860 |
2014-01-10 12:00:57 | serhiy.storchaka | set | messages:
+ msg207856 |
2014-01-10 10:28:05 | giampaolo.rodola | set | messages:
+ msg207846 |
2014-01-10 10:24:37 | serhiy.storchaka | set | messages:
+ msg207843 |
2014-01-10 10:19:17 | giampaolo.rodola | set | messages:
+ msg207841 |
2014-01-10 09:59:45 | vajrasky | set | files:
+ fix_error_message_getspall_v2.patch
messages:
+ msg207839 |
2014-01-09 21:06:50 | giampaolo.rodola | set | messages:
+ msg207793 |
2014-01-09 20:41:33 | serhiy.storchaka | set | priority: normal -> low |
2013-08-20 14:27:05 | vajrasky | set | files:
+ fix_error_message_getspall.patch
nosy:
+ vajrasky messages:
+ msg195692
keywords:
+ patch |
2013-08-20 13:51:53 | serhiy.storchaka | set | nosy:
+ loewis, serhiy.storchaka, akuchling
messages:
+ msg195689 versions:
+ Python 3.3, Python 3.4, - Python 3.1, Python 3.2 |
2010-11-11 14:58:32 | eric.araujo | set | versions:
- Python 2.6 |
2010-11-11 14:03:10 | giampaolo.rodola | create | |