Interesting issue with NDR_NOALIGN and nbt encoding.

[Jeremy Allison]

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 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 ?

> > 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 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.

Jeremy.
-------------- next part --------------
diff --git a/librpc/ndr/libndr.h b/librpc/ndr/libndr.h
index 37a3145..9e51fd9 100644
--- a/librpc/ndr/libndr.h
+++ b/librpc/ndr/libndr.h
@@ -135,7 +135,7 @@ struct ndr_print {
 #define LIBNDR_FLAG_ALIGN4       (1<<23)
 #define LIBNDR_FLAG_ALIGN8       (1<<24)
 
-#define LIBNDR_ALIGN_FLAGS (LIBNDR_FLAG_ALIGN2|LIBNDR_FLAG_ALIGN4|LIBNDR_FLAG_ALIGN8)
+#define LIBNDR_ALIGN_FLAGS (LIBNDR_FLAG_NOALIGN|LIBNDR_FLAG_ALIGN2|LIBNDR_FLAG_ALIGN4|LIBNDR_FLAG_ALIGN8)
 
 #define LIBNDR_PRINT_ARRAY_HEX   (1<<25)
 #define LIBNDR_PRINT_SET_VALUES  (1<<26)
diff --git a/librpc/ndr/ndr.c b/librpc/ndr/ndr.c
index 2279d1c..3b83b65 100644
--- a/librpc/ndr/ndr.c
+++ b/librpc/ndr/ndr.c
@@ -383,6 +383,10 @@ _PUBLIC_ void ndr_set_flags(uint32_t *pflags, uint32_t new_flags)
 	}
 	if (new_flags & LIBNDR_ALIGN_FLAGS) {
 		(*pflags) &= ~LIBNDR_FLAG_REMAINING;
+		/* Ensure we only have the align
+		   flag set in the new_flags, remove
+		   any old align flag. */
+		(*pflags) &= ~LIBNDR_ALIGN_FLAGS;
 	}
 	if (new_flags & LIBNDR_FLAG_NO_RELATIVE_REVERSE) {
 		(*pflags) &= ~LIBNDR_FLAG_RELATIVE_REVERSE;