illumos gate

Interesting. How did you reproduce this? Have a test client to share?

It looks like all the MLSVC functions lsarpc_s_LookupSids,
lsarpc_s_LookupSids2, lsarpc_s_LookupSids3 (and others)
may call smb_account_free multiple times on the same object.

I'd suggest making smb_account_free zero out the
members after it free's their contents.

Gordon Ross wrote:

Interesting. How did you reproduce this? Have a test client to share?

It is pretty simple. Just try to

~# rpcclient -U ""%"" -c "lookupsids S-1-5" 192.168.1.18

It looks like all the MLSVC functions lsarpc_s_LookupSids,
lsarpc_s_LookupSids2, lsarpc_s_LookupSids3 (and others)
may call smb_account_free multiple times on the same object.

As I can see only lsarpc_s_LookupSids, lsarpc_s_LookupSids2 are effected and
lsarpc_s_LookupSids3 does not call any free function.

I'd suggest making smb_account_free zero out the
members after it free's their contents.

I am preparing CR, please wait.

https://www.illumos.org/rb/r/1229/

OK, thanks. One thing that was not clear to me after reading the above is that
when lsa_lookup_sid returns an error, it leaves junk in the output struct.

That's not ideal. Normally functions like this should leave the output struct
cleared out (or as it was on entry) when returning an error.

Gordon Ross wrote:

OK, thanks. One thing that was not clear to me after reading the above is that
when lsa_lookup_sid returns an error, it leaves junk in the output struct.

That's not ideal. Normally functions like this should leave the output struct
cleared out (or as it was on entry) when returning an error.

Yes, it should be calling convention for lsa_lookup_sid() though. But just fixing lsa_lookup_sid() is not enough, because bzero() is still required when doing next iteration in for(). In this case, bzero() can be before lsa_lookup_sid(). You can choose what fix is more elegant and right.

Gordon Ross wrote:

I would prefer a fix with these properties:
a: There should be no cleanup to do by the caller of a "get" function that returns an error.
b: Cleanup of successful "get" returned data should happen exactly once or each get.

As I wrote, there should be calling convention for lsa_lookup_sid(). And I was thinking about this during preparing fix to achieve (a) and (b) as well. But I also saw that all another SMB functions that work with smb_account_t argument do not cleanup argument during returning error, in other words pollute it. For instance: lsar_lookup_sids() and smb_sam_lookup_sid() and many-many others.

And that brought a dualism, because either all those smb functions should be modified or neither of them.

Also there was a question why passed argument was left polluted in many-many functions and maybe it was done intentionally ? Maybe it is important do not zeroing it? And possible answer is: to keep information when function returns error for analyzing in dtrace func:return probes. If we bzero() before return, this information is cleared and brings impossibility to find out for what account error is returned.