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
spwd returning different value depending on privileges #54597
Comments
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? |
See also bpo-18787. |
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: 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). |
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. |
Here is the updated patch addressing Giampaolo's concern. |
LGTM |
I doubt that we can change behavior at this time. |
You mean this should be made in 3.4 only? |
Even for 3.4 it looks too late. But I afraid we can't change this behavior never. Users of spwd don't expect Only one safe way is design new API and deprecate old API. This will be great |
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. 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. |
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 :) |
I still think this should be fixed (raise an exception) in the next major release. |
Unless Serhiy (or someone else) still objects, I say go ahead and commit it to default. |
Adding a test would be a good idea, though... |
This issue should be considered in conjunction with bpo-18787. 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? |
The approach in the patch seems good to me. |
Too late for 3.5 for this IMO. |
The man page for setspent says this:
" -> 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. |
I'm moving this back to patch review - it needs a test, particularly because of the question I have around setspent. |
Note the spwd module is deprecated in 3.11 and set for removal in 3.13. See PEP 594 – Removing dead batteries from the standard library and #91217. |
Closing per Hugo's comment. A |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: