Bug 10880 - S3: source3/smbd/process.c::srv_send_smb() returns true on the error path.
Summary: S3: source3/smbd/process.c::srv_send_smb() returns true on the error path.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 11041
  Show dependency treegraph
 
Reported: 2014-10-16 03:16 UTC by Warren
Modified: 2015-01-09 10:33 UTC (History)
4 users (show)

See Also:


Attachments
git-am fix for 4.1.next, 4.0.next. (1.16 KB, patch)
2014-10-28 21:05 UTC, Jeremy Allison
ddiss: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Warren 2014-10-16 03:16:16 UTC
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.
Comment 1 Jeremy Allison 2014-10-28 00:15:29 UTC
Ah, this already got fixed in master. I'll look at the backport required.
Jeremy.
Comment 2 Jeremy Allison 2014-10-28 21:05:06 UTC
Created attachment 10377 [details]
git-am fix for 4.1.next, 4.0.next.

Pretty obvious back-port IMHO :-).

Jeremy.
Comment 3 David Disseldorp 2014-10-29 13:51:25 UTC
Comment on attachment 10377 [details]
git-am fix for 4.1.next, 4.0.next.

LGTM
Comment 4 David Disseldorp 2014-10-29 13:54:30 UTC
Karolin, please merge to maintenance branches.
Comment 5 Karolin Seeger 2014-10-29 20:09:27 UTC
Pushed to autobuild-v4-[0|1]-test.
Comment 6 Karolin Seeger 2014-11-04 20:07:04 UTC
(In reply to Karolin Seeger from comment #5)
Pushed to v4-0-test, waiting for autobuild-v4-1-test.
Comment 7 Karolin Seeger 2014-11-10 20:31:50 UTC
(In reply to Karolin Seeger from comment #6)
Pushed to v4-1-test.

Closing out bug report.

Thanks!
Comment 8 Warren 2015-01-09 07:43:41 UTC
(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?
Comment 9 David Disseldorp 2015-01-09 10:00:38 UTC
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.
Comment 10 Warren 2015-01-09 10:10:01 UTC
(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?
Comment 11 David Disseldorp 2015-01-09 10:33:30 UTC
(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.