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

[Richard Sharpe]

On Wed, Jul 24, 2013 at 4:45 PM, Nick Boyce <nick.boyce at gmail.com> wrote:
> 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.

So it turns out to be mostly benign. Usually, there are not a lot of
commands queued up in the socket buffers. In our case there are cases
when things can hang up in the file system and then things can pile up
in the socket buffers.

Also, it only happens for SMB1 not SMB2.


> 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



-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)