On 09.05.2014 21:33, Jakub Hrozek wrote: > On Mon, 2014-05-05 at 11:25 +0200, Stef Walter wrote: >> On 02.05.2014 17:25, Jakub Hrozek wrote: >>>>> [PATCH 1/6] SBUS: two trivial style fixes SSIA >>>>> >>>>> [PATCH 2/6] SBUS: Add a convenience function Adds a convenience >>>>> function that constructs a DBusError internally and as such can be >>>>> used to mark an sbus request as failed without having to create a >>>>> DBusError instance by the caller. The error message must be either >>>>> allocated on top of dbus_req or a constant, DBus itself doesn't copy >>>>> the error message! This is reflected in the doxygen docstring. >>>> >>>> Hmmm, rather than that (and because of the dbus behavior), my preference >>>> would be to have an sbus_error_new() method that created a DBusError on >>>> a MEM_CTX, and could be used like this: >>>> >>>> sbus_request_fail_and_finish (dbus_req, >>>> sbus_error_new (dbus_req, >>>> "dotted.Error", >>>> "format str: %s", arg)); >>>> >>>> This is what I sorta imagined when implementing >>>> sbus_request_fail_and_finish() >>> >>> Done, this makes sense, thanks for the suggestion. >> >> +DBusError *sbus_error_new(TALLOC_CTX *mem_ctx, >> + const char *dbus_err_name, >> + const char *fmt, >> + ...); >> >> This declaration should be decorated with the appropriate GCC attribute >> for printf style functions. Otherwise folks will easily create printf >> string vulnerabilities. ie: >> >> #if __GNUC__ > 2 >> __attribute__((__format__(__printf__, 3, 4))); >> #endif >> >> Or you probably have this wrapped up in a nice macro in SSSD, like this: >> >> http://cgit.freedesktop.org/p11-glue/p11-kit/tree/common/compat.h#n47 >> > > Thanks, good catch. We already have SSS_ATTRIBUTE_PRINTF() macro, which > I forgot to use. The patch was updated. > >>>>> [PATCH 3/6] IFP: Add utility functions Adds a number of utility >>>>> functions, most importanly ifp_req_create(). The ifp_req is a >>>>> structure that will be passed along with the ifp request and contains >>>>> the sbus request and some ifp-specific data, like caller ID or the >>>>> responder context. >>>> >>>> dbus_bus_get_unix_user() blocks. Is that fine in this case? >>> >>> I implemented what you suggested in patch 5/9. It's not polished yet, >>> because I wanted to check if that's what you had in mind before spending >>> too much time. >> >> In general it looks right on. >> >>> In particular, I would like to chain concurrent requests for the same >>> sender ID to not trash the sysbus if many requests came in after startup >>> for instance. Also unit tests are needed. >> >> That makes sense. > > I haven't implemented the request chaining mostly because I think we > have bigger issues to solve. But I don't think we should forget about > the problem either, so I filed: > https://fedorahosted.org/sssd/ticket/2329 > > I split the async request to retrieve the caller ID into a separate > patch, because with the sbus_message_handler changes, the combined patch > was too big and too hard to read. > >> >>> I'm not too happy about sbus_message_handler now either, I'd like to >>> refactor it to be more readable, maybe pass around a single struct >>> instance instead of handler/sender/interface/etc.. >> >> I think that struct should be sbus_request. But more to the point, I >> think that much of the code should run *after* you get the caller's >> information, not before. >> > > Yeah, that was one of the points I was pondering -- how much should the > sbus message handler do. The attached patch "SBUS: Refactor > sbus_message_handler to retrieve caller ID" only uses the message > handler to verify the message should be handled by one of SSSD's > interfaces and if so, starts the get-ID async request. The real work is > done in the request callback, sbus_handler_got_caller_id(). > >> +struct sbus_unhandled_request { >> + struct sbus_request *dbus_req; >> + const struct sbus_method_meta *method; >> + sbus_msg_handler_fn handler_fn; >> >> By the way you already have the above 2 fields inside of dbus_req. > > This structure is gone in the attached patches. > >> >> + key.type = HASH_KEY_STRING; >> + key.str = discard_const(state->sender); >> + value.type = HASH_VALUE_UINT; >> + value.ul = state->uid; >> + ret = hash_enter(state->conn->clients, &key, &value); >> + if (ret != HASH_SUCCESS) { >> + ret = EIO; >> + goto done; >> + } >> >> What owns state->member? Is it going to be referenced after free? >> > > Ah, good catch! The current version of the async request strdups the > state onto the request state, so the request would run to completion > regardless of what happens to the 'sender' variable. > >>> * Should we carry on when we can't retrieve the ID for one reason or >>> another and let the callee kick out the request? >> >> I think you should drop the request if you can't identify the caller >> (may have exited for example). Maybe log something, but no use trying to >> talk to a caller you cannot identify. >> > > Right, done. > >> By the way, once you want the PID or SELinux context, you'll want to >> change GetConnectionUnixUser to GetConnectionCredentials. For now though >> it's fine to just get the UID. >> > > I added a comment to the code. > >>>>> +int ifp_enomem(struct ifp_req *ireq) >>>> >>>> This won't work as you expect. When you're out of memory it's unlikely >>>> that you'll be able to send a normal message. DBus has method calls >>>> specifically for this case: >>>> >>>> http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga2fea5f972d1bfe7bcde8c0ec65ca9e90 >>> >>> Ah, OK, I must say this part was always a bit confusing to me. Why do we try >>> to send messages crated with dbus_message_new_error(DBUS_ERROR_NO_MEMORY) >>> in sbus_message_handler, should those be fixed too? >> >> Yes probably. I'm not a big fan of trying to handle memory errors like >> other errors. It just never works reliably, and it's nearly untestable. >> I think avoiding a crash freeing up resources and moving on, maybe log >> something if you can, is the best approach. >> >> I think the only reason dbus even has this code is so things like >> systemd (ie: pid 1) can make dbus calls. >> >>> These options can be used to configure the InfoPipe responder. >> >> PATCH007: This stuff should be using polkit. I understand if the current >> patch set doesn't use polkit, but sssd will eventually need to ask >> polkit if X principal is allowed to do Y via DBus. Perhaps allowed_uids >> would bypass the polkit check (many DBus services do such a bypass with >> uid = 0). >> > > I agree, but I would prefer to finish the getsetters and implement the > object caching etc first. So I filed: > https://fedorahosted.org/sssd/ticket/2328 > > Thanks for the review! > > New patchset is attached.
This looks good. Stef _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
