There are tests all over the SMB1 code to check that srv_send_smb fails, but it never returns false

[Nick Boyce]

On Sat, Jul 20, 2013 at 1:02 AM, Richard Sharpe
<realrichardsharpe at gmail.com> wrote:
> On Fri, Jul 19, 2013 at 12:57 PM, Richard Sharpe
> <realrichardsharpe at gmail.com> wrote:

>> We see the following code in srv_send_smb (even in master):
[...]
             ret = write_data(sconn->sock, buf_out+nwritten, len - nwritten);
[...]
>> out:
>>         SMB_PERFCOUNT_END(pcd);
>>
>>         smbd_unlock_socket(sconn);
>>         return true;
>> }
>>
>> Even if the write to the socket/fd fails, we never return false
[...]
> I think this is the fix:
>
> diff --git a/source3/smbd/process.c b/source3/smbd/process.c
> index 5ef0fd3..210a281 100644
> --- a/source3/smbd/process.c
> +++ b/source3/smbd/process.c
> @@ -194,7 +194,7 @@ out:
>         SMB_PERFCOUNT_END(pcd);
>
>         smbd_unlock_socket(sconn);
> -       return true;
> +       return ret > 0;
>  }

Erm .. [cough] .... I'm just a luser here, reading along at home, and
in no way experienced at Samba hacking (I'm an old-skool programmer in
Another Language), but (and believe me, no offence intended) ... isn't
this an absolutely horrific omission of the
"quick-lets-get-the-code-up-and-running-but-we-must-change-that-before-release"
kind ?   Preventing an important error from ever being reported ....
it may well be that no error was ever reasonably expected to be
possible at this point [idk] but when I was a trainee (Big Iron ..
different world) I'd have been beaten with a bat for ignoring a return
code.


TL;DR: These things happen when we're in a rush - but much more
importantly, isn't this the sort of thing that should have been picked
up by the numerous code safety source scanning tools (e.g. [1]) that
must have been run over the code base by now ?


I mean ... the prototype is :

  bool srv_send_smb(struct smbd_server_connection *sconn, char *buffer,
            bool do_signing, uint32_t seqnum,
            bool do_encrypt,
            struct smb_perfcount_data *pcd)

This function is clearly intended to return 'bool', yet the return
value would appear to be irrelevant (due to being constant).  There's
only one 'return' in the function body ... it's easy to analyse.

I'm probably missing something here, and I'm sure you folks have
better things to do, but if anyone could find the time to explain to
me whether this is either (a) no biggie because XYZ, or (b) a little
alarming but we're all keeping calm and running the relevant analysers
now, I'd be very grateful :-)

[1] http://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis

Cheers,
Nick
--
Beware of bugs in the above code; I have only proved it correct,
not tried it.  -- Donald Knuth