Interesting issue with NDR_NOALIGN and nbt encoding.
abartlet at samba.org
Wed May 23 23:52:25 MDT 2012
On Wed, 2012-05-23 at 22:32 -0700, Jeremy Allison wrote:
> On Thu, May 24, 2012 at 03:23:36PM +1000, Andrew Bartlett wrote:
> > That commit was in 2008, when we first got the WSPP docs, and I started
> > to properly handle these structures. I may not have filed a
> > release-critical bug to move the comment, but I assure you, the rest of
> > this code is very old indeed.
> > What is much more relevant is the work I did (including testing) on
> > WinXP failing to join 3.6 RC:
> > https://bugzilla.samba.org/show_bug.cgi?id=8326
> > Also for background see the change Volker made in commit
> > 2642f385887fbd3aecd4286a4d6223a21d981714
> > Author: Volker Lendecke <vl at samba.org>
> > Date: Thu Nov 17 22:36:22 2011 +0100
> > s3: Fix bug 8371
> > ndr_set_flag or's in the given flag (ALIGN4). At this point,
> > ndr->flags
> > contains NOALIGN, which will persist. In ndr_push_DATA_BLOB NOALIGN
> > overrides
> > everything else, so that the ALIGN4 is not respected.
> > Autobuild-User: Volker Lendecke <vlendec at samba.org>
> > Autobuild-Date: Fri Nov 18 09:33:37 CET 2011 on sn-devel-104
> > Metze:
> > The patch from metze on the bug
> > https://bugzilla.samba.org/show_bug.cgi?id=8371
> > https://attachments.samba.org/attachment.cgi?id=7118 does not seem to
> > have made it into master, but also seems relevant.
> No worries - I wasn't blaming you, just trying
> to understand the history of the code here. It's
> really interesting stuff and you seemed to be the
> last one touching it - and you know how that looks :-).
I do appreciate you pinging me about this, as with my work in 2008 and
the WinXP domain join bug I found just before the 3.6 release, I have
had a good deal to do with this code.
> > I doubt it will allow us not to hand-generate to code you originally
> > mentioned. Certainly for the response 'netlogon'
> > packets, the issue is that the only way to parse them is to first read
> > the last bytes, and then work forward.
> Can you explain more there ? Why is this the case ?
In netlogon_samlogon_response the union is switched on the value 8 bytes
in from the end of the packet! In nbt_netlogon_response it is similar,
the last 4 bytes.
> > > So attached is a patch to do just that. I'm testing
> > > it currently in master, but I'd really like to get
> > > feedback if I've missed something as to why it is
> > > allowed to set any of LIBNDR_FLAG_ALIGN2|LIBNDR_FLAG_ALIGN4|LIBNDR_FLAG_ALIGN8
> > > concurrently with LIBNDR_FLAG_NOALIGN.
> > What is the point of LIBNDR_FLAG_NOALIGN then if all the alignment flags
> > override it?
> Ah - they're *SUPPOSED* to override it - that's the point.
> But they do so temporarily.
> Whenever you set a [flag(NDR_ALIGNXXX)] on a field
> you're saying that field (and everything it contains)
> need to be aligned on that boundary. But it's only
> for that field.
> So the code generated looks like :
> uint32_t _flags_save_xxx = ndr->flags;
> ndr_set_flags(&ndr->flags, LIBNDR_FLAG_ALIGNY);
> NDR_CHECK(ndr_push_XXX(ndr, NDR_SCALARS, r->xxx));
> ndr->flags = _flags_save_xxx;
> Note we save off the containing ndr->flags and restore
> after dealing with the sub-field.
> So I think the correct fix here is to treat NDR_NOALIGN
> in *exactly* the same way as the other NDR_ALIGNX types,
> and when any of them is set it overrides any of the others
> previously set.
I guess that seems reasonable. It does seem to prevent noalign from
overriding those however. Effectively it means 'no default alignment',
rather than 'no alignment'. We use it in a lot of places - fixing this
for NBT could expose an assumption somewhere else.
> I think the small patch I just posted (and am attaching
> again to this email) does just that.
> > I'm not totally sure here either, except to say that we need more
> > known-correct blobs and tests for them, and that a good number of us
> > have been stumbling though different parts of this puzzle over the past
> > year, and at least we should connect the dots. It may be that some of
> > the top level NOALIGN flags are simply wrong.
> More testing gratefully received :-). I'm going to see
> what feedback my change gets from the testers of that
> 3.6.x bug.
Have we worked out what would go wrong if we simply dropped the NOALIGN
totally? Either that, or make a new flags NDR_NO_DEFAULT_ALIGN to have
it align these strings (marked with their alignment), but not a uint32?
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
More information about the samba-technical