Project

General

Profile

Actions

Bug #9881

closed

smbd terminated by SIGABRT after smb_account_free()

Added by Vitaliy Gusev over 2 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Start date:
2018-10-10
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:

Description

Effected any latest version of smb/server

From core dump:

status: process terminated by SIGABRT (Abort), pid=669 uid=0 code=-1

Stack trace:

libc.so.1`raise+0x2b
libumem.so.1`umem_do_abort+0x2b
libumem.so.1`umem_err_recoverable+0x5a
libumem.so.1`process_free+0xbf
libumem.so.1`umem_malloc_free+0x1a
libsmb.so.1`smb_account_free+0x1e
libmlsvc.so.1`lsarpc_s_LookupSids+0x1d7
libmlrpc.so.2`ndr_generic_call_stub+0xe6
libmlsvc.so.1`lsarpc_call_stub+0x27
libmlrpc.so.2`ndr_svc_request+0x5f
libmlrpc.so.2`ndr_svc_process+0x44
libmlrpc.so.2`ndr_pipe_process+0x9a
libmlrpc.so.2`ndr_pipe_worker+0x54
pipesvc_worker+0x195
libc.so.1`_thrp_setup+0x88
libc.so.1`_lwp_start


Files

il-9881-gwr.patch (1.5 KB) il-9881-gwr.patch Gordon Ross, 2018-10-12 04:17 AM

Related issues

Related to illumos gate - Bug #9882: smbd: service exit timed out during terminatingClosedVitaliy Gusev2018-10-10

Actions
Actions #1

Updated by Gordon Ross over 2 years ago

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.

Actions #2

Updated by Vitaliy Gusev over 2 years ago

  • Related to Bug #9882: smbd: service exit timed out during terminating added
Actions #3

Updated by Vitaliy Gusev over 2 years ago

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.

Actions #5

Updated by Gordon Ross over 2 years ago

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.

Actions #6

Updated by Vitaliy Gusev over 2 years ago

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.

Actions #8

Updated by Vitaliy Gusev over 2 years ago

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.

Actions #9

Updated by Dan McDonald over 2 years ago

  • Subject changed from smbd terminated by SIGABRT after smb_account_free() to smbd terminated by SIGABRT after smb_account_free()
Actions #10

Updated by Electric Monk over 2 years ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 62f63298eba531d48f87aa8c2089298cb7821962

commit  62f63298eba531d48f87aa8c2089298cb7821962
Author: Vitaliy Gusev <gusev.vitaliy@gmail.com>
Date:   2018-10-19T17:00:17.000Z

    9881 smbd terminated by SIGABRT after smb_account_free()
    Reviewed by: Gordon Ross <gordon.w.ross@gmail.com>
    Reviewed by: Jason King <jason.brian.king@gmail.com>
    Approved by: Dan McDonald <danmcd@joyent.com>

Actions

Also available in: Atom PDF