[PATCH] Fix smbclient regression not printing session setup anymore

[Jeremy Allison]

On Wed, Jun 07, 2017 at 05:11:42PM +0200, Stefan Metzmacher via samba-technical wrote:
> Am 06.06.2017 um 18:47 schrieb Andreas Schneider via samba-technical:
> > On Tuesday, 6 June 2017 18:29:59 CEST Jeremy Allison wrote:
> >> On Tue, Jun 06, 2017 at 06:27:00PM +0200, Andreas Schneider via samba-
> > technical wrote:
> >>> On Tuesday, 6 June 2017 18:08:09 CEST Jeremy Allison wrote:
> >>>> On Tue, Jun 06, 2017 at 05:56:21PM +0200, Andreas Schneider via samba-
> >>>
> >>> technical wrote:
> >>>>> On Tuesday, 6 June 2017 17:35:52 CEST Andreas Schneider via
> >>>>> samba-technical
> >>>>>
> >>>>> wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> with Samba 4.6 we have a regression. We do not get the information
> >>>>>> of
> >>>>>> the
> >>>>>> session setup filled out so smbclient can't print it.
> >>>>>>
> >>>>>> Domain=[SAMBA-TEST] OS=[Windows 6.1] Server=[Samba
> >>>>>> 4.7.0pre1-DEVELOPERBUILD] smb: \>
> >>>>>>
> >>>>>> The attached patchset addresses the issue. I've open a bug for this:
> >>>>>>
> >>>>>> https://bugzilla.samba.org/show_bug.cgi?id=12824
> >>>>>
> >>>>> I've added a test to confirm that the message is actually printed so
> >>>>> we do
> >>>>> not regress again ...
> >>>>
> >>>> The change from 'bool align_odd = true' to 'bool align_odd = false'
> >>>> concerns me.
> >>>>
> >>>> smb_bytes_pull_str() is used from all SMB1 session code inside
> >>>> libcli/smb/smb1cli_session.c
> >>>
> >>> The code called smb_bytes_talloc_string() before and this didn't handle
> >>> align_odd before, why should it now?
> >>
> >> Sorry, I'm being dumb. What called smb_bytes_talloc_string() before ?
> > 
> > 9fffec88033a385f3ebb8fe8520b9b39c831d98f
> 
> We used to pass 'inhdr' to smb_bytes_talloc_string() before,
> that way we got the alignment correct as the unicode characters
> are aligned to the beginning of the SMB request buffer.

Yep, I remember having to get that right a *LOOONG* time
ago - that's why an alarm went off in a dim reptillian
part of my brain when I first looked at the patch :-) :-).

> In smb_bytes_pull_str() is designed to pull the string
> just from the "bytes" part of the SMB request, which starts
> at an odd offset relative to the start of the complete SMB request
> buffer. I'm not 100% sure if that's also the case for chained requests,
> but at least samba aligns the wct field of subsequent requests to a
> 4-byte boundary, see smb_splice_chain() and smb1cli_req_chain_submit().
> 
> The attached patch should improve this, but it would be better to have
> a regression test. I try to write a cmocka test for it with
> Andreas in the next days.

I'll review (and push if happy) the attached patch shortly.