Came across source3/smbd/process.c::srv_send_smb() recently and noticed the function returns true regardless. E.g. it returns true if it failed to encrypt or write_data(). This change was originally made in 54c51a66 (https://git.samba.org/?p=samba.git;a=commitdiff;h=54c51a66e3e31c70a641d7efac2d4b08c3007278#patch17). That part was sneaked in when SMB message statistics was added in Samba 3.5. Unfortunately, the commit message didn't mention anything about why this change was made. However, it returned false on error path in pre-3.5 . Without returning the error, callers will blindly continue when an error happens, which is probably not what we want. I might be wrong though as I didn't look at the calling context further.
Ah, this already got fixed in master. I'll look at the backport required. Jeremy.
Created attachment 10377 [details] git-am fix for 4.1.next, 4.0.next. Pretty obvious back-port IMHO :-). Jeremy.
Comment on attachment 10377 [details] git-am fix for 4.1.next, 4.0.next. LGTM
Karolin, please merge to maintenance branches.
Pushed to autobuild-v4-[0|1]-test.
(In reply to Karolin Seeger from comment #5) Pushed to v4-0-test, waiting for autobuild-v4-1-test.
(In reply to Karolin Seeger from comment #6) Pushed to v4-1-test. Closing out bug report. Thanks!
(In reply to Jeremy Allison from comment #2) I think the backport code missed one line of code. git diff (against v4-1-stable). diff --git a/source3/smbd/process.c b/source3/smbd/process.c index d01bf39..9d84578 100644 --- a/source3/smbd/process.c +++ b/source3/smbd/process.c @@ -172,6 +172,7 @@ bool srv_send_smb(struct smbd_server_connection *sconn, char DEBUG(0, ("send_smb: SMB encryption failed " "on outgoing packet! Error %s\n", nt_errstr(status) )); + ret = -1; goto out; } } Should we reopen this bug?
On Fri, 09 Jan 2015 07:43:41 +0000, samba-bugs@samba.org wrote: > https://bugzilla.samba.org/show_bug.cgi?id=10880 > > --- Comment #8 from Warren <w_cao2000@msn.com> --- > (In reply to Jeremy Allison from comment #2) > > I think the backport code missed one line of code. > > git diff (against v4-1-stable). > > diff --git a/source3/smbd/process.c b/source3/smbd/process.c > index d01bf39..9d84578 100644 > --- a/source3/smbd/process.c > +++ b/source3/smbd/process.c > @@ -172,6 +172,7 @@ bool srv_send_smb(struct smbd_server_connection *sconn, > char > DEBUG(0, ("send_smb: SMB encryption failed " > "on outgoing packet! Error %s\n", > nt_errstr(status) )); > + ret = -1; > goto out; > } > } > > Should we reopen this bug? > That fix went into master after this change, via: commit cc983c9a6a92f3d127ec6461b15aed3fa90e6d30 Author: Volker Lendecke <vl@samba.org> Date: Sun Aug 18 20:35:32 2013 +0000 smbd: Fix CID 1063259 Uninitialized scalar variable ...but was never merged for maintenance. I'll raise a new bug to track this. Thanks a lot for the heads up Warren.
(In reply to David Disseldorp from comment #9) > smbd: Fix CID 1063259 Uninitialized scalar variable Good pickup by Coverity. Just wondering where we could access the Samba Coverity result?
(In reply to Warren from comment #10) The Coverity results aren't public. Access can be requested via scan.coverity.com. Bug 11041 has been raised to track the merge of Volker's fix.