composite rpc call
mimir at samba.org
Tue Feb 22 10:55:27 GMT 2005
On Tue, Feb 22, 2005 at 02:00:10PM +1100, Andrew Tridgell wrote:
> > Could you please take a look at source/libnet/userinfo.c file and tell
> > me what is coded incorrectly. I haven't tested it yet, so almost
> > certainly something is broken, but you surely have some opinion on
> > that code just by looking.
> I hope you don't mind me replying publicly. I thought that others
> might find the answers interesting.
Of course. It has some educational valor for potential samba hackers.
> Comments in no particular order:
> The "struct rpc_composite_userinfo" needs to be in a public header
> file, as it will be used by callers
Yes, I know. I've forgotten to commit this bit. It's been fixed already.
> In the _send() function you copy the io structure that the user passed
> in. Then in the _recv() function you fill in that copy. That means
> that with your current code the user never gets to see the results of
> the call.
Oh, you're right. I got inspired by loadfile code too much.
> There are two easy solutions you can choose from for this:
> 1) don't copy the io structure, and assume that the caller keeps it
> valid (ie. doesn't go out of scope) during the whole call. This
> is how the pidl-generated RPC calls work.
> 2) add a io parameter to the _recv() call, and fill it in, thus
> allowing the caller to use a different io structure pointer in
> the _send() and _recv() calls. This is how I structured the
> libcli/nbt/ client library.
This one definately seems to be more apropriate for libnet.
> Which pattern you use is up to you. I haven't been consistent between
> subsystems in how I handle this, although within each subsystem I have
> chosen one method or the other. I suggest you do the same.
> Lines like this:
> memcpy((void*)r.in.user_handle, (void*)rep->out.user_handle, sizeof(struct policy_handle));
> should be replaced with this:
> r.in.user_handle = rep->out.user_handle;
> using memcpy for a structure copy is ugly and unnecessary (this
> applies to all the memcpy() calls in your userinfo.c code)
I don't like it too. I just wasn't sure if ordinary pointer assign can
potentially be freed between the calls. But if you say I can just copy
it, not the structure itself, then it's fine to me.
> You need to check for failure for calls like this:
> sid = dom_sid_parse_talloc(c, s->io.in.sid);
> add something like this:
> if (sid == NULL) goto failed;
> The line
> s->pipe->conn->pending = dcerpc_samr_OpenUser_send(p, c, r);
> is completely wrong. That is operating on internal queues that
> application code should never touch (plus it won't work). Instead,
> follow the examples in other parts of the code and do something like
That was one of my primary concerns as digging through the structure
to do such ordinary thing seemed a bit suspicous to be right.
> s->req = dcerpc_samr_OpenUser_send(p, c, r);
> if (s->req == NULL) goto failed;
> s->req->async.callback = userinfo_handler;
> s->req->async.private = c;
> and add:
> struct rpc_request *req;
> into struct userinfo_state
> The line
> c->state = RPC_REQUEST_DONE;
> uses the wrong type of enum. A composite_context structure contains
> enum smbcli_request_state state
> so you probably meant this:
> c->state = SMBCLI_REQUEST_DONE;
> c->state = SMBCLI_REQUEST_ERROR;
> The same goes for all other places you use RPC_REQUEST_*
So it is SMBCLI states anyway, since that's the transport for RPC calls,
do I get it correctly ?
> In userinfo_openuser() you do this:
> struct rpc_request *req = s->pipe->conn->pending;
> that should be this:
> struct rpc_request *req = s->req;
> The line
> rep = (struct samr_OpenUser*)req->ndr.struct_ptr;
> is no good. What you probably wanted was to declare:
> struct samr_OpenUser *openuser;
> in struct userinfo_state, then allocate it in the _send() call like
> s->openuser = talloc(s, struct samr_OpenUser);
It looks like I need to put rpc calls' structures in userinfo_state,
doesn't it ? At least a few of them.
> That's enough for now. Several of the above problems apply to more
> than one place. When you've fixed up the above, show me the code again
> and I'll take another look.
OK, thank you.
Samba Team member http://www.samba.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: Digital signature
Url : http://lists.samba.org/archive/samba-technical/attachments/20050222/7cb3e0ac/attachment.bin
More information about the samba-technical