[PATCH] smbd crash when change notify is disabled

[Ralph Böhme]

On Mon, Aug 31, 2015 at 12:44:24PM +0200, Stefan Metzmacher wrote:
> Am 31.08.2015 um 11:25 schrieb Michael Adam:
> > On 2015-08-31 at 11:20 +0200, Stefan Metzmacher wrote:
> >> Am 23.08.2015 um 09:30 schrieb Ralph Böhme:
> >>> On Sat, Aug 15, 2015 at 11:25:47AM +0200, Ralph Böhme wrote:
> >>>> On Fri, Aug 14, 2015 at 11:55:11AM +0200, Ralph Böhme wrote:
> >>>>> Hi Michael,
> >>>>>
> >>>>> On Fri, Aug 14, 2015 at 10:14:13AM +0200, Michael Adam wrote:
> >>>>>> On 2015-08-12 at 12:53 +0200, Ralph Böhme wrote:
> >>>>>>> Hi!
> >>>>>>>
> >>>>>>> Bug <https://bugzilla.samba.org/show_bug.cgi?id=11444> :
> >>>>>>>
> >>>>>>>     If "change notify = no" is set in smb.conf, notify_ctx is
> >>>>>>>     NULL. Then in file_free() we pass notify_ctx (= NULL) to
> >>>>>>>     notify_remove() which doesn't check for that and crashes.
> >>>>>>>
> >>>>>>> Patch attached. Can someone please review and push if ok?
> >>>>>>
> >>>>>> This looks good to me, except for one thing:
> >>>>>>
> >>>>>> Could you use torture_assert_foobar macros in the test suite
> >>>>>> instead of adding yet another incarnation of CHECK_STATUS?
> >>>>>> (torture_assert_ntstatus_equal_goto() and
> >>>>>>  torture_assert_ntstatus_ok_goto().)
> >>>>>
> >>>>> RIP CHECK_STATUS. :) Pushed with this change. Thanks!
> >>>>
> >>>> got the following error from autobuild:
> >>>>
> >>>> UNEXPECTED(failure): samba3.rpc.samba3.netlogon.netlogon(nt4_dc)
> >>>>
> >>>> REASON: Exception: Exception: ../source4/torture/rpc/samba3rpc.c:1426:
> >>>> Expression `auth2(torture, cli, wks_creds)' failed: auth2 failed
> >>>>
> >>>> FAILED (1 failures, 0 errors and 0 unexpected successes in 0
> >>>> testsuites)
> >>>>
> >>>> I tried three times, didn't help. Local autobuild works fine. What's
> >>>> going on?
> >>>>
> >>>> Log:
> >>>> <https://git.samba.org/slow/samba-autobuild/samba.stdout>
> >>>
> >>> running the patches incrementally in a private autobuild points at the
> >>> last patch being the culprit. No idea why.
> >>>
> >>> Attached is an updated patchset that simply modifies an existing env
> >>> (simpleserver) instead of adding a new one. This works.
> >>>
> >>> Please review and push if ok. Thanks!
> >>
> >> https://technet.microsoft.com/zh-cn/cc246747 says:
> >> If the underlying object store does not support change notifications,
> >> the server MUST fail this request with STATUS_NOT_SUPPORTED.
> >>
> >> So I think we should use NT_STATUS_NOT_SUPPORTED instead of
> >> NT_STATUS_NOT_IMPLEMENTED.
> 
> I just noticed that this change is just for notify_remove()
> and the only caller ignored the result.
> 
> I also found that the MS-CIFS says:
> 
> If the server does not support the NT_TRANSACT_NOTIFY_CHANGE subcommand,
> it can return an
> error response with STATUS_NOT_IMPLEMENTED (ERRDOS/ERRbadfunc) in
> response to an
> NT_TRANSACT_NOTIFY_CHANGE Request. Alternatively, it can send
> STATUS_NOTIFY_ENUM_DIR
> (ERRDOS/ERR_NOTIFY_ENUM_DIR) to cause the client to enumerate the directory
> 
> I'll push the patchset unchanged for now.
> 
> Changing the status for SMB2 to NOT_SUPPORTED
> or changing both to ENUM_DIR can be a later change.

+1

-Ralph

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de,mailto:kontakt@sernet.de