composite rpc call
Rafal Szczesniak
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:
> Mimir,
>
> > 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:
>
> (1)
> 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.
> (2)
> 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.
>
>
> (3)
> 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.
> (4)
> 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;
Sure. Added.
> (5)
> 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
> this:
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
>
> (6)
> The line
> c->state = RPC_REQUEST_DONE;
> uses the wrong type of enum. A composite_context structure contains
> this:
> enum smbcli_request_state state
> so you probably meant this:
> c->state = SMBCLI_REQUEST_DONE;
> or
> 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 ?
> (7)
> In userinfo_openuser() you do this:
> struct rpc_request *req = s->pipe->conn->pending;
> that should be this:
> struct rpc_request *req = s->req;
>
> (8)
> 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
> this:
> 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.
cheers,
--
Rafal Szczesniak
Samba Team member http://www.samba.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
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
mailing list