Quoth Keith M Wesolowski on Tue, Oct 02, 2007 at 11:13:20AM -0700:
> On Thu, Sep 13, 2007 at 08:34:31PM -0700, David Bustos wrote:
> > > > cmd/svc/startd/graph.c
> > > > 5583: Shouldn't you add an SCF_ERROR_PERMISSION_DENIED case to this
> > > > switch?
> > > >
> > > > cmd/svc/startd/libscf.c
> > > > old 966,1012,2321,2346,2372,2882,2958,2983: Shouldn't you add EACCES
> > > > cases to these switches?
> > > >
> > > > old 1685,1814,3159,3486: Shouldn't you add SCF_ERROR_PERMISSION_DENIED
> > > > cases to these switches?
> > > >
> > > > cmd/svc/startd/startd.c
> > > > 508: Shouldn't you add an SCF_ERROR_PERMISSION_DENIED case to this
> > > > switch?
> > >
> > > This is a can't happen case, but for consistency with the existing
> > > error handling I've added them to the bad_error() paths.
> >
> > They can't happen if the user doesn't do anything stupid. Since they're
> > outside the control of svc.startd, they should be handled.
>
> The user has no control of these things; startd is reading from
> framework property groups, for which read_authorization has no effect.
> See rc_node_pg_check_read_protect() near 4049. Neither a bad manifest
> nor administrator error can trigger these code paths. This is
> described in the ARC case and clarified by Gary's addendum. The only
> way these can fail on my account is a bug in configd.
>
> Is there some manner in which this can fail that I'm overlooking?
Yes: we don't stop users from changing the types of property groups. In
process_actions() we've verified that the property group's name is
SCF_PG_RESTARTER_ACTIONS, but we haven't verified the type. startd
shouldn't abort() if the user changed it to something other than
framework and read-protected it. In libscf_get_basic_instance_data(),
we've retrieved the property group named SCF_PG_GENERAL_OVR, but we
haven't verified it's type. startd shouldn't abort() if the user
changed it to something other than framework and read-protected it.
If there are cases where we have verified the property group type, then
I'll agree with you.
That said, I suspect there's not a legitimate use case for customizing
property group types or property types, though I anticipate resistance
from the team over such a proposal. In the meantime, you should handle
these misconfigurations gracefully.
> > > > cmd/svc/svccfg/svccfg_libscf.c
> > ...
> > > > 7302: Shouldn't you tell the user what you couldn't read? And
> > > > shouldn't you produce an empty property element anyway?
> > >
> > > No. We reach this case only if the user has given -a; that is,
> > > explicitly requested all property values. If we are unable to obtain
> > > one, but generate the manifest/archive anyway, its contents will not
> > > be what the user requested. The documentation changes discussed in
> > > the ARC case make this clear.
> >
> > Ok, but the documentation says "an error results". I think I was asking
> > you to make the error contain the name of the property group, rather
> > than just "permission denied".
>
> I'll file an RFE for this but I don't intend to implement it. The
> documentation is sufficiently clear that no unprivileged user should
> expect export/archive -a to be successful.
I understand your prioritization of usability work.
...
> > cmd/svc/configd/rc_node.c
> > 2013: Since this function can already fail with _NO_RESOURCES, why not
> > malloc() this?
>
> At old 1977, this was allocated on the stack. I see little reason to
> change that.
It's subtle and not widely heeded in svc.configd, so I'll fix it if
I come across that code later.
...
> > 2340: Can you make this
> >
> > case REP_PROTOCOL_NO_RESOURCES:
> > return (ret);
> >
> > default:
> > bad_error("rc_node_pg_check_read_protect", ret);
> > }
> >
> > ?
>
> I don't think _DELETED is a bad_error() here, is it? I think
>
> case REP_PROTOCOL_FAIL_NO_RESOURCES:
> case REP_PROTOCOL_FAIL_DELETED:
> return (ret);
>
> default:
> bad_error("rc_node_pg_check_read_protect", ret);
> }
>
> is correct. We can already return _DELETED above.
Right.
...
> > 2729: Is there any reason not to move this up with the
> > rc_node_modify_permission_check() invocation?
>
> I was extremely careful to preserve the exact precedence of error
> codes in all of these cases. I'd prefer to leave it as it is.
That's admirable, but I suspect it's not worth the extra locking. At
least add a comment explaining why you're doing it that way.
...
> > 4706: I think this is supposed to be indented.
>
> cstyle(1) says otherwise. If you look closely, you'll see that it's
> already part of a continuation, and is indented 4 spaces. I agree
> with your sense of aesthetics, but if this is a bug it's in cstyle.
Ah, yes.
...
> > cmd/svc/startd/libscf.c
> > 992,1050,2353,2380,2406,2918,2991,3017: I don't think bad_error() is
> > the right thing to do here. Why shouldn't we log a "misconfigured"
> > error for this service and continue? At the very least, a better
> > error message is warranted.
> >
> > 1709,1846: I don't think bad_error() is the right thing to do here.
> > Why shouldn't we log a misconfiguration error and continue? At the
> > very least, a better error message is warranted.
> >
> > 3201: I don't think assert(0); abort() is the right thing to do here.
> > Why don't we just continue to clear the action, as in the
> > SCF_ERROR_CONSTRAINT_VIOLATED case?
>
> See my response for cmd/svc/startd/graph.c:5583 et al above. There's
> no way for a misconfiguration to have this result, as we are reading
> from framework property groups. If configd denies read access to a
> framework property group, it's a bug.
svc.configd doesn't guarantee correspondence between property group
names and types. Unless we've verified the property group type, you
shouldn't abort().
...
> > cmd/svc/svccfg/svccfg_libscf.c
...
> > 3188,3253,3435,3625,3678: I don't think bad_error() is the right thing
> > to do here. Why not propagate the error up the stack? At the very
> > least, a better error message is warranted.
> >
> > 3742,3848,4230: I don't think bad_error() is the right thing to do
> > here. Why not do something similar to ECANCELED? At the very
> > least, a better error message is warranted.
> >
> > 4101: I don't think bad_error() is the right thing to do here. Why
> > not do something similar to SCF_ERROR_CONSTRAINT_VIOLATED? At the
> > very least, a better error message is warranted.
>
> Clearly I'm just dense. What error should we display, "Your configd
> is buggy; it incorrectly denies read access to framework property
> groups. Please return it to Sun Microsystems, Inc. for a free
> replacement."? And how is that really any different from dropping a
> core that will tell us the exact same thing in about 5 seconds of
> debugging?
For 3188 of svccfg_libscf.c, it seems we're loading from the last-import
snapshot, the creation of which should be private, so the contents
should have been created by svccfg, so it probably isn't read-protected,
unless there's a bug in svccfg, which wouldn't surprise me.... Anyway,
that upgrade code is complicated and should be removed in the enchanced
profiles project, so I can understand reluctance to spend time on it.
Which covers all of these cases, actually. I still think it would be
better to emit more precise messages, but given their unlikliness,
I suppose it's ok to fix them in response to escalations, and I also
understand your prioritization of usability work.
> > 4269: "(backend access denied)" doesn't apply to your new error.
> > Wouldn't it be better to return EPERM for it?
>
> I'll remove the (backend access denied). What is the advantage of
> returning EPERM instead of EACCES?
The backend-access-denied code is meant for when there are problems
accessing a remote backend. In this case, there wasn't a problem
accessing the backend; you're explicitly denying access due to
permissions. Therefore EPERM is more appropriate.
...
> > cmd/svc/svccfg/svccfg_help.c
> > 21: This change seems unnecessary.
> >
> > help_messages: Shouldn't you add an entry for restore?
>
> Yes. I added
>
> { SCC_RESTORE, "restore file\n\n"
> "Restore the contents of a previously-generated archive."
> },
>
> in response to another reviewer's similar comment.
Indeed. You should also add the command to help() in svccfg_engine.c.
> > lib/librestart/common/librestart.c
> > 1072,1371: You shouldn't abort here. You should return an error
> > somehow.
>
> SCF_PG_RESTARTER is a framework property group.
By convention. We'll still reach this code if it was customized.
> Based on this review, I'll be filing four bugs/RFEs:
>
> inheritance of read_authorization from svc incorrect for snapshots
> clean up 6537749 et al to reflect late-breaking code review comments
> svccfg export/archive -a could provide better error messages
> cstyle(1) should allow do-while without braces
The last exists as 6589749.
> and fixing the first two of these. I'll have a webrev in a couple of
> days. The only open issue on which we seem to disagree is
> error-handling for _PERMISSION_DENIED failures on reads from framework
> property groups.
Thanks, Keith.
David
